| Message ID | patch-v3-3.5-637039634e7-20210826T140414Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | bundle: show progress on "unbundle" | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > 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. As bundle API has been stable over a long period of time, I do not think the loss of flexibility is too bad (i.e. we no longer can cary any extra information that cannot be expressed as a textual argument given to the index-pack process), compared to gained flexibility of the index-pack command line we can use. > 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; > > - if (flags & BUNDLE_VERBOSE) > - argv_index_pack[3] = "-v"; > + strvec_push(&ip.args, "index-pack"); > + strvec_push(&ip.args, "--fix-thin"); > + strvec_push(&ip.args, "--stdin"); I would have expected you'd use pushl() here. > + if (extra_index_pack_args) { > + strvec_pushvec(&ip.args, extra_index_pack_args); > + 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 84a6df1b65d..d47a7a3c69c 100644 > --- a/bundle.h > +++ b/bundle.h > @@ -26,16 +26,20 @@ 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 Good to comment on this. I found it a bit surprising that the in argument "extra_index_pack_args" is eaten by the consumer, instead of leaving it up to the caller when to clear (or reuse before clear) it. > + * (like the run-command.h API itself does). This is not even run-command.h, so the mention of "run-command.h API itself" sounded strange to me. Without "itself", perhaps I would have slightly less puzzled. IIRC, run_command() only clears upon success and does not clear if start_command() fails, so it is not a very good comparison. I think the comment reads better without this last parenthesized note. > */ > 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 17e9629710a..99b2498e5dd 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); > + transport->progress ? &extra_index_pack_args : NULL); Why conditional? If transport->progress is false, extra_index_pack_args is a strvec that has 0 elements in it, and the strvec_pushvec() code knows how to handle it correctly and efficiently anyway. I do not see a reason why we want to make the caller harder to read with the same conditional twice (i.e. if we pass NULL in non-verbose case, we can unconditionally put "-v" in the array, and clear the array on the side of this caller, and that would allow us lose the clear from the unbundle() function). Other than that, looks very simple. Nicely done. Thanks.
diff --git a/builtin/bundle.c b/builtin/bundle.c index 053a51bea1b..f9360c32c6c 100644 --- a/builtin/bundle.c +++ b/builtin/bundle.c @@ -177,7 +177,7 @@ 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, NULL) || list_bundle_refs(&header, argc, argv); bundle_header_release(&header); cleanup: diff --git a/bundle.c b/bundle.c index ab63f402261..10d20bb0c48 100644 --- a/bundle.c +++ b/bundle.c @@ -569,18 +569,20 @@ int create_bundle(struct repository *r, const char *path, } 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; - if (flags & BUNDLE_VERBOSE) - argv_index_pack[3] = "-v"; + strvec_push(&ip.args, "index-pack"); + strvec_push(&ip.args, "--fix-thin"); + strvec_push(&ip.args, "--stdin"); + if (extra_index_pack_args) { + strvec_pushvec(&ip.args, extra_index_pack_args); + 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 84a6df1b65d..d47a7a3c69c 100644 --- a/bundle.h +++ b/bundle.h @@ -26,16 +26,20 @@ 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 + * (like the run-command.h API itself does). */ 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 17e9629710a..99b2498e5dd 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); + transport->progress ? &extra_index_pack_args : NULL); transport->hash_algo = data->header.hash_algo; return ret; }
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 <avarab@gmail.com> --- builtin/bundle.c | 2 +- bundle.c | 14 ++++++++------ bundle.h | 8 ++++++-- transport.c | 6 +++++- 4 files changed, 20 insertions(+), 10 deletions(-)