Re: [PATCH v3 4/6] builtin/bundle: refactor option handling for progress meter

Karthik Nayak <[email protected]> Thu, 20 Aug 2026 07:17:30 -0400
Newsgroups	org.kernel.vger.git
Message-ID	<CAOLa=ZT-Tw2gMVCBS7b62VSkSJAHyVwO7dffsFW3Q4QzQe1JZg@mail.gmail.com>
Patrick Steinhardt <[email protected]> writes:

> The git-bundle(1) command has a couple of command line options that
> relate to whether or not progress should be reported. These options
> match the options that git-pack-objects(1) expects, and consequently
> they mostly get passed through to it directly.
>
> This results in somewhat of a confusing interface: there are four
> different options that relate to whether or not progress should be
> displayed and how verbose it should be. But in reality, there's really
> only two modes:
>
>   - "--progress" and "--all-progress" result in the same outcome, which
>     is also documented as such.
>
>   - "--all-progress-implied" does nothing as we pass that argument to
>     git-pack-objects(1) unconditionally anyway.
>
> So in the end, the options only control whether or not progress should
> be displayed at all, nothing else.
>
> Refactor the interface to instead use a simple `progress` boolean. This
> makes argument handling a lot more straight-forward and it prepares us
> for the next commit, where we're migrating git-bundle(1) to the generic
> interface for generating a packfile.
>
> Signed-off-by: Patrick Steinhardt <[email protected]>
> ---
>  builtin/bundle.c | 33 ++++++++++++++++-----------------
>  1 file changed, 16 insertions(+), 17 deletions(-)
>
> diff --git a/builtin/bundle.c b/builtin/bundle.c
> index 1e170e9278..bfafadc984 100644
> --- a/builtin/bundle.c
> +++ b/builtin/bundle.c
> @@ -70,35 +70,34 @@ static int parse_options_cmd_bundle(int argc,
>  static int cmd_bundle_create(int argc, const char **argv, const char *prefix,
>  			     struct repository *repo UNUSED) {
>  	struct strvec pack_opts = STRVEC_INIT;
> +	int progress = isatty(STDERR_FILENO);
>  	int version = -1;
> -	int ret;
>  	struct option options[] = {
> -		OPT_PASSTHRU_ARGV('q', "quiet", &pack_opts, NULL,
> -				  N_("do not show progress meter"),
> -				  PARSE_OPT_NOARG),
> -		OPT_PASSTHRU_ARGV(0, "progress", &pack_opts, NULL,
> -				  N_("show progress meter"),
> -				  PARSE_OPT_NOARG),
> -		OPT_PASSTHRU_ARGV(0, "all-progress", &pack_opts, NULL,
> -				  N_("historical; same as --progress"),
> -				  PARSE_OPT_NOARG | PARSE_OPT_HIDDEN),
> -		OPT_PASSTHRU_ARGV(0, "all-progress-implied", &pack_opts, NULL,
> -				  N_("historical; does nothing"),
> -				  PARSE_OPT_NOARG | PARSE_OPT_HIDDEN),
> +		OPT_NEGBIT('q', "quiet", &progress,
> +			   N_("do not show progress meter"), 1),
> +		OPT_BIT(0, "progress", &progress,
> +			N_("show progress meter"), 1),
> +		OPT_BIT_F(0, "all-progress", &progress,
> +			  N_("historical; same as --progress"), 1,
> +			  PARSE_OPT_HIDDEN),
> +		OPT_NOOP_NOARG(0, "all-progress-implied"),
>  		OPT_INTEGER(0, "version", &version,
>  			    N_("specify bundle format version")),
>  		OPT_END()
>  	};
>

This is much nicer to read.

>  	char *bundle_file;
> -
> -	if (isatty(STDERR_FILENO))
> -		strvec_push(&pack_opts, "--progress");
> -	strvec_push(&pack_opts, "--all-progress-implied");
> +	int ret;
>
>  	argc = parse_options_cmd_bundle(argc, argv, prefix,
>  			builtin_bundle_create_usage, options, &bundle_file);
>  	/* bundle internals use argv[1] as further parameters */
>
> +	if (progress)
> +		strvec_push(&pack_opts, "--progress");
> +	else
> +		strvec_push(&pack_opts, "--quiet");
> +	strvec_push(&pack_opts, "--all-progress-implied");
> +
>

Tangent: While trying to understand this patch, I noticed that we only
list the '-q' shortform for '--quiet' in the 'git-pack-objects(1)'
documentation.

>  	if (!startup_info->have_repository)
>  		die(_("Need a repository to create a bundle."));
>  	ret = !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);
>
> --
> 2.55.0.822.g20453c30eb.dirty
signature.asc (application/pgp-signature, 690 B)