| Message ID | patch-v3-5.5-cd38b0f0fed-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: > + if (progress) { > + strvec_push(&extra_args, "-v"); > + strvec_push(&extra_args, "--progress-title"); > + strvec_push(&extra_args, _("Unbundling objects")); Nice. I would have expected to see pushl() though. > + } > + > + ret = !!unbundle(the_repository, &header, bundle_fd, progress ? > + &extra_args : NULL) || Again, I wouldn't make the &extra_args conditional to progress here. Future code change may decide to pass more args to underlying index-pack and the criteria for doing so may be different from progress. If this code cares about readability, it should uncondtionally pass &extra_args. If this code cares about readability *and* micro-optimization, then the condition should be on !!extra_args.nr, not on whatever set of conditions happen to be used in today's code to throw items into extra_args array. Other than that, this was a pleasant read. Thanks.
On Fri, Aug 27 2021, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> + if (progress) { >> + strvec_push(&extra_args, "-v"); >> + strvec_push(&extra_args, "--progress-title"); >> + strvec_push(&extra_args, _("Unbundling objects")); > > Nice. I would have expected to see pushl() though. Will fix. Thanks! >> + } >> + >> + ret = !!unbundle(the_repository, &header, bundle_fd, progress ? >> + &extra_args : NULL) || > > Again, I wouldn't make the &extra_args conditional to progress > here. Future code change may decide to pass more args to underlying > index-pack and the criteria for doing so may be different from > progress. > > If this code cares about readability, it should uncondtionally pass > &extra_args. I agree, but I landed myself in a game of reviewer ping-pong. I had it that way originally, then Derrick Stolee suggested changing it in this way in <30620e13-4509-1905-7644-9962b6adf9c5@gmail.com>, I'll just change it back :)
diff --git a/Documentation/git-bundle.txt b/Documentation/git-bundle.txt index ac0d0038350..71b5ecabd1f 100644 --- a/Documentation/git-bundle.txt +++ b/Documentation/git-bundle.txt @@ -13,7 +13,7 @@ SYNOPSIS [--version=<version>] <file> <git-rev-list-args> 'git bundle' verify [-q | --quiet] <file> 'git bundle' list-heads <file> [<refname>...] -'git bundle' unbundle <file> [<refname>...] +'git bundle' unbundle [--progress] <file> [<refname>...] DESCRIPTION ----------- diff --git a/builtin/bundle.c b/builtin/bundle.c index f9360c32c6c..1fbfe280c57 100644 --- a/builtin/bundle.c +++ b/builtin/bundle.c @@ -162,10 +162,15 @@ 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; + struct strvec extra_args = STRVEC_INIT; argc = parse_options_cmd_bundle(argc, argv, prefix, builtin_bundle_unbundle_usage, options, &bundle_file); @@ -177,7 +182,15 @@ 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, NULL) || + + if (progress) { + strvec_push(&extra_args, "-v"); + strvec_push(&extra_args, "--progress-title"); + strvec_push(&extra_args, _("Unbundling objects")); + } + + ret = !!unbundle(the_repository, &header, bundle_fd, progress ? + &extra_args : NULL) || list_bundle_refs(&header, argc, argv); bundle_header_release(&header); cleanup:
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 <avarab@gmail.com> --- Documentation/git-bundle.txt | 2 +- builtin/bundle.c | 15 ++++++++++++++- 2 files changed, 15 insertions(+), 2 deletions(-)