Re: [PATCH 1/1] multi-pack-index: add --no-progress Add --no-progress option to git multi-pack-index. The progress feature was added in 144d703 ("multi-pack-index: report progress during 'verify'", 2018-09-13) but the ability to opt-out was overlooked.

[Junio C Hamano <gitster@pobox.com>]

"William Baker via GitGitGadget" <gitgitgadget@gmail.com> writes:

>  [verse]
> -'git multi-pack-index' [--object-dir=<dir>] <subcommand>
> +'git multi-pack-index' [--object-dir=<dir>] <subcommand> [--[no-]progress]

I am wondering what the reasoning behind having this new one *after*
the subcommand while the existing one *before* is.  Isn't the
--[no-]progress option supported by all subcommands of the
multi-pack-index command, just like the --object-dir=<dir> option
is?

If there is no reason that explains why one must come before and the
other must come after the subcommand, we are then adding another
thing the end-users or scriptors need to memorize, which is not
ideal.

> +--[no-]progress::
> +	Turn progress on/off explicitly. If neither is specified, progress is 
> +	shown if standard error is connected to a terminal.

Sounds sensible.

> diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c
> index b1ea1a6aa1..f8b2a74179 100644
> --- a/builtin/multi-pack-index.c
> +++ b/builtin/multi-pack-index.c
> @@ -6,34 +6,41 @@
>  #include "trace2.h"
>  
>  static char const * const builtin_multi_pack_index_usage[] = {
> -	N_("git multi-pack-index [--object-dir=<dir>] (write|verify|expire|repack --batch-size=<size>)"),
> +	N_("git multi-pack-index [--object-dir=<dir>] (write|verify|expire|repack --batch-size=<size>) [--[no-]progress]"),
>  	NULL
>  };

The same comment as the SYNOPSIS part.

>  static struct opts_multi_pack_index {
>  	const char *object_dir;
>  	unsigned long batch_size;
> +	int progress;
>  } opts;
>  
>  int cmd_multi_pack_index(int argc, const char **argv,
>  			 const char *prefix)
>  {
> +	unsigned flags = 0;
> +
>  	static struct option builtin_multi_pack_index_options[] = {
>  		OPT_FILENAME(0, "object-dir", &opts.object_dir,
>  		  N_("object directory containing set of packfile and pack-index pairs")),
>  		OPT_MAGNITUDE(0, "batch-size", &opts.batch_size,
>  		  N_("during repack, collect pack-files of smaller size into a batch that is larger than this size")),
> +		OPT_BOOL(0, "progress", &opts.progress, N_("force progress reporting")),
>  		OPT_END(),
>  	};

Seeing that all the options that made me curious (see above) are
defined here for all subcommands, I suspect that "technically",
all options must come before the <subcommand> (side note: I know
parse-options may reorder commands after options, but in the
documentation and usage, we strongly discourage users from relying
on the reordering and always show "global --options, subcommand, and
then subcommand --options" order).  I also see in the code that
handles opts.batch_size that there is a workaround for this inverted
code structure to make sure subcommands other than repack does not
allow --batch-size option specified.

Unless and until we get rid of the "git multi-pack-index" as a
separate command (side note: when it happens, we;'d call the
underlying midx API functions directly from appropriate places in
the codepaths like "gc"), we probably would want to correct the use
of parse_options() API in the implementation of this command before
adding any new option or subcommand.

> @@ -47,14 +54,15 @@ int cmd_multi_pack_index(int argc, const char **argv,
>  	trace2_cmd_mode(argv[0]);
>  
>  	if (!strcmp(argv[0], "repack"))
> -		return midx_repack(the_repository, opts.object_dir, (size_t)opts.batch_size);
> +		return midx_repack(the_repository, opts.object_dir, 
> +			(size_t)opts.batch_size, flags);
>  	if (opts.batch_size)
>  		die(_("--batch-size option is only for 'repack' subcommand"));
>  
>  	if (!strcmp(argv[0], "write"))
>  		return write_midx_file(opts.object_dir);
>  	if (!strcmp(argv[0], "verify"))
> -		return verify_midx_file(the_repository, opts.object_dir);
> +		return verify_midx_file(the_repository, opts.object_dir, flags);
>  	if (!strcmp(argv[0], "expire"))
>  		return expire_midx_packs(the_repository, opts.object_dir);

We can see that the new option only affects "verify", even though
the SYNOPSIS and usage text pretends that everybody understands and
reacts to it.  Shouldn't it be documented just like how --batch-size
is documented that it is understood only by "repack"?

If the mid-term aspiration of this patch is to later enhance other
subcommands to also understand the progress output or verbosity
option (and if the excuse given as a response to the above analysis
is "this is just a first step, more will come later"), the instead
of adding a "unsigned flag" local variable to the function, it would
probably make much more sense to

 (1) make struct opts_multi_pack_index as a part of the public API
     between cmd_multi_pack_index() and midx.c and document it in
     midx.h;

 (2) instead of passing opts.object_dir to existing command
     implementations, pass &opts, the pointer to the whole
     structure;

 (3) add a new field "unsigned progress" to the structure, and teach
     the command line parser to flip it upon seeing "--[no-]progress".