commit-graph: drop top-level --[no-]progress

PATCH In review Taylor Blau 5 participants 10 messages Sep 18, 2021 – Sep 22, 2021 Documentation

Share & tools

Taylor Blau

author

2021-09-18 16:02 UTC #0 lore raw

cover commit-graph: drop top-level --[no-]progress

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/[email protected]/
[2]: https://lore.kernel.org/git/[email protected]/

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(-)

Signature · 1 line

2.33.0.96.g73915697e6

Taylor Blau

author

2021-09-18 16:02 UTC #1 lore raw

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

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.

Signed-off-by Taylor Blau <[email protected]>

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

builtin/commit-graph.c +4 -2

index 21fc6e934b..067587a0fd 100644
@@ -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);

Signature · 1 line

2.33.0.96.g73915697e6

Derrick Stolee

2021-09-20 12:46 UTC #2 lore raw

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

Quoted 16 lines · 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.

Quoted 8 lines

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.

Quoted 19 lines

builtin/commit-graph.c +2 -2

@@ -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);

builtin/commit-graph.c static int graph_write(int argc, const char **argv) · line 251

Quoted 6 lines

builtin/commit-graph.c +2 -0

@@ -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

Taylor Blau

author

2021-09-20 15:03 UTC #3 lore raw

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

Quoted 6 lines · Derrick Stolee

Quoted 2 lines · Taylor Blau
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/[email protected]/

Junio C Hamano

2021-09-20 21:26 UTC #4 lore raw

In reply to Taylor Blau

Quoted 3 lines · Taylor Blau

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.

Taylor Blau

author

2021-09-21 02:20 UTC #5 lore raw

Quoted 10 lines · Junio C Hamano

Quoted 3 lines · Taylor Blau
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 <[email protected]>

---
 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(-)

Documentation/git-multi-pack-index.txt +3 -3

index ffd601bc17..5ba4bd5166 100644
@@ -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:

builtin/multi-pack-index.c +26 -5

index 649aa5f9ab..5f11a3067d 100644
@@ -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,

t/t5319-multi-pack-index.sh +6 -6

index 3d4d9f10c3..86b7de2281 100755
@@ -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

Ævar Arnfjörð Bjarmason

2021-09-21 18:39 UTC #6 lore raw

Quoted 12 lines · Taylor Blau

Quoted 10 lines · Junio C Hamano
Quoted 3 lines · Taylor Blau
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.

Quoted 4 lines

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/[email protected]/

Quoted 189 lines

--- >8 ---

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

Signed-off-by Taylor Blau <[email protected]>

---
 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(-)

Documentation/git-multi-pack-index.txt +3 -3

index ffd601bc17..5ba4bd5166 100644
@@ -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:

builtin/multi-pack-index.c +26 -5

index 649aa5f9ab..5f11a3067d 100644
@@ -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,

t/t5319-multi-pack-index.sh +6 -6

index 3d4d9f10c3..86b7de2281 100755
@@ -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
 	)
 '

Taylor Blau

author

2021-09-21 20:38 UTC #7 lore raw

Quoted 29 lines · Ævar Arnfjörð Bjarmason

Quoted 12 lines · Taylor Blau
Quoted 10 lines · Junio C Hamano
Quoted 3 lines · Taylor Blau
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.
Quoted 4 lines
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]

Quoted 7 lines

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/[email protected]/

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.

Quoted 2 lines

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

Junio C Hamano

2021-09-22 16:22 UTC #8 lore raw

In reply to Taylor Blau

Quoted 9 lines · Taylor Blau

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 <[email protected]>
---

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.

Quoted 180 lines

 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(-)

Documentation/git-multi-pack-index.txt +3 -3

index ffd601bc17..5ba4bd5166 100644
@@ -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:

builtin/multi-pack-index.c +26 -5

index 649aa5f9ab..5f11a3067d 100644
@@ -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,

t/t5319-multi-pack-index.sh +6 -7

index 3d4d9f10c3..86b7de2281 100755
@@ -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

Jeff King

2021-09-21 03:55 UTC #9 lore raw

In reply to Junio C Hamano

Quoted 10 lines · Junio C Hamano

Quoted 3 lines · Taylor Blau
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