From 08342573792e9af79bf41b32c45ac471d25303bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 26 Aug 2021 16:05:47 +0200 Subject: [PATCH 1/4] bundle API: start writing API documentation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There are no other API docs in bundle.h, but this is at least a start. We'll add a parameter to this function in a subsequent commit, but let's start by documenting it. The "/**" comment (as opposed to "/*") signifies the start of API documentation. See [1] and bdfdaa4978d (strbuf.h: integrate api-strbuf.txt documentation, 2015-01-16) and 6afbbdda333 (strbuf.h: unify documentation comments beginnings, 2015-01-16) for a discussion of that convention. 1. https://lore.kernel.org/git/874kbeecfu.fsf@evledraar.gmail.com/ Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- bundle.h | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/bundle.h b/bundle.h index 1927d8cd6a..84a6df1b65 100644 --- a/bundle.h +++ b/bundle.h @@ -27,6 +27,13 @@ int create_bundle(struct repository *r, const char *path, int version); int verify_bundle(struct repository *r, struct bundle_header *header, int verbose); #define BUNDLE_VERBOSE 1 + +/** + * Unbundle after reading the header with read_bundle_header(). + * + * We'll invoke "git index-pack --stdin --fix-thin" for you on the + * provided `bundle_fd` from read_bundle_header(). + */ int unbundle(struct repository *r, struct bundle_header *header, int bundle_fd, int flags); int list_bundle_refs(struct bundle_header *header, From 7366096de9d3e4aee4b49dfdf0438a8636187a84 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Sun, 5 Sep 2021 09:34:43 +0200 Subject: [PATCH 2/4] bundle API: change "flags" to be "extra_index_pack_args" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since the "flags" parameter was added in be042aff24c (Teach progress eye-candy to fetch_refs_from_bundle(), 2011-09-18) there's never been more than the one flag: BUNDLE_VERBOSE. Let's have the only caller who cares about that pass "-v" itself instead through new "extra_index_pack_args" parameter. The flexibility of being able to pass arbitrary arguments to "unbundle" will be used in a subsequent commit. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- builtin/bundle.c | 4 +++- bundle.c | 12 ++++++------ bundle.h | 7 +++++-- transport.c | 6 +++++- 4 files changed, 19 insertions(+), 10 deletions(-) diff --git a/builtin/bundle.c b/builtin/bundle.c index 053a51bea1..9b86c8529c 100644 --- a/builtin/bundle.c +++ b/builtin/bundle.c @@ -166,6 +166,7 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix) OPT_END() }; char *bundle_file; + struct strvec extra_index_pack_args = STRVEC_INIT; argc = parse_options_cmd_bundle(argc, argv, prefix, builtin_bundle_unbundle_usage, options, &bundle_file); @@ -177,7 +178,8 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix) } if (!startup_info->have_repository) die(_("Need a repository to unbundle.")); - ret = !!unbundle(the_repository, &header, bundle_fd, 0) || + ret = !!unbundle(the_repository, &header, bundle_fd, + &extra_index_pack_args) || list_bundle_refs(&header, argc, argv); bundle_header_release(&header); cleanup: diff --git a/bundle.c b/bundle.c index ab63f40226..a0bb687b0f 100644 --- a/bundle.c +++ b/bundle.c @@ -569,18 +569,18 @@ err: } int unbundle(struct repository *r, struct bundle_header *header, - int bundle_fd, int flags) + int bundle_fd, struct strvec *extra_index_pack_args) { - const char *argv_index_pack[] = {"index-pack", - "--fix-thin", "--stdin", NULL, NULL}; struct child_process ip = CHILD_PROCESS_INIT; + strvec_pushl(&ip.args, "index-pack", "--fix-thin", "--stdin", NULL); - if (flags & BUNDLE_VERBOSE) - argv_index_pack[3] = "-v"; + if (extra_index_pack_args) { + strvec_pushv(&ip.args, extra_index_pack_args->v); + strvec_clear(extra_index_pack_args); + } if (verify_bundle(r, header, 0)) return -1; - ip.argv = argv_index_pack; ip.in = bundle_fd; ip.no_stdout = 1; ip.git_cmd = 1; diff --git a/bundle.h b/bundle.h index 84a6df1b65..06009fe6b1 100644 --- a/bundle.h +++ b/bundle.h @@ -26,16 +26,19 @@ int create_bundle(struct repository *r, const char *path, int argc, const char **argv, struct strvec *pack_options, int version); int verify_bundle(struct repository *r, struct bundle_header *header, int verbose); -#define BUNDLE_VERBOSE 1 /** * Unbundle after reading the header with read_bundle_header(). * * We'll invoke "git index-pack --stdin --fix-thin" for you on the * provided `bundle_fd` from read_bundle_header(). + * + * Provide "extra_index_pack_args" to pass any extra arguments + * (e.g. "-v" for verbose/progress), NULL otherwise. The provided + * "extra_index_pack_args" (if any) will be strvec_clear()'d for you. */ int unbundle(struct repository *r, struct bundle_header *header, - int bundle_fd, int flags); + int bundle_fd, struct strvec *extra_index_pack_args); int list_bundle_refs(struct bundle_header *header, int argc, const char **argv); diff --git a/transport.c b/transport.c index 17e9629710..ab9b03ae9f 100644 --- a/transport.c +++ b/transport.c @@ -162,12 +162,16 @@ static int fetch_refs_from_bundle(struct transport *transport, int nr_heads, struct ref **to_fetch) { struct bundle_transport_data *data = transport->data; + struct strvec extra_index_pack_args = STRVEC_INIT; int ret; + if (transport->progress) + strvec_push(&extra_index_pack_args, "-v"); + if (!data->get_refs_from_bundle_called) get_refs_from_bundle(transport, 0, NULL); ret = unbundle(the_repository, &data->header, data->fd, - transport->progress ? BUNDLE_VERBOSE : 0); + &extra_index_pack_args); transport->hash_algo = data->header.hash_algo; return ret; } From f46c46e4f22506a01aa0033b37ef027d9e87585f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Sun, 5 Sep 2021 09:34:44 +0200 Subject: [PATCH 3/4] index-pack: add --progress-title option MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a --progress-title option to index-pack, when data is piped into index-pack its progress is a proxy for whatever's feeding it data. This option will allow us to set a more relevant progress bar title in "git bundle unbundle", and is also used in my "bundle-uri" RFC patches[1] by a new caller in fetch-pack.c. The code change in cmd_index_pack() won't handle "--progress-title=xyz", only "--progress-title xyz", and the "(i+1)" style (as opposed to "i + 1") is a bit odd. Not using the "--long-option=value" style is inconsistent with existing long options handled by cmd_index_pack(), but makes the code that needs to call it better (two strvec_push(), instead of needing a strvec_pushf()). Since the option is internal-only the inconsistency shouldn't matter. I'm copying the pattern to handle it as-is from the handling of the existing "-o" option in the same function, see 9cf6d3357aa (Add git-index-pack utility, 2005-10-12) for its addition. That's a short option, but the code to implement the two is the same in functionality and style. Eventually we'd like to migrate all of this this to parse_options(), which would make these differences in behavior go away. 1. https://lore.kernel.org/git/RFC-cover-00.13-0000000000-20210805T150534Z-avarab@gmail.com/ Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- Documentation/git-index-pack.txt | 6 ++++++ builtin/index-pack.c | 6 ++++++ 2 files changed, 12 insertions(+) diff --git a/Documentation/git-index-pack.txt b/Documentation/git-index-pack.txt index 7fa74b9e79..1f1e359225 100644 --- a/Documentation/git-index-pack.txt +++ b/Documentation/git-index-pack.txt @@ -82,6 +82,12 @@ OPTIONS --strict:: Die, if the pack contains broken objects or links. +--progress-title:: + For internal use only. ++ +Set the title of the progress bar. The title is "Receiving objects" by +default and "Indexing objects" when `--stdin` is specified. + --check-self-contained-and-connected:: Die if the pack contains broken links. For internal use only. diff --git a/builtin/index-pack.c b/builtin/index-pack.c index 8336466865..0841c039ae 100644 --- a/builtin/index-pack.c +++ b/builtin/index-pack.c @@ -122,6 +122,7 @@ static int strict; static int do_fsck_object; static struct fsck_options fsck_options = FSCK_OPTIONS_MISSING_GITMODULES; static int verbose; +static const char *progress_title; static int show_resolving_progress; static int show_stat; static int check_self_contained_and_connected; @@ -1157,6 +1158,7 @@ static void parse_pack_objects(unsigned char *hash) if (verbose) progress = start_progress( + progress_title ? progress_title : from_stdin ? _("Receiving objects") : _("Indexing objects"), nr_objects); for (i = 0; i < nr_objects; i++) { @@ -1806,6 +1808,10 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix) input_len = sizeof(*hdr); } else if (!strcmp(arg, "-v")) { verbose = 1; + } else if (!strcmp(arg, "--progress-title")) { + if (progress_title || (i+1) >= argc) + usage(index_pack_usage); + progress_title = argv[++i]; } else if (!strcmp(arg, "--show-resolving-progress")) { show_resolving_progress = 1; } else if (!strcmp(arg, "--report-end-of-input")) { From d941cc4c342f4feca6be2a410ac6dc56908880ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Sun, 5 Sep 2021 09:34:45 +0200 Subject: [PATCH 4/4] bundle: show progress on "unbundle" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "unbundle" command added in 2e0afafebd8 (Add git-bundle: move objects and references by archive, 2007-02-22) did not show progress output, even though the underlying API learned how to show progress in be042aff24c (Teach progress eye-candy to fetch_refs_from_bundle(), 2011-09-18). Now we'll show "Unbundling objects" using the new --progress-title option to "git index-pack", to go with its existing "Receiving objects" and "Indexing objects" (which it shows when invoked with "--stdin", and with a pack file, respectively). Unlike "git bundle create" we don't handle "--quiet" here, nor "--all-progress" and "--all-progress-implied". Those are all specific to "create" (and "verify", in the case of "--quiet"). The structure of the existing documentation is a bit unclear, e.g. the documentation for the "--quiet" option added in 79862b6b77c (bundle-create: progress output control, 2019-11-10) only describes how it works for "create", and not for "verify". That and other issues in it should be fixed, but I'd like to avoid untangling that mess right now. Let's just support the standard "--no-progress" implicitly here, and leave cleaning up the general behavior of "git bundle" for a later change. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- Documentation/git-bundle.txt | 2 +- builtin/bundle.c | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/Documentation/git-bundle.txt b/Documentation/git-bundle.txt index ac0d003835..71b5ecabd1 100644 --- a/Documentation/git-bundle.txt +++ b/Documentation/git-bundle.txt @@ -13,7 +13,7 @@ SYNOPSIS [--version=] 'git bundle' verify [-q | --quiet] 'git bundle' list-heads [...] -'git bundle' unbundle [...] +'git bundle' unbundle [--progress] [...] DESCRIPTION ----------- diff --git a/builtin/bundle.c b/builtin/bundle.c index 9b86c8529c..91975def2d 100644 --- a/builtin/bundle.c +++ b/builtin/bundle.c @@ -162,7 +162,11 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix) struct bundle_header header = BUNDLE_HEADER_INIT; int bundle_fd = -1; int ret; + int progress = isatty(2); + struct option options[] = { + OPT_BOOL(0, "progress", &progress, + N_("show progress meter")), OPT_END() }; char *bundle_file; @@ -178,6 +182,9 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix) } if (!startup_info->have_repository) die(_("Need a repository to unbundle.")); + if (progress) + strvec_pushl(&extra_index_pack_args, "-v", "--progress-title", + _("Unbundling objects"), NULL); ret = !!unbundle(the_repository, &header, bundle_fd, &extra_index_pack_args) || list_bundle_refs(&header, argc, argv);