[PATCH 0/1] commit-graph: drop top-level --[no-]progress

[PATCH 0/1] commit-graph: drop top-level --[no-]progress

[Taylor Blau]

This patch provides one way to fix an issue that SZEDER described in [1]. The
issue is that `git commit-graph` began accepting `--[no-]progress` as a
top-level argument as an unintentional side-effect of 84e4484f12 (commit-graph:
use parse_options_concat(), 2021-08-23).

Some discussion beginning at [2] indicates a couple of reasons why this is
undesirable, and they are summarized in the patch below.

But most importantly, if we do want to get rid of the top-level
`--[no-]progress`, then we should act quickly to do it before 84e4484f12 (which
currently isn't tagged) is released.

An open question is whether the same should be done for the multi-pack-index
command, whose top-level support for `--[no-]progress` was released in v2.32.0
with 60ca94769c (builtin/multi-pack-index.c: split sub-commands, 2021-03-30).

[1]: https://lore.kernel.org/git/20210917211337.GC2118053@szeder.dev/
[2]: https://lore.kernel.org/git/YUUQzswYL5x74Tps@coredump.intra.peff.net/

Taylor Blau (1):
  builtin/commit-graph.c: don't accept common --[no-]progress

 builtin/commit-graph.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

-- 
2.33.0.96.g73915697e6

[PATCH 1/1] builtin/commit-graph.c: don't accept common --[no-]progress

[Taylor Blau]

In 84e4484f12 (commit-graph: use parse_options_concat(), 2021-08-23) we
unified common options of commit-graph's subcommands into a single
"common_opts" array.

But 84e4484f12 introduced a behavior change which is to accept the
"--[no-]progress" option before any sub-commands, e.g.,

    git commit-graph --progress write ...

Prior to that commit, the above would error out with "unknown option".

There are two issues with this behavior change. First is that the
top-level --[no-]progress is not always respected. This is because
isatty(2) is performed in the sub-commands, which unconditionally
overwrites any --[no-]progress that was given at the top-level.

But the second issue is that the existing sub-commands of commit-graph
only happen to both have a sensible interpretation of what `--progress`
or `--no-progress` means. If we ever added a sub-command which didn't
have a notion of progress, we would be forced to ignore the top-level
`--[no-]progress` altogether.

Since we haven't released a version of Git that supports --[no-]progress
as a top-level option for `git commit-graph`, let's remove it.

Suggested-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 builtin/commit-graph.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 21fc6e934b..067587a0fd 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -50,8 +50,6 @@ static struct option common_opts[] = {
 	OPT_STRING(0, "object-dir", &opts.obj_dir,
 		   N_("dir"),
 		   N_("the object directory to store the graph")),
-	OPT_BOOL(0, "progress", &opts.progress,
-		 N_("force progress reporting")),
 	OPT_END()
 };
 
@@ -95,6 +93,8 @@ static int graph_verify(int argc, const char **argv)
 	static struct option builtin_commit_graph_verify_options[] = {
 		OPT_BOOL(0, "shallow", &opts.shallow,
 			 N_("if the commit-graph is split, only verify the tip file")),
+		OPT_BOOL(0, "progress", &opts.progress,
+			 N_("force progress reporting")),
 		OPT_END(),
 	};
 	struct option *options = add_common_options(builtin_commit_graph_verify_options);
@@ -246,6 +246,8 @@ static int graph_write(int argc, const char **argv)
 		OPT_CALLBACK_F(0, "max-new-filters", &write_opts.max_new_filters,
 			NULL, N_("maximum number of changed-path Bloom filters to compute"),
 			0, write_option_max_new_filters),
+		OPT_BOOL(0, "progress", &opts.progress,
+			 N_("force progress reporting")),
 		OPT_END(),
 	};
 	struct option *options = add_common_options(builtin_commit_graph_write_options);
-- 
2.33.0.96.g73915697e6

Re: [PATCH 1/1] builtin/commit-graph.c: don't accept common --[no-]progress

[Derrick Stolee]

On 9/18/2021 12:02 PM, Taylor Blau wrote:
> In 84e4484f12 (commit-graph: use parse_options_concat(), 2021-08-23) we
> unified common options of commit-graph's subcommands into a single
> "common_opts" array.
> 
> But 84e4484f12 introduced a behavior change which is to accept the
> "--[no-]progress" option before any sub-commands, e.g.,
> 
>     git commit-graph --progress write ...
> 
> Prior to that commit, the above would error out with "unknown option".
> 
> There are two issues with this behavior change. First is that the
> top-level --[no-]progress is not always respected. This is because
> isatty(2) is performed in the sub-commands, which unconditionally
> overwrites any --[no-]progress that was given at the top-level.
> 
> But the second issue is that the existing sub-commands of commit-graph
> only happen to both have a sensible interpretation of what `--progress`
> or `--no-progress` means. If we ever added a sub-command which didn't
> have a notion of progress, we would be forced to ignore the top-level
> `--[no-]progress` altogether.
> 
> Since we haven't released a version of Git that supports --[no-]progress
> as a top-level option for `git commit-graph`, let's remove it.

I agree that is the best way to respond right now. Moving it to
top-level will need more work.

> @@ -50,8 +50,6 @@ static struct option common_opts[] = {
>  	OPT_STRING(0, "object-dir", &opts.obj_dir,
>  		   N_("dir"),
>  		   N_("the object directory to store the graph")),
> -	OPT_BOOL(0, "progress", &opts.progress,
> -		 N_("force progress reporting")),
>  	OPT_END()
>  };
>  
> @@ -95,6 +93,8 @@ static int graph_verify(int argc, const char **argv)
>  	static struct option builtin_commit_graph_verify_options[] = {
>  		OPT_BOOL(0, "shallow", &opts.shallow,
>  			 N_("if the commit-graph is split, only verify the tip file")),
> +		OPT_BOOL(0, "progress", &opts.progress,
> +			 N_("force progress reporting")),
>  		OPT_END(),
>  	};
>  	struct option *options = add_common_options(builtin_commit_graph_verify_options);
> @@ -246,6 +246,8 @@ static int graph_write(int argc, const char **argv)
>  		OPT_CALLBACK_F(0, "max-new-filters", &write_opts.max_new_filters,
>  			NULL, N_("maximum number of changed-path Bloom filters to compute"),
>  			0, write_option_max_new_filters),
> +		OPT_BOOL(0, "progress", &opts.progress,
> +			 N_("force progress reporting")),
>  		OPT_END(),

Meanwhile this diff is easy to verify.

Thanks,
-Stolee

Re: [PATCH 1/1] builtin/commit-graph.c: don't accept common --[no-]progress

[Taylor Blau]

On Mon, Sep 20, 2021 at 08:46:32AM -0400, Derrick Stolee wrote:
> On 9/18/2021 12:02 PM, Taylor Blau wrote:
> > Since we haven't released a version of Git that supports --[no-]progress
> > as a top-level option for `git commit-graph`, let's remove it.
>
> I agree that is the best way to respond right now. Moving it to
> top-level will need more work.

SZEDER posted a patch in [1] which would allow us to define a top-level
`--[no-]progress` option for the commit-graph builtin. (I'm assuming
that you meant the builtin when you said "top-level", and not git
itself).

But see some of his commentary above the patch in [1] about why we may
want to avoid applying something like his patch, in particular:

  In general, even when all subcommands of a git command understand a
  particular --option, that does not mean that it's a good idea to teach
  that option to that git command.  E.g. what if we later add another
  subcommand for which that --option doesn't make any sense?  And from
  the quoted discussion above it seems that teaching 'git commit-graph'
  the '--progress' option was not intentional at all.

This patch has the added advantage that we can always "go back" to
SZEDER's approach and make `--[no-]progress` work as an option to `git
commit-graph`. But doing this buys us some time to make sure that is the
approach we want to take.

Thanks,
Taylor

[1]: https://lore.kernel.org/git/20210917211337.GC2118053@szeder.dev/

Re: [PATCH 0/1] commit-graph: drop top-level --[no-]progress

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> An open question is whether the same should be done for the multi-pack-index
> command, whose top-level support for `--[no-]progress` was released in v2.32.0
> with 60ca94769c (builtin/multi-pack-index.c: split sub-commands, 2021-03-30).

We do not mind too much about "breaking backward compatibility" by
removing the mistaken "git multi-pack-index --progress cmd", I would
say.  It's not like people would type it once every day and removing
the "support" will break their finger-memory.

Re: [PATCH 0/1] commit-graph: drop top-level --[no-]progress

[Taylor Blau]

On Mon, Sep 20, 2021 at 02:24:04PM -0700, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > An open question is whether the same should be done for the multi-pack-index
> > command, whose top-level support for `--[no-]progress` was released in v2.32.0
> > with 60ca94769c (builtin/multi-pack-index.c: split sub-commands, 2021-03-30).
>
> We do not mind too much about "breaking backward compatibility" by
> removing the mistaken "git multi-pack-index --progress cmd", I would
> say.  It's not like people would type it once every day and removing
> the "support" will break their finger-memory.

OK; if we don't mind then we could do something like the following on
top. But if we're OK to remove the top-level `--progress` option from
the commit-graph and multi-pack-index builtins at any time, then both
patches become far less urgent.

Thanks,
Taylor

--- >8 ---

Subject: [PATCH] builtin/multi-pack-index.c: disable top-level --[no-]progress

In a similar spirit as the previous patch, let sub-commands which
support showing or hiding a progress meter handle parsing the
`--progress` or `--no-progress` option, but do not expose it as an
option to the top-level `multi-pack-index` builtin.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/git-multi-pack-index.txt |  6 ++---
 builtin/multi-pack-index.c             | 31 +++++++++++++++++++++-----
 t/t5319-multi-pack-index.sh            | 12 +++++-----
 3 files changed, 35 insertions(+), 14 deletions(-)

diff --git a/Documentation/git-multi-pack-index.txt b/Documentation/git-multi-pack-index.txt
index ffd601bc17..5ba4bd5166 100644
--- a/Documentation/git-multi-pack-index.txt
+++ b/Documentation/git-multi-pack-index.txt
@@ -9,8 +9,7 @@ git-multi-pack-index - Write and verify multi-pack-indexes
 SYNOPSIS
 --------
 [verse]
-'git multi-pack-index' [--object-dir=<dir>] [--[no-]progress]
-	[--preferred-pack=<pack>] <subcommand>
+'git multi-pack-index' [--object-dir=<dir>] <sub-command>

 DESCRIPTION
 -----------
@@ -26,7 +25,8 @@ OPTIONS

 --[no-]progress::
 	Turn progress on/off explicitly. If neither is specified, progress is
-	shown if standard error is connected to a terminal.
+	shown if standard error is connected to a terminal. Supported by
+	sub-commands `write`, `verify`, `expire`, and `repack.

 The following subcommands are available:

diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c
index 649aa5f9ab..5f11a3067d 100644
--- a/builtin/multi-pack-index.c
+++ b/builtin/multi-pack-index.c
@@ -52,7 +52,6 @@ static struct opts_multi_pack_index {
 static struct option common_opts[] = {
 	OPT_FILENAME(0, "object-dir", &opts.object_dir,
 	  N_("object directory containing set of packfile and pack-index pairs")),
-	OPT_BIT(0, "progress", &opts.flags, N_("force progress reporting"), MIDX_PROGRESS),
 	OPT_END(),
 };

@@ -68,6 +67,8 @@ static int cmd_multi_pack_index_write(int argc, const char **argv)
 		OPT_STRING(0, "preferred-pack", &opts.preferred_pack,
 			   N_("preferred-pack"),
 			   N_("pack for reuse when computing a multi-pack bitmap")),
+		OPT_BIT(0, "progress", &opts.flags,
+			N_("force progress reporting"), MIDX_PROGRESS),
 		OPT_END(),
 	};

@@ -75,6 +76,8 @@ static int cmd_multi_pack_index_write(int argc, const char **argv)

 	trace2_cmd_mode(argv[0]);

+	if (isatty(2))
+		opts.flags |= MIDX_PROGRESS;
 	argc = parse_options(argc, argv, NULL,
 			     options, builtin_multi_pack_index_write_usage,
 			     PARSE_OPT_KEEP_UNKNOWN);
@@ -90,10 +93,18 @@ static int cmd_multi_pack_index_write(int argc, const char **argv)

 static int cmd_multi_pack_index_verify(int argc, const char **argv)
 {
-	struct option *options = common_opts;
+	struct option *options;
+	static struct option builtin_multi_pack_index_verify_options[] = {
+		OPT_BIT(0, "progress", &opts.flags,
+			N_("force progress reporting"), MIDX_PROGRESS),
+		OPT_END(),
+	};
+	options = add_common_options(builtin_multi_pack_index_verify_options);

 	trace2_cmd_mode(argv[0]);

+	if (isatty(2))
+		opts.flags |= MIDX_PROGRESS;
 	argc = parse_options(argc, argv, NULL,
 			     options, builtin_multi_pack_index_verify_usage,
 			     PARSE_OPT_KEEP_UNKNOWN);
@@ -106,10 +117,18 @@ static int cmd_multi_pack_index_verify(int argc, const char **argv)

 static int cmd_multi_pack_index_expire(int argc, const char **argv)
 {
-	struct option *options = common_opts;
+	struct option *options;
+	static struct option builtin_multi_pack_index_expire_options[] = {
+		OPT_BIT(0, "progress", &opts.flags,
+			N_("force progress reporting"), MIDX_PROGRESS),
+		OPT_END(),
+	};
+	options = add_common_options(builtin_multi_pack_index_expire_options);

 	trace2_cmd_mode(argv[0]);

+	if (isatty(2))
+		opts.flags |= MIDX_PROGRESS;
 	argc = parse_options(argc, argv, NULL,
 			     options, builtin_multi_pack_index_expire_usage,
 			     PARSE_OPT_KEEP_UNKNOWN);
@@ -126,6 +145,8 @@ static int cmd_multi_pack_index_repack(int argc, const char **argv)
 	static struct option builtin_multi_pack_index_repack_options[] = {
 		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_BIT(0, "progress", &opts.flags,
+		  N_("force progress reporting"), MIDX_PROGRESS),
 		OPT_END(),
 	};

@@ -133,6 +154,8 @@ static int cmd_multi_pack_index_repack(int argc, const char **argv)

 	trace2_cmd_mode(argv[0]);

+	if (isatty(2))
+		opts.flags |= MIDX_PROGRESS;
 	argc = parse_options(argc, argv, NULL,
 			     options,
 			     builtin_multi_pack_index_repack_usage,
@@ -154,8 +177,6 @@ int cmd_multi_pack_index(int argc, const char **argv,

 	git_config(git_default_config, NULL);

-	if (isatty(2))
-		opts.flags |= MIDX_PROGRESS;
 	argc = parse_options(argc, argv, prefix,
 			     builtin_multi_pack_index_options,
 			     builtin_multi_pack_index_usage,
diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
index 3d4d9f10c3..86b7de2281 100755
--- a/t/t5319-multi-pack-index.sh
+++ b/t/t5319-multi-pack-index.sh
@@ -174,12 +174,12 @@ test_expect_success 'write progress off for redirected stderr' '
 '

 test_expect_success 'write force progress on for stderr' '
-	GIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir --progress write 2>err &&
+	GIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir write --progress 2>err &&
 	test_file_not_empty err
 '

 test_expect_success 'write with the --no-progress option' '
-	GIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir --no-progress write 2>err &&
+	GIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir write --no-progress 2>err &&
 	test_line_count = 0 err
 '

@@ -429,12 +429,12 @@ test_expect_success 'repack progress off for redirected stderr' '
 '

 test_expect_success 'repack force progress on for stderr' '
-	GIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir --progress repack 2>err &&
+	GIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir repack --progress 2>err &&
 	test_file_not_empty err
 '

 test_expect_success 'repack with the --no-progress option' '
-	GIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir --no-progress repack 2>err &&
+	GIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir repack --no-progress 2>err &&
 	test_line_count = 0 err
 '

@@ -618,7 +618,7 @@ test_expect_success 'expire progress off for redirected stderr' '
 test_expect_success 'expire force progress on for stderr' '
 	(
 		cd dup &&
-		GIT_PROGRESS_DELAY=0 git multi-pack-index --progress expire 2>err &&
+		GIT_PROGRESS_DELAY=0 git multi-pack-index expire --progress 2>err &&
 		test_file_not_empty err
 	)
 '
@@ -626,7 +626,7 @@ test_expect_success 'expire force progress on for stderr' '
 test_expect_success 'expire with the --no-progress option' '
 	(
 		cd dup &&
-		GIT_PROGRESS_DELAY=0 git multi-pack-index --no-progress expire 2>err &&
+		GIT_PROGRESS_DELAY=0 git multi-pack-index expire --no-progress 2>err &&
 		test_line_count = 0 err
 	)
 '
--
2.33.0.96.g73915697e6

Re: [PATCH 0/1] commit-graph: drop top-level --[no-]progress

[Ævar Arnfjörð Bjarmason]

On Mon, Sep 20 2021, Taylor Blau wrote:

> On Mon, Sep 20, 2021 at 02:24:04PM -0700, Junio C Hamano wrote:
>> Taylor Blau <me@ttaylorr.com> writes:
>>
>> > An open question is whether the same should be done for the multi-pack-index
>> > command, whose top-level support for `--[no-]progress` was released in v2.32.0
>> > with 60ca94769c (builtin/multi-pack-index.c: split sub-commands, 2021-03-30).
>>
>> We do not mind too much about "breaking backward compatibility" by
>> removing the mistaken "git multi-pack-index --progress cmd", I would
>> say.  It's not like people would type it once every day and removing
>> the "support" will break their finger-memory.
>
> OK; if we don't mind then we could do something like the following on
> top. But if we're OK to remove the top-level `--progress` option from
> the commit-graph and multi-pack-index builtins at any time, then both
> patches become far less urgent.

I think just taking both this and your commit-graph patches is the right
thing to do at this point. I.e. we almost entirely take:

    git [git-opts] <cmd> [cmd-opts]

Or:

    git [git-opts] <cmd> <subcmd> [subcmd-opts]

And almost never:

    git [git-opts] <cmd> [global-subcmd-opts] <subcmd> [subcmd-opts]

A notable exception is the --object-dir (I think I found out from
Derrick at some point why that was even needed v.s. the top-level
--git-dir, but I can't remember).

I think that fixing th commit-graph regression I introduced is an
obvious thing to do at this point, and likewise with multi-pack-index I
think it's young enough that we can just change it and not live with the
wart forever.

And as a general thing, we really should be marking all new built-ins as
having an experimental interface for the first N releases, to catch and
correct these sorts of issues.

But just as a *general* comment on where our UI should and shouldn't be
headed, I find your [1] an entirely unconvincing reply to [2]. I.e.:

    [...] the [...] issue is that the existing sub-commands of
    commit-graph only happen to both have a sensible interpretation of
    what `--progress` or `--no-progress` means. If we ever added a
    sub-command which didn't have a notion of progress, we would be
    forced to ignore the top-level `--[no-]progress` altogether.

I'd think if anything that's an argument for pursuing the
[global-subcmd-opts] for these sorts of options, i.e.:

 * We're not talking about a --find option or something that's likely to
   have different meanings.

   If using a [global-subcmd-opts] pattern means that we can't have some
   command have a --progress that means something entirely
   different. Then that to me seems like an obvious argument for the
   opposite point, i.e. that oddity should name its option something
   else, just as we're spared the confusion of not having --exec-path or
   whatever behave differently per-command.

 * I really don't see how for an option like --progress that we've got
   en established pattern working the same way across the board, why
   it's an issue that something accepts a --progress and doesn't do
   anything with it yet, or ever.

   We don't insist on our config system being configured to the command
   or subcommand level, I don't see why in terms of implementation or
   ease of user understanding why it would be desirable to treat this
   differently.

   I.e. not everything's a --progress or core.pager, but some things
   are, and having those things be closer to global than not is useful.

Anyway, as I noted let's take both of these patches now. I just wrote
this out mainly for my own future reference. I am interesting in
extending parse_options() and the config system into something that
allows you to all-at-once define commands and subcommands and have
what's now a bunch of duplicated and subtly different code done for you
automatically.

In that scenario I don't see why in terms of both commands and config
why we wouldn't define things in terms of "levels" and always understand
certain things at certain levels, and pass them down. E.g. --exec-path
and --object-format at the first, maybe --progress or
--output-format=json at the second (between a command and subcommand)
level etc.

1. https://lore.kernel.org/git/e41e65ddf77c596a7926e75bfc15f21c075d0f03.1631980949.git.me@ttaylorr.com/
2. https://lore.kernel.org/git/87zgsad6mn.fsf@evledraar.gmail.com/


> --- >8 ---
>
> Subject: [PATCH] builtin/multi-pack-index.c: disable top-level --[no-]progress
>
> In a similar spirit as the previous patch, let sub-commands which
> support showing or hiding a progress meter handle parsing the
> `--progress` or `--no-progress` option, but do not expose it as an
> option to the top-level `multi-pack-index` builtin.
>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
>  Documentation/git-multi-pack-index.txt |  6 ++---
>  builtin/multi-pack-index.c             | 31 +++++++++++++++++++++-----
>  t/t5319-multi-pack-index.sh            | 12 +++++-----
>  3 files changed, 35 insertions(+), 14 deletions(-)
>
> diff --git a/Documentation/git-multi-pack-index.txt b/Documentation/git-multi-pack-index.txt
> index ffd601bc17..5ba4bd5166 100644
> --- a/Documentation/git-multi-pack-index.txt
> +++ b/Documentation/git-multi-pack-index.txt
> @@ -9,8 +9,7 @@ git-multi-pack-index - Write and verify multi-pack-indexes
>  SYNOPSIS
>  --------
>  [verse]
> -'git multi-pack-index' [--object-dir=<dir>] [--[no-]progress]
> -	[--preferred-pack=<pack>] <subcommand>
> +'git multi-pack-index' [--object-dir=<dir>] <sub-command>
>
>  DESCRIPTION
>  -----------
> @@ -26,7 +25,8 @@ OPTIONS
>
>  --[no-]progress::
>  	Turn progress on/off explicitly. If neither is specified, progress is
> -	shown if standard error is connected to a terminal.
> +	shown if standard error is connected to a terminal. Supported by
> +	sub-commands `write`, `verify`, `expire`, and `repack.
>
>  The following subcommands are available:
>
> diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c
> index 649aa5f9ab..5f11a3067d 100644
> --- a/builtin/multi-pack-index.c
> +++ b/builtin/multi-pack-index.c
> @@ -52,7 +52,6 @@ static struct opts_multi_pack_index {
>  static struct option common_opts[] = {
>  	OPT_FILENAME(0, "object-dir", &opts.object_dir,
>  	  N_("object directory containing set of packfile and pack-index pairs")),
> -	OPT_BIT(0, "progress", &opts.flags, N_("force progress reporting"), MIDX_PROGRESS),
>  	OPT_END(),
>  };
>
> @@ -68,6 +67,8 @@ static int cmd_multi_pack_index_write(int argc, const char **argv)
>  		OPT_STRING(0, "preferred-pack", &opts.preferred_pack,
>  			   N_("preferred-pack"),
>  			   N_("pack for reuse when computing a multi-pack bitmap")),
> +		OPT_BIT(0, "progress", &opts.flags,
> +			N_("force progress reporting"), MIDX_PROGRESS),
>  		OPT_END(),
>  	};
>
> @@ -75,6 +76,8 @@ static int cmd_multi_pack_index_write(int argc, const char **argv)
>
>  	trace2_cmd_mode(argv[0]);
>
> +	if (isatty(2))
> +		opts.flags |= MIDX_PROGRESS;
>  	argc = parse_options(argc, argv, NULL,
>  			     options, builtin_multi_pack_index_write_usage,
>  			     PARSE_OPT_KEEP_UNKNOWN);
> @@ -90,10 +93,18 @@ static int cmd_multi_pack_index_write(int argc, const char **argv)
>
>  static int cmd_multi_pack_index_verify(int argc, const char **argv)
>  {
> -	struct option *options = common_opts;
> +	struct option *options;
> +	static struct option builtin_multi_pack_index_verify_options[] = {
> +		OPT_BIT(0, "progress", &opts.flags,
> +			N_("force progress reporting"), MIDX_PROGRESS),
> +		OPT_END(),
> +	};
> +	options = add_common_options(builtin_multi_pack_index_verify_options);
>
>  	trace2_cmd_mode(argv[0]);
>
> +	if (isatty(2))
> +		opts.flags |= MIDX_PROGRESS;
>  	argc = parse_options(argc, argv, NULL,
>  			     options, builtin_multi_pack_index_verify_usage,
>  			     PARSE_OPT_KEEP_UNKNOWN);
> @@ -106,10 +117,18 @@ static int cmd_multi_pack_index_verify(int argc, const char **argv)
>
>  static int cmd_multi_pack_index_expire(int argc, const char **argv)
>  {
> -	struct option *options = common_opts;
> +	struct option *options;
> +	static struct option builtin_multi_pack_index_expire_options[] = {
> +		OPT_BIT(0, "progress", &opts.flags,
> +			N_("force progress reporting"), MIDX_PROGRESS),
> +		OPT_END(),
> +	};
> +	options = add_common_options(builtin_multi_pack_index_expire_options);
>
>  	trace2_cmd_mode(argv[0]);
>
> +	if (isatty(2))
> +		opts.flags |= MIDX_PROGRESS;
>  	argc = parse_options(argc, argv, NULL,
>  			     options, builtin_multi_pack_index_expire_usage,
>  			     PARSE_OPT_KEEP_UNKNOWN);
> @@ -126,6 +145,8 @@ static int cmd_multi_pack_index_repack(int argc, const char **argv)
>  	static struct option builtin_multi_pack_index_repack_options[] = {
>  		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_BIT(0, "progress", &opts.flags,
> +		  N_("force progress reporting"), MIDX_PROGRESS),
>  		OPT_END(),
>  	};
>
> @@ -133,6 +154,8 @@ static int cmd_multi_pack_index_repack(int argc, const char **argv)
>
>  	trace2_cmd_mode(argv[0]);
>
> +	if (isatty(2))
> +		opts.flags |= MIDX_PROGRESS;
>  	argc = parse_options(argc, argv, NULL,
>  			     options,
>  			     builtin_multi_pack_index_repack_usage,
> @@ -154,8 +177,6 @@ int cmd_multi_pack_index(int argc, const char **argv,
>
>  	git_config(git_default_config, NULL);
>
> -	if (isatty(2))
> -		opts.flags |= MIDX_PROGRESS;
>  	argc = parse_options(argc, argv, prefix,
>  			     builtin_multi_pack_index_options,
>  			     builtin_multi_pack_index_usage,
> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
> index 3d4d9f10c3..86b7de2281 100755
> --- a/t/t5319-multi-pack-index.sh
> +++ b/t/t5319-multi-pack-index.sh
> @@ -174,12 +174,12 @@ test_expect_success 'write progress off for redirected stderr' '
>  '
>
>  test_expect_success 'write force progress on for stderr' '
> -	GIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir --progress write 2>err &&
> +	GIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir write --progress 2>err &&
>  	test_file_not_empty err
>  '
>
>  test_expect_success 'write with the --no-progress option' '
> -	GIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir --no-progress write 2>err &&
> +	GIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir write --no-progress 2>err &&
>  	test_line_count = 0 err
>  '
>
> @@ -429,12 +429,12 @@ test_expect_success 'repack progress off for redirected stderr' '
>  '
>
>  test_expect_success 'repack force progress on for stderr' '
> -	GIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir --progress repack 2>err &&
> +	GIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir repack --progress 2>err &&
>  	test_file_not_empty err
>  '
>
>  test_expect_success 'repack with the --no-progress option' '
> -	GIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir --no-progress repack 2>err &&
> +	GIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir repack --no-progress 2>err &&
>  	test_line_count = 0 err
>  '
>
> @@ -618,7 +618,7 @@ test_expect_success 'expire progress off for redirected stderr' '
>  test_expect_success 'expire force progress on for stderr' '
>  	(
>  		cd dup &&
> -		GIT_PROGRESS_DELAY=0 git multi-pack-index --progress expire 2>err &&
> +		GIT_PROGRESS_DELAY=0 git multi-pack-index expire --progress 2>err &&
>  		test_file_not_empty err
>  	)
>  '
> @@ -626,7 +626,7 @@ test_expect_success 'expire force progress on for stderr' '
>  test_expect_success 'expire with the --no-progress option' '
>  	(
>  		cd dup &&
> -		GIT_PROGRESS_DELAY=0 git multi-pack-index --no-progress expire 2>err &&
> +		GIT_PROGRESS_DELAY=0 git multi-pack-index expire --no-progress 2>err &&
>  		test_line_count = 0 err
>  	)
>  '

Re: [PATCH 0/1] commit-graph: drop top-level --[no-]progress

[Taylor Blau]

On Tue, Sep 21, 2021 at 08:19:47PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
> On Mon, Sep 20 2021, Taylor Blau wrote:
>
> > On Mon, Sep 20, 2021 at 02:24:04PM -0700, Junio C Hamano wrote:
> >> Taylor Blau <me@ttaylorr.com> writes:
> >>
> >> > An open question is whether the same should be done for the multi-pack-index
> >> > command, whose top-level support for `--[no-]progress` was released in v2.32.0
> >> > with 60ca94769c (builtin/multi-pack-index.c: split sub-commands, 2021-03-30).
> >>
> >> We do not mind too much about "breaking backward compatibility" by
> >> removing the mistaken "git multi-pack-index --progress cmd", I would
> >> say.  It's not like people would type it once every day and removing
> >> the "support" will break their finger-memory.
> >
> > OK; if we don't mind then we could do something like the following on
> > top. But if we're OK to remove the top-level `--progress` option from
> > the commit-graph and multi-pack-index builtins at any time, then both
> > patches become far less urgent.
>
> I think just taking both this and your commit-graph patches is the right
> thing to do at this point. I.e. we almost entirely take:
>
>     git [git-opts] <cmd> [cmd-opts]
>
> Or:
>
>     git [git-opts] <cmd> <subcmd> [subcmd-opts]
>
> And almost never:
>
>     git [git-opts] <cmd> [global-subcmd-opts] <subcmd> [subcmd-opts]
>
> A notable exception is the --object-dir (I think I found out from
> Derrick at some point why that was even needed v.s. the top-level
> --git-dir, but I can't remember).

There's a good explanation in:

    https://lore.kernel.org/git/22366f81-65a6-55d1-706c-59f877127be0@gmail.com/

and a lot of related discussion happening throughout that whole
sub-thread. The gist is that it's to be able to treat directories that
look like they are a repository's object store (but don't actually
belong to any real repository) as if they are an alternate.

> But just as a *general* comment on where our UI should and shouldn't be
> headed, I find your [1] an entirely unconvincing reply to [2]. I.e.:

I think that's a fine argument in the other direction. But to be fair, I
consider the top-level '-c foo.bar=baz' to be different than a
sub-command of the `commit-graph` builtin supporting `--progress`.

Perhaps you consider these the same, and I could even understand why.
But to me, at least, I would be disappointed if we introduced a new
sub-command of commit-graph which didn't generate a progress meter,
while still accepting `--progress`.

In other words, as a user, I would be somewhat confused if I didn't
know any better to have asked for `--progress` in a mode which no
progress will be generated. I imagine it would be confusing not to see
any output *and* not have `--progress` be rejected as an unrecognized
option.

Anyway. To be honest, I find this whole discussion a little too
theoretical for my taste. I think the patch(es) that I wrote for
commit-graph and multi-pack-index seem relatively uncontroversial, and
(at least in the commit-graph case) fix a real problem that we could
avoid leaking out into a release.

Thanks,
Taylor

Re: [PATCH 0/1] commit-graph: drop top-level --[no-]progress

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> Subject: [PATCH] builtin/multi-pack-index.c: disable top-level --[no-]progress
>
> In a similar spirit as the previous patch, let sub-commands which
> support showing or hiding a progress meter handle parsing the
> `--progress` or `--no-progress` option, but do not expose it as an
> option to the top-level `multi-pack-index` builtin.
>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---

Hmph, so a few older releases accepts --progress at a wrong point on
the command line but going forward we will correct it?

OK.  Then I buy the mention of "as the _previous_ patch" above ;-)

Thanks.

>  Documentation/git-multi-pack-index.txt |  6 ++---
>  builtin/multi-pack-index.c             | 31 +++++++++++++++++++++-----
>  t/t5319-multi-pack-index.sh            | 12 +++++-----
>  3 files changed, 35 insertions(+), 14 deletions(-)
>
> diff --git a/Documentation/git-multi-pack-index.txt b/Documentation/git-multi-pack-index.txt
> index ffd601bc17..5ba4bd5166 100644
> --- a/Documentation/git-multi-pack-index.txt
> +++ b/Documentation/git-multi-pack-index.txt
> @@ -9,8 +9,7 @@ git-multi-pack-index - Write and verify multi-pack-indexes
>  SYNOPSIS
>  --------
>  [verse]
> -'git multi-pack-index' [--object-dir=<dir>] [--[no-]progress]
> -	[--preferred-pack=<pack>] <subcommand>
> +'git multi-pack-index' [--object-dir=<dir>] <sub-command>
>
>  DESCRIPTION
>  -----------
> @@ -26,7 +25,8 @@ OPTIONS
>
>  --[no-]progress::
>  	Turn progress on/off explicitly. If neither is specified, progress is
> -	shown if standard error is connected to a terminal.
> +	shown if standard error is connected to a terminal. Supported by
> +	sub-commands `write`, `verify`, `expire`, and `repack.
>
>  The following subcommands are available:
>
> diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c
> index 649aa5f9ab..5f11a3067d 100644
> --- a/builtin/multi-pack-index.c
> +++ b/builtin/multi-pack-index.c
> @@ -52,7 +52,6 @@ static struct opts_multi_pack_index {
>  static struct option common_opts[] = {
>  	OPT_FILENAME(0, "object-dir", &opts.object_dir,
>  	  N_("object directory containing set of packfile and pack-index pairs")),
> -	OPT_BIT(0, "progress", &opts.flags, N_("force progress reporting"), MIDX_PROGRESS),
>  	OPT_END(),
>  };
>
> @@ -68,6 +67,8 @@ static int cmd_multi_pack_index_write(int argc, const char **argv)
>  		OPT_STRING(0, "preferred-pack", &opts.preferred_pack,
>  			   N_("preferred-pack"),
>  			   N_("pack for reuse when computing a multi-pack bitmap")),
> +		OPT_BIT(0, "progress", &opts.flags,
> +			N_("force progress reporting"), MIDX_PROGRESS),
>  		OPT_END(),
>  	};
>
> @@ -75,6 +76,8 @@ static int cmd_multi_pack_index_write(int argc, const char **argv)
>
>  	trace2_cmd_mode(argv[0]);
>
> +	if (isatty(2))
> +		opts.flags |= MIDX_PROGRESS;
>  	argc = parse_options(argc, argv, NULL,
>  			     options, builtin_multi_pack_index_write_usage,
>  			     PARSE_OPT_KEEP_UNKNOWN);
> @@ -90,10 +93,18 @@ static int cmd_multi_pack_index_write(int argc, const char **argv)
>
>  static int cmd_multi_pack_index_verify(int argc, const char **argv)
>  {
> -	struct option *options = common_opts;
> +	struct option *options;
> +	static struct option builtin_multi_pack_index_verify_options[] = {
> +		OPT_BIT(0, "progress", &opts.flags,
> +			N_("force progress reporting"), MIDX_PROGRESS),
> +		OPT_END(),
> +	};
> +	options = add_common_options(builtin_multi_pack_index_verify_options);
>
>  	trace2_cmd_mode(argv[0]);
>
> +	if (isatty(2))
> +		opts.flags |= MIDX_PROGRESS;
>  	argc = parse_options(argc, argv, NULL,
>  			     options, builtin_multi_pack_index_verify_usage,
>  			     PARSE_OPT_KEEP_UNKNOWN);
> @@ -106,10 +117,18 @@ static int cmd_multi_pack_index_verify(int argc, const char **argv)
>
>  static int cmd_multi_pack_index_expire(int argc, const char **argv)
>  {
> -	struct option *options = common_opts;
> +	struct option *options;
> +	static struct option builtin_multi_pack_index_expire_options[] = {
> +		OPT_BIT(0, "progress", &opts.flags,
> +			N_("force progress reporting"), MIDX_PROGRESS),
> +		OPT_END(),
> +	};
> +	options = add_common_options(builtin_multi_pack_index_expire_options);
>
>  	trace2_cmd_mode(argv[0]);
>
> +	if (isatty(2))
> +		opts.flags |= MIDX_PROGRESS;
>  	argc = parse_options(argc, argv, NULL,
>  			     options, builtin_multi_pack_index_expire_usage,
>  			     PARSE_OPT_KEEP_UNKNOWN);
> @@ -126,6 +145,8 @@ static int cmd_multi_pack_index_repack(int argc, const char **argv)
>  	static struct option builtin_multi_pack_index_repack_options[] = {
>  		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_BIT(0, "progress", &opts.flags,
> +		  N_("force progress reporting"), MIDX_PROGRESS),
>  		OPT_END(),
>  	};
>
> @@ -133,6 +154,8 @@ static int cmd_multi_pack_index_repack(int argc, const char **argv)
>
>  	trace2_cmd_mode(argv[0]);
>
> +	if (isatty(2))
> +		opts.flags |= MIDX_PROGRESS;
>  	argc = parse_options(argc, argv, NULL,
>  			     options,
>  			     builtin_multi_pack_index_repack_usage,
> @@ -154,8 +177,6 @@ int cmd_multi_pack_index(int argc, const char **argv,
>
>  	git_config(git_default_config, NULL);
>
> -	if (isatty(2))
> -		opts.flags |= MIDX_PROGRESS;
>  	argc = parse_options(argc, argv, prefix,
>  			     builtin_multi_pack_index_options,
>  			     builtin_multi_pack_index_usage,
> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
> index 3d4d9f10c3..86b7de2281 100755
> --- a/t/t5319-multi-pack-index.sh
> +++ b/t/t5319-multi-pack-index.sh
> @@ -174,12 +174,12 @@ test_expect_success 'write progress off for redirected stderr' '
>  '
>
>  test_expect_success 'write force progress on for stderr' '
> -	GIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir --progress write 2>err &&
> +	GIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir write --progress 2>err &&
>  	test_file_not_empty err
>  '
>
>  test_expect_success 'write with the --no-progress option' '
> -	GIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir --no-progress write 2>err &&
> +	GIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir write --no-progress 2>err &&
>  	test_line_count = 0 err
>  '
>
> @@ -429,12 +429,12 @@ test_expect_success 'repack progress off for redirected stderr' '
>  '
>
>  test_expect_success 'repack force progress on for stderr' '
> -	GIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir --progress repack 2>err &&
> +	GIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir repack --progress 2>err &&
>  	test_file_not_empty err
>  '
>
>  test_expect_success 'repack with the --no-progress option' '
> -	GIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir --no-progress repack 2>err &&
> +	GIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir repack --no-progress 2>err &&
>  	test_line_count = 0 err
>  '
>
> @@ -618,7 +618,7 @@ test_expect_success 'expire progress off for redirected stderr' '
>  test_expect_success 'expire force progress on for stderr' '
>  	(
>  		cd dup &&
> -		GIT_PROGRESS_DELAY=0 git multi-pack-index --progress expire 2>err &&
> +		GIT_PROGRESS_DELAY=0 git multi-pack-index expire --progress 2>err &&
>  		test_file_not_empty err
>  	)
>  '
> @@ -626,7 +626,7 @@ test_expect_success 'expire force progress on for stderr' '
>  test_expect_success 'expire with the --no-progress option' '
>  	(
>  		cd dup &&
> -		GIT_PROGRESS_DELAY=0 git multi-pack-index --no-progress expire 2>err &&
> +		GIT_PROGRESS_DELAY=0 git multi-pack-index expire --no-progress 2>err &&
>  		test_line_count = 0 err
>  	)
>  '
> --
> 2.33.0.96.g73915697e6

Re: [PATCH 0/1] commit-graph: drop top-level --[no-]progress

[Jeff King]

On Mon, Sep 20, 2021 at 02:24:04PM -0700, Junio C Hamano wrote:

> Taylor Blau <me@ttaylorr.com> writes:
> 
> > An open question is whether the same should be done for the multi-pack-index
> > command, whose top-level support for `--[no-]progress` was released in v2.32.0
> > with 60ca94769c (builtin/multi-pack-index.c: split sub-commands, 2021-03-30).
> 
> We do not mind too much about "breaking backward compatibility" by
> removing the mistaken "git multi-pack-index --progress cmd", I would
> say.  It's not like people would type it once every day and removing
> the "support" will break their finger-memory.

Just to play devil's advocate: it is possible that somebody scripted it,
since it is after all a pretty plumbing-ish command.

I do find it somewhat unlikely, though, given how little time it has
been around, how unlike the rest of Git it is, and how odd it seems for
scripts to ask for --progress (though maybe --no-progress is more
likely?).

So I'm OK with it, but I think "finger-memory" is not the whole story.

-Peff