{"thread":{"id":"56533","subject":"[PATCH 0/1] commit-graph: drop top-level --[no-]progress","startedAt":"2021-09-18T16:02:37Z","lastAt":"2021-09-22T16:22:58Z","messageCount":10,"participants":["Taylor Blau","Derrick Stolee","Junio C Hamano","Jeff King","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"436312","messageId":"cover.1631980949.git.me@ttaylorr.com","threadId":"56533","inReplyTo":null,"subject":"[PATCH 0/1] commit-graph: drop top-level --[no-]progress","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-18T16:02:34Z","receivedAt":"2021-09-18T16:02:37Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This patch provides one way to fix an issue that SZEDER described in [1]. The\nissue is that `git commit-graph` began accepting `--[no-]progress` as a\ntop-level argument as an unintentional side-effect of 84e4484f12 (commit-graph:\nuse parse_options_concat(), 2021-08-23).\n\nSome discussion beginning at [2] indicates a couple of reasons why this is\nundesirable, and they are summarized in the patch below.\n\nBut most importantly, if we do want to get rid of the top-level\n`--[no-]progress`, then we should act quickly to do it before 84e4484f12 (which\ncurrently isn't tagged) is released.\n\nAn open question is whether the same should be done for the multi-pack-index\ncommand, whose top-level support for `--[no-]progress` was released in v2.32.0\nwith 60ca94769c (builtin/multi-pack-index.c: split sub-commands, 2021-03-30).\n\n[1]: https://lore.kernel.org/git/20210917211337.GC2118053@szeder.dev/\n[2]: https://lore.kernel.org/git/YUUQzswYL5x74Tps@coredump.intra.peff.net/\n\nTaylor Blau (1):\n  builtin/commit-graph.c: don't accept common --[no-]progress\n\n builtin/commit-graph.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\n-- \n2.33.0.96.g73915697e6\n"},{"id":"436313","messageId":"e41e65ddf77c596a7926e75bfc15f21c075d0f03.1631980949.git.me@ttaylorr.com","threadId":"56533","inReplyTo":"cover.1631980949.git.me@ttaylorr.com","subject":"[PATCH 1/1] builtin/commit-graph.c: don't accept common --[no-]progress","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-18T16:02:37Z","receivedAt":"2021-09-18T16:02:40Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In 84e4484f12 (commit-graph: use parse_options_concat(), 2021-08-23) we\nunified common options of commit-graph's subcommands into a single\n\"common_opts\" array.\n\nBut 84e4484f12 introduced a behavior change which is to accept the\n\"--[no-]progress\" option before any sub-commands, e.g.,\n\n    git commit-graph --progress write ...\n\nPrior to that commit, the above would error out with \"unknown option\".\n\nThere are two issues with this behavior change. First is that the\ntop-level --[no-]progress is not always respected. This is because\nisatty(2) is performed in the sub-commands, which unconditionally\noverwrites any --[no-]progress that was given at the top-level.\n\nBut the second issue is that the existing sub-commands of commit-graph\nonly happen to both have a sensible interpretation of what `--progress`\nor `--no-progress` means. If we ever added a sub-command which didn't\nhave a notion of progress, we would be forced to ignore the top-level\n`--[no-]progress` altogether.\n\nSince we haven't released a version of Git that supports --[no-]progress\nas a top-level option for `git commit-graph`, let's remove it.\n\nSuggested-by: SZEDER Gábor <szeder.dev@gmail.com>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/commit-graph.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 21fc6e934b..067587a0fd 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -50,8 +50,6 @@ static struct option common_opts[] = {\n \tOPT_STRING(0, \"object-dir\", &opts.obj_dir,\n \t\t   N_(\"dir\"),\n \t\t   N_(\"the object directory to store the graph\")),\n-\tOPT_BOOL(0, \"progress\", &opts.progress,\n-\t\t N_(\"force progress reporting\")),\n \tOPT_END()\n };\n \n@@ -95,6 +93,8 @@ static int graph_verify(int argc, const char **argv)\n \tstatic struct option builtin_commit_graph_verify_options[] = {\n \t\tOPT_BOOL(0, \"shallow\", &opts.shallow,\n \t\t\t N_(\"if the commit-graph is split, only verify the tip file\")),\n+\t\tOPT_BOOL(0, \"progress\", &opts.progress,\n+\t\t\t N_(\"force progress reporting\")),\n \t\tOPT_END(),\n \t};\n \tstruct option *options = add_common_options(builtin_commit_graph_verify_options);\n@@ -246,6 +246,8 @@ static int graph_write(int argc, const char **argv)\n \t\tOPT_CALLBACK_F(0, \"max-new-filters\", &write_opts.max_new_filters,\n \t\t\tNULL, N_(\"maximum number of changed-path Bloom filters to compute\"),\n \t\t\t0, write_option_max_new_filters),\n+\t\tOPT_BOOL(0, \"progress\", &opts.progress,\n+\t\t\t N_(\"force progress reporting\")),\n \t\tOPT_END(),\n \t};\n \tstruct option *options = add_common_options(builtin_commit_graph_write_options);\n-- \n2.33.0.96.g73915697e6\n"},{"id":"436413","messageId":"088241fc-4070-95ea-b5eb-01c7771ee3af@gmail.com","threadId":"56533","inReplyTo":"e41e65ddf77c596a7926e75bfc15f21c075d0f03.1631980949.git.me@ttaylorr.com","subject":"Re: [PATCH 1/1] builtin/commit-graph.c: don't accept common --[no-]progress","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-09-20T12:46:32Z","receivedAt":"2021-09-20T12:46:36Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/18/2021 12:02 PM, Taylor Blau wrote:\n> In 84e4484f12 (commit-graph: use parse_options_concat(), 2021-08-23) we\n> unified common options of commit-graph's subcommands into a single\n> \"common_opts\" array.\n> \n> But 84e4484f12 introduced a behavior change which is to accept the\n> \"--[no-]progress\" option before any sub-commands, e.g.,\n> \n>     git commit-graph --progress write ...\n> \n> Prior to that commit, the above would error out with \"unknown option\".\n> \n> There are two issues with this behavior change. First is that the\n> top-level --[no-]progress is not always respected. This is because\n> isatty(2) is performed in the sub-commands, which unconditionally\n> overwrites any --[no-]progress that was given at the top-level.\n> \n> But the second issue is that the existing sub-commands of commit-graph\n> only happen to both have a sensible interpretation of what `--progress`\n> or `--no-progress` means. If we ever added a sub-command which didn't\n> have a notion of progress, we would be forced to ignore the top-level\n> `--[no-]progress` altogether.\n> \n> Since we haven't released a version of Git that supports --[no-]progress\n> as a top-level option for `git commit-graph`, let's remove it.\n\nI agree that is the best way to respond right now. Moving it to\ntop-level will need more work.\n\n> @@ -50,8 +50,6 @@ static struct option common_opts[] = {\n>  \tOPT_STRING(0, \"object-dir\", &opts.obj_dir,\n>  \t\t   N_(\"dir\"),\n>  \t\t   N_(\"the object directory to store the graph\")),\n> -\tOPT_BOOL(0, \"progress\", &opts.progress,\n> -\t\t N_(\"force progress reporting\")),\n>  \tOPT_END()\n>  };\n>  \n> @@ -95,6 +93,8 @@ static int graph_verify(int argc, const char **argv)\n>  \tstatic struct option builtin_commit_graph_verify_options[] = {\n>  \t\tOPT_BOOL(0, \"shallow\", &opts.shallow,\n>  \t\t\t N_(\"if the commit-graph is split, only verify the tip file\")),\n> +\t\tOPT_BOOL(0, \"progress\", &opts.progress,\n> +\t\t\t N_(\"force progress reporting\")),\n>  \t\tOPT_END(),\n>  \t};\n>  \tstruct option *options = add_common_options(builtin_commit_graph_verify_options);\n> @@ -246,6 +246,8 @@ static int graph_write(int argc, const char **argv)\n>  \t\tOPT_CALLBACK_F(0, \"max-new-filters\", &write_opts.max_new_filters,\n>  \t\t\tNULL, N_(\"maximum number of changed-path Bloom filters to compute\"),\n>  \t\t\t0, write_option_max_new_filters),\n> +\t\tOPT_BOOL(0, \"progress\", &opts.progress,\n> +\t\t\t N_(\"force progress reporting\")),\n>  \t\tOPT_END(),\n\nMeanwhile this diff is easy to verify.\n\nThanks,\n-Stolee\n\n"},{"id":"436420","messageId":"YUiiozypuNk1Qx91@nand.local","threadId":"56533","inReplyTo":"088241fc-4070-95ea-b5eb-01c7771ee3af@gmail.com","subject":"Re: [PATCH 1/1] builtin/commit-graph.c: don't accept common --[no-]progress","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-20T15:02:59Z","receivedAt":"2021-09-20T15:03:06Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Sep 20, 2021 at 08:46:32AM -0400, Derrick Stolee wrote:\n> On 9/18/2021 12:02 PM, Taylor Blau wrote:\n> > Since we haven't released a version of Git that supports --[no-]progress\n> > as a top-level option for `git commit-graph`, let's remove it.\n>\n> I agree that is the best way to respond right now. Moving it to\n> top-level will need more work.\n\nSZEDER posted a patch in [1] which would allow us to define a top-level\n`--[no-]progress` option for the commit-graph builtin. (I'm assuming\nthat you meant the builtin when you said \"top-level\", and not git\nitself).\n\nBut see some of his commentary above the patch in [1] about why we may\nwant to avoid applying something like his patch, in particular:\n\n  In general, even when all subcommands of a git command understand a\n  particular --option, that does not mean that it's a good idea to teach\n  that option to that git command.  E.g. what if we later add another\n  subcommand for which that --option doesn't make any sense?  And from\n  the quoted discussion above it seems that teaching 'git commit-graph'\n  the '--progress' option was not intentional at all.\n\nThis patch has the added advantage that we can always \"go back\" to\nSZEDER's approach and make `--[no-]progress` work as an option to `git\ncommit-graph`. But doing this buys us some time to make sure that is the\napproach we want to take.\n\nThanks,\nTaylor\n\n[1]: https://lore.kernel.org/git/20210917211337.GC2118053@szeder.dev/\n"},{"id":"436458","messageId":"xmqqr1dj9c0b.fsf@gitster.g","threadId":"56533","inReplyTo":"cover.1631980949.git.me@ttaylorr.com","subject":"Re: [PATCH 0/1] commit-graph: drop top-level --[no-]progress","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-20T21:24:04Z","receivedAt":"2021-09-20T21:26:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> An open question is whether the same should be done for the multi-pack-index\n> command, whose top-level support for `--[no-]progress` was released in v2.32.0\n> with 60ca94769c (builtin/multi-pack-index.c: split sub-commands, 2021-03-30).\n\nWe do not mind too much about \"breaking backward compatibility\" by\nremoving the mistaken \"git multi-pack-index --progress cmd\", I would\nsay.  It's not like people would type it once every day and removing\nthe \"support\" will break their finger-memory.\n\n"},{"id":"436497","messageId":"YUj/h3xucy4JR7B1@nand.local","threadId":"56533","inReplyTo":"xmqqr1dj9c0b.fsf@gitster.g","subject":"Re: [PATCH 0/1] commit-graph: drop top-level --[no-]progress","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-20T21:39:19Z","receivedAt":"2021-09-21T02:20:06Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Sep 20, 2021 at 02:24:04PM -0700, Junio C Hamano wrote:\n> Taylor Blau <me@ttaylorr.com> writes:\n>\n> > An open question is whether the same should be done for the multi-pack-index\n> > command, whose top-level support for `--[no-]progress` was released in v2.32.0\n> > with 60ca94769c (builtin/multi-pack-index.c: split sub-commands, 2021-03-30).\n>\n> We do not mind too much about \"breaking backward compatibility\" by\n> removing the mistaken \"git multi-pack-index --progress cmd\", I would\n> say.  It's not like people would type it once every day and removing\n> the \"support\" will break their finger-memory.\n\nOK; if we don't mind then we could do something like the following on\ntop. But if we're OK to remove the top-level `--progress` option from\nthe commit-graph and multi-pack-index builtins at any time, then both\npatches become far less urgent.\n\nThanks,\nTaylor\n\n--- >8 ---\n\nSubject: [PATCH] builtin/multi-pack-index.c: disable top-level --[no-]progress\n\nIn a similar spirit as the previous patch, let sub-commands which\nsupport showing or hiding a progress meter handle parsing the\n`--progress` or `--no-progress` option, but do not expose it as an\noption to the top-level `multi-pack-index` builtin.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n Documentation/git-multi-pack-index.txt |  6 ++---\n builtin/multi-pack-index.c             | 31 +++++++++++++++++++++-----\n t/t5319-multi-pack-index.sh            | 12 +++++-----\n 3 files changed, 35 insertions(+), 14 deletions(-)\n\ndiff --git a/Documentation/git-multi-pack-index.txt b/Documentation/git-multi-pack-index.txt\nindex ffd601bc17..5ba4bd5166 100644\n--- a/Documentation/git-multi-pack-index.txt\n+++ b/Documentation/git-multi-pack-index.txt\n@@ -9,8 +9,7 @@ git-multi-pack-index - Write and verify multi-pack-indexes\n SYNOPSIS\n --------\n [verse]\n-'git multi-pack-index' [--object-dir=<dir>] [--[no-]progress]\n-\t[--preferred-pack=<pack>] <subcommand>\n+'git multi-pack-index' [--object-dir=<dir>] <sub-command>\n\n DESCRIPTION\n -----------\n@@ -26,7 +25,8 @@ OPTIONS\n\n --[no-]progress::\n \tTurn progress on/off explicitly. If neither is specified, progress is\n-\tshown if standard error is connected to a terminal.\n+\tshown if standard error is connected to a terminal. Supported by\n+\tsub-commands `write`, `verify`, `expire`, and `repack.\n\n The following subcommands are available:\n\ndiff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c\nindex 649aa5f9ab..5f11a3067d 100644\n--- a/builtin/multi-pack-index.c\n+++ b/builtin/multi-pack-index.c\n@@ -52,7 +52,6 @@ static struct opts_multi_pack_index {\n static struct option common_opts[] = {\n \tOPT_FILENAME(0, \"object-dir\", &opts.object_dir,\n \t  N_(\"object directory containing set of packfile and pack-index pairs\")),\n-\tOPT_BIT(0, \"progress\", &opts.flags, N_(\"force progress reporting\"), MIDX_PROGRESS),\n \tOPT_END(),\n };\n\n@@ -68,6 +67,8 @@ static int cmd_multi_pack_index_write(int argc, const char **argv)\n \t\tOPT_STRING(0, \"preferred-pack\", &opts.preferred_pack,\n \t\t\t   N_(\"preferred-pack\"),\n \t\t\t   N_(\"pack for reuse when computing a multi-pack bitmap\")),\n+\t\tOPT_BIT(0, \"progress\", &opts.flags,\n+\t\t\tN_(\"force progress reporting\"), MIDX_PROGRESS),\n \t\tOPT_END(),\n \t};\n\n@@ -75,6 +76,8 @@ static int cmd_multi_pack_index_write(int argc, const char **argv)\n\n \ttrace2_cmd_mode(argv[0]);\n\n+\tif (isatty(2))\n+\t\topts.flags |= MIDX_PROGRESS;\n \targc = parse_options(argc, argv, NULL,\n \t\t\t     options, builtin_multi_pack_index_write_usage,\n \t\t\t     PARSE_OPT_KEEP_UNKNOWN);\n@@ -90,10 +93,18 @@ static int cmd_multi_pack_index_write(int argc, const char **argv)\n\n static int cmd_multi_pack_index_verify(int argc, const char **argv)\n {\n-\tstruct option *options = common_opts;\n+\tstruct option *options;\n+\tstatic struct option builtin_multi_pack_index_verify_options[] = {\n+\t\tOPT_BIT(0, \"progress\", &opts.flags,\n+\t\t\tN_(\"force progress reporting\"), MIDX_PROGRESS),\n+\t\tOPT_END(),\n+\t};\n+\toptions = add_common_options(builtin_multi_pack_index_verify_options);\n\n \ttrace2_cmd_mode(argv[0]);\n\n+\tif (isatty(2))\n+\t\topts.flags |= MIDX_PROGRESS;\n \targc = parse_options(argc, argv, NULL,\n \t\t\t     options, builtin_multi_pack_index_verify_usage,\n \t\t\t     PARSE_OPT_KEEP_UNKNOWN);\n@@ -106,10 +117,18 @@ static int cmd_multi_pack_index_verify(int argc, const char **argv)\n\n static int cmd_multi_pack_index_expire(int argc, const char **argv)\n {\n-\tstruct option *options = common_opts;\n+\tstruct option *options;\n+\tstatic struct option builtin_multi_pack_index_expire_options[] = {\n+\t\tOPT_BIT(0, \"progress\", &opts.flags,\n+\t\t\tN_(\"force progress reporting\"), MIDX_PROGRESS),\n+\t\tOPT_END(),\n+\t};\n+\toptions = add_common_options(builtin_multi_pack_index_expire_options);\n\n \ttrace2_cmd_mode(argv[0]);\n\n+\tif (isatty(2))\n+\t\topts.flags |= MIDX_PROGRESS;\n \targc = parse_options(argc, argv, NULL,\n \t\t\t     options, builtin_multi_pack_index_expire_usage,\n \t\t\t     PARSE_OPT_KEEP_UNKNOWN);\n@@ -126,6 +145,8 @@ static int cmd_multi_pack_index_repack(int argc, const char **argv)\n \tstatic struct option builtin_multi_pack_index_repack_options[] = {\n \t\tOPT_MAGNITUDE(0, \"batch-size\", &opts.batch_size,\n \t\t  N_(\"during repack, collect pack-files of smaller size into a batch that is larger than this size\")),\n+\t\tOPT_BIT(0, \"progress\", &opts.flags,\n+\t\t  N_(\"force progress reporting\"), MIDX_PROGRESS),\n \t\tOPT_END(),\n \t};\n\n@@ -133,6 +154,8 @@ static int cmd_multi_pack_index_repack(int argc, const char **argv)\n\n \ttrace2_cmd_mode(argv[0]);\n\n+\tif (isatty(2))\n+\t\topts.flags |= MIDX_PROGRESS;\n \targc = parse_options(argc, argv, NULL,\n \t\t\t     options,\n \t\t\t     builtin_multi_pack_index_repack_usage,\n@@ -154,8 +177,6 @@ int cmd_multi_pack_index(int argc, const char **argv,\n\n \tgit_config(git_default_config, NULL);\n\n-\tif (isatty(2))\n-\t\topts.flags |= MIDX_PROGRESS;\n \targc = parse_options(argc, argv, prefix,\n \t\t\t     builtin_multi_pack_index_options,\n \t\t\t     builtin_multi_pack_index_usage,\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 3d4d9f10c3..86b7de2281 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -174,12 +174,12 @@ test_expect_success 'write progress off for redirected stderr' '\n '\n\n test_expect_success 'write force progress on for stderr' '\n-\tGIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir --progress write 2>err &&\n+\tGIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir write --progress 2>err &&\n \ttest_file_not_empty err\n '\n\n test_expect_success 'write with the --no-progress option' '\n-\tGIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir --no-progress write 2>err &&\n+\tGIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir write --no-progress 2>err &&\n \ttest_line_count = 0 err\n '\n\n@@ -429,12 +429,12 @@ test_expect_success 'repack progress off for redirected stderr' '\n '\n\n test_expect_success 'repack force progress on for stderr' '\n-\tGIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir --progress repack 2>err &&\n+\tGIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir repack --progress 2>err &&\n \ttest_file_not_empty err\n '\n\n test_expect_success 'repack with the --no-progress option' '\n-\tGIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir --no-progress repack 2>err &&\n+\tGIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir repack --no-progress 2>err &&\n \ttest_line_count = 0 err\n '\n\n@@ -618,7 +618,7 @@ test_expect_success 'expire progress off for redirected stderr' '\n test_expect_success 'expire force progress on for stderr' '\n \t(\n \t\tcd dup &&\n-\t\tGIT_PROGRESS_DELAY=0 git multi-pack-index --progress expire 2>err &&\n+\t\tGIT_PROGRESS_DELAY=0 git multi-pack-index expire --progress 2>err &&\n \t\ttest_file_not_empty err\n \t)\n '\n@@ -626,7 +626,7 @@ test_expect_success 'expire force progress on for stderr' '\n test_expect_success 'expire with the --no-progress option' '\n \t(\n \t\tcd dup &&\n-\t\tGIT_PROGRESS_DELAY=0 git multi-pack-index --no-progress expire 2>err &&\n+\t\tGIT_PROGRESS_DELAY=0 git multi-pack-index expire --no-progress 2>err &&\n \t\ttest_line_count = 0 err\n \t)\n '\n--\n2.33.0.96.g73915697e6\n\n"},{"id":"436562","messageId":"YUlXpYR6kKiqsXx+@peff.net","threadId":"56533","inReplyTo":"xmqqr1dj9c0b.fsf@gitster.g","subject":"Re: [PATCH 0/1] commit-graph: drop top-level --[no-]progress","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-21T03:55:17Z","receivedAt":"2021-09-21T03:55:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 20, 2021 at 02:24:04PM -0700, Junio C Hamano wrote:\n\n> Taylor Blau <me@ttaylorr.com> writes:\n> \n> > An open question is whether the same should be done for the multi-pack-index\n> > command, whose top-level support for `--[no-]progress` was released in v2.32.0\n> > with 60ca94769c (builtin/multi-pack-index.c: split sub-commands, 2021-03-30).\n> \n> We do not mind too much about \"breaking backward compatibility\" by\n> removing the mistaken \"git multi-pack-index --progress cmd\", I would\n> say.  It's not like people would type it once every day and removing\n> the \"support\" will break their finger-memory.\n\nJust to play devil's advocate: it is possible that somebody scripted it,\nsince it is after all a pretty plumbing-ish command.\n\nI do find it somewhat unlikely, though, given how little time it has\nbeen around, how unlike the rest of Git it is, and how odd it seems for\nscripts to ask for --progress (though maybe --no-progress is more\nlikely?).\n\nSo I'm OK with it, but I think \"finger-memory\" is not the whole story.\n\n-Peff\n"},{"id":"436652","messageId":"87zgs593ja.fsf@evledraar.gmail.com","threadId":"56533","inReplyTo":"YUj/h3xucy4JR7B1@nand.local","subject":"Re: [PATCH 0/1] commit-graph: drop top-level --[no-]progress","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-21T18:19:47Z","receivedAt":"2021-09-21T18:39:26Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Sep 20 2021, Taylor Blau wrote:\n\n> On Mon, Sep 20, 2021 at 02:24:04PM -0700, Junio C Hamano wrote:\n>> Taylor Blau <me@ttaylorr.com> writes:\n>>\n>> > An open question is whether the same should be done for the multi-pack-index\n>> > command, whose top-level support for `--[no-]progress` was released in v2.32.0\n>> > with 60ca94769c (builtin/multi-pack-index.c: split sub-commands, 2021-03-30).\n>>\n>> We do not mind too much about \"breaking backward compatibility\" by\n>> removing the mistaken \"git multi-pack-index --progress cmd\", I would\n>> say.  It's not like people would type it once every day and removing\n>> the \"support\" will break their finger-memory.\n>\n> OK; if we don't mind then we could do something like the following on\n> top. But if we're OK to remove the top-level `--progress` option from\n> the commit-graph and multi-pack-index builtins at any time, then both\n> patches become far less urgent.\n\nI think just taking both this and your commit-graph patches is the right\nthing to do at this point. I.e. we almost entirely take:\n\n    git [git-opts] <cmd> [cmd-opts]\n\nOr:\n\n    git [git-opts] <cmd> <subcmd> [subcmd-opts]\n\nAnd almost never:\n\n    git [git-opts] <cmd> [global-subcmd-opts] <subcmd> [subcmd-opts]\n\nA notable exception is the --object-dir (I think I found out from\nDerrick at some point why that was even needed v.s. the top-level\n--git-dir, but I can't remember).\n\nI think that fixing th commit-graph regression I introduced is an\nobvious thing to do at this point, and likewise with multi-pack-index I\nthink it's young enough that we can just change it and not live with the\nwart forever.\n\nAnd as a general thing, we really should be marking all new built-ins as\nhaving an experimental interface for the first N releases, to catch and\ncorrect these sorts of issues.\n\nBut just as a *general* comment on where our UI should and shouldn't be\nheaded, I find your [1] an entirely unconvincing reply to [2]. I.e.:\n\n    [...] the [...] issue is that the existing sub-commands of\n    commit-graph only happen to both have a sensible interpretation of\n    what `--progress` or `--no-progress` means. If we ever added a\n    sub-command which didn't have a notion of progress, we would be\n    forced to ignore the top-level `--[no-]progress` altogether.\n\nI'd think if anything that's an argument for pursuing the\n[global-subcmd-opts] for these sorts of options, i.e.:\n\n * We're not talking about a --find option or something that's likely to\n   have different meanings.\n\n   If using a [global-subcmd-opts] pattern means that we can't have some\n   command have a --progress that means something entirely\n   different. Then that to me seems like an obvious argument for the\n   opposite point, i.e. that oddity should name its option something\n   else, just as we're spared the confusion of not having --exec-path or\n   whatever behave differently per-command.\n\n * I really don't see how for an option like --progress that we've got\n   en established pattern working the same way across the board, why\n   it's an issue that something accepts a --progress and doesn't do\n   anything with it yet, or ever.\n\n   We don't insist on our config system being configured to the command\n   or subcommand level, I don't see why in terms of implementation or\n   ease of user understanding why it would be desirable to treat this\n   differently.\n\n   I.e. not everything's a --progress or core.pager, but some things\n   are, and having those things be closer to global than not is useful.\n\nAnyway, as I noted let's take both of these patches now. I just wrote\nthis out mainly for my own future reference. I am interesting in\nextending parse_options() and the config system into something that\nallows you to all-at-once define commands and subcommands and have\nwhat's now a bunch of duplicated and subtly different code done for you\nautomatically.\n\nIn that scenario I don't see why in terms of both commands and config\nwhy we wouldn't define things in terms of \"levels\" and always understand\ncertain things at certain levels, and pass them down. E.g. --exec-path\nand --object-format at the first, maybe --progress or\n--output-format=json at the second (between a command and subcommand)\nlevel etc.\n\n1. https://lore.kernel.org/git/e41e65ddf77c596a7926e75bfc15f21c075d0f03.1631980949.git.me@ttaylorr.com/\n2. https://lore.kernel.org/git/87zgsad6mn.fsf@evledraar.gmail.com/\n\n\n> --- >8 ---\n>\n> Subject: [PATCH] builtin/multi-pack-index.c: disable top-level --[no-]progress\n>\n> In a similar spirit as the previous patch, let sub-commands which\n> support showing or hiding a progress meter handle parsing the\n> `--progress` or `--no-progress` option, but do not expose it as an\n> option to the top-level `multi-pack-index` builtin.\n>\n> Signed-off-by: Taylor Blau <me@ttaylorr.com>\n> ---\n>  Documentation/git-multi-pack-index.txt |  6 ++---\n>  builtin/multi-pack-index.c             | 31 +++++++++++++++++++++-----\n>  t/t5319-multi-pack-index.sh            | 12 +++++-----\n>  3 files changed, 35 insertions(+), 14 deletions(-)\n>\n> diff --git a/Documentation/git-multi-pack-index.txt b/Documentation/git-multi-pack-index.txt\n> index ffd601bc17..5ba4bd5166 100644\n> --- a/Documentation/git-multi-pack-index.txt\n> +++ b/Documentation/git-multi-pack-index.txt\n> @@ -9,8 +9,7 @@ git-multi-pack-index - Write and verify multi-pack-indexes\n>  SYNOPSIS\n>  --------\n>  [verse]\n> -'git multi-pack-index' [--object-dir=<dir>] [--[no-]progress]\n> -\t[--preferred-pack=<pack>] <subcommand>\n> +'git multi-pack-index' [--object-dir=<dir>] <sub-command>\n>\n>  DESCRIPTION\n>  -----------\n> @@ -26,7 +25,8 @@ OPTIONS\n>\n>  --[no-]progress::\n>  \tTurn progress on/off explicitly. If neither is specified, progress is\n> -\tshown if standard error is connected to a terminal.\n> +\tshown if standard error is connected to a terminal. Supported by\n> +\tsub-commands `write`, `verify`, `expire`, and `repack.\n>\n>  The following subcommands are available:\n>\n> diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c\n> index 649aa5f9ab..5f11a3067d 100644\n> --- a/builtin/multi-pack-index.c\n> +++ b/builtin/multi-pack-index.c\n> @@ -52,7 +52,6 @@ static struct opts_multi_pack_index {\n>  static struct option common_opts[] = {\n>  \tOPT_FILENAME(0, \"object-dir\", &opts.object_dir,\n>  \t  N_(\"object directory containing set of packfile and pack-index pairs\")),\n> -\tOPT_BIT(0, \"progress\", &opts.flags, N_(\"force progress reporting\"), MIDX_PROGRESS),\n>  \tOPT_END(),\n>  };\n>\n> @@ -68,6 +67,8 @@ static int cmd_multi_pack_index_write(int argc, const char **argv)\n>  \t\tOPT_STRING(0, \"preferred-pack\", &opts.preferred_pack,\n>  \t\t\t   N_(\"preferred-pack\"),\n>  \t\t\t   N_(\"pack for reuse when computing a multi-pack bitmap\")),\n> +\t\tOPT_BIT(0, \"progress\", &opts.flags,\n> +\t\t\tN_(\"force progress reporting\"), MIDX_PROGRESS),\n>  \t\tOPT_END(),\n>  \t};\n>\n> @@ -75,6 +76,8 @@ static int cmd_multi_pack_index_write(int argc, const char **argv)\n>\n>  \ttrace2_cmd_mode(argv[0]);\n>\n> +\tif (isatty(2))\n> +\t\topts.flags |= MIDX_PROGRESS;\n>  \targc = parse_options(argc, argv, NULL,\n>  \t\t\t     options, builtin_multi_pack_index_write_usage,\n>  \t\t\t     PARSE_OPT_KEEP_UNKNOWN);\n> @@ -90,10 +93,18 @@ static int cmd_multi_pack_index_write(int argc, const char **argv)\n>\n>  static int cmd_multi_pack_index_verify(int argc, const char **argv)\n>  {\n> -\tstruct option *options = common_opts;\n> +\tstruct option *options;\n> +\tstatic struct option builtin_multi_pack_index_verify_options[] = {\n> +\t\tOPT_BIT(0, \"progress\", &opts.flags,\n> +\t\t\tN_(\"force progress reporting\"), MIDX_PROGRESS),\n> +\t\tOPT_END(),\n> +\t};\n> +\toptions = add_common_options(builtin_multi_pack_index_verify_options);\n>\n>  \ttrace2_cmd_mode(argv[0]);\n>\n> +\tif (isatty(2))\n> +\t\topts.flags |= MIDX_PROGRESS;\n>  \targc = parse_options(argc, argv, NULL,\n>  \t\t\t     options, builtin_multi_pack_index_verify_usage,\n>  \t\t\t     PARSE_OPT_KEEP_UNKNOWN);\n> @@ -106,10 +117,18 @@ static int cmd_multi_pack_index_verify(int argc, const char **argv)\n>\n>  static int cmd_multi_pack_index_expire(int argc, const char **argv)\n>  {\n> -\tstruct option *options = common_opts;\n> +\tstruct option *options;\n> +\tstatic struct option builtin_multi_pack_index_expire_options[] = {\n> +\t\tOPT_BIT(0, \"progress\", &opts.flags,\n> +\t\t\tN_(\"force progress reporting\"), MIDX_PROGRESS),\n> +\t\tOPT_END(),\n> +\t};\n> +\toptions = add_common_options(builtin_multi_pack_index_expire_options);\n>\n>  \ttrace2_cmd_mode(argv[0]);\n>\n> +\tif (isatty(2))\n> +\t\topts.flags |= MIDX_PROGRESS;\n>  \targc = parse_options(argc, argv, NULL,\n>  \t\t\t     options, builtin_multi_pack_index_expire_usage,\n>  \t\t\t     PARSE_OPT_KEEP_UNKNOWN);\n> @@ -126,6 +145,8 @@ static int cmd_multi_pack_index_repack(int argc, const char **argv)\n>  \tstatic struct option builtin_multi_pack_index_repack_options[] = {\n>  \t\tOPT_MAGNITUDE(0, \"batch-size\", &opts.batch_size,\n>  \t\t  N_(\"during repack, collect pack-files of smaller size into a batch that is larger than this size\")),\n> +\t\tOPT_BIT(0, \"progress\", &opts.flags,\n> +\t\t  N_(\"force progress reporting\"), MIDX_PROGRESS),\n>  \t\tOPT_END(),\n>  \t};\n>\n> @@ -133,6 +154,8 @@ static int cmd_multi_pack_index_repack(int argc, const char **argv)\n>\n>  \ttrace2_cmd_mode(argv[0]);\n>\n> +\tif (isatty(2))\n> +\t\topts.flags |= MIDX_PROGRESS;\n>  \targc = parse_options(argc, argv, NULL,\n>  \t\t\t     options,\n>  \t\t\t     builtin_multi_pack_index_repack_usage,\n> @@ -154,8 +177,6 @@ int cmd_multi_pack_index(int argc, const char **argv,\n>\n>  \tgit_config(git_default_config, NULL);\n>\n> -\tif (isatty(2))\n> -\t\topts.flags |= MIDX_PROGRESS;\n>  \targc = parse_options(argc, argv, prefix,\n>  \t\t\t     builtin_multi_pack_index_options,\n>  \t\t\t     builtin_multi_pack_index_usage,\n> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\n> index 3d4d9f10c3..86b7de2281 100755\n> --- a/t/t5319-multi-pack-index.sh\n> +++ b/t/t5319-multi-pack-index.sh\n> @@ -174,12 +174,12 @@ test_expect_success 'write progress off for redirected stderr' '\n>  '\n>\n>  test_expect_success 'write force progress on for stderr' '\n> -\tGIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir --progress write 2>err &&\n> +\tGIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir write --progress 2>err &&\n>  \ttest_file_not_empty err\n>  '\n>\n>  test_expect_success 'write with the --no-progress option' '\n> -\tGIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir --no-progress write 2>err &&\n> +\tGIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir write --no-progress 2>err &&\n>  \ttest_line_count = 0 err\n>  '\n>\n> @@ -429,12 +429,12 @@ test_expect_success 'repack progress off for redirected stderr' '\n>  '\n>\n>  test_expect_success 'repack force progress on for stderr' '\n> -\tGIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir --progress repack 2>err &&\n> +\tGIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir repack --progress 2>err &&\n>  \ttest_file_not_empty err\n>  '\n>\n>  test_expect_success 'repack with the --no-progress option' '\n> -\tGIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir --no-progress repack 2>err &&\n> +\tGIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir repack --no-progress 2>err &&\n>  \ttest_line_count = 0 err\n>  '\n>\n> @@ -618,7 +618,7 @@ test_expect_success 'expire progress off for redirected stderr' '\n>  test_expect_success 'expire force progress on for stderr' '\n>  \t(\n>  \t\tcd dup &&\n> -\t\tGIT_PROGRESS_DELAY=0 git multi-pack-index --progress expire 2>err &&\n> +\t\tGIT_PROGRESS_DELAY=0 git multi-pack-index expire --progress 2>err &&\n>  \t\ttest_file_not_empty err\n>  \t)\n>  '\n> @@ -626,7 +626,7 @@ test_expect_success 'expire force progress on for stderr' '\n>  test_expect_success 'expire with the --no-progress option' '\n>  \t(\n>  \t\tcd dup &&\n> -\t\tGIT_PROGRESS_DELAY=0 git multi-pack-index --no-progress expire 2>err &&\n> +\t\tGIT_PROGRESS_DELAY=0 git multi-pack-index expire --no-progress 2>err &&\n>  \t\ttest_line_count = 0 err\n>  \t)\n>  '\n\n"},{"id":"436662","messageId":"YUpC0moHj4K53Wk2@nand.local","threadId":"56533","inReplyTo":"87zgs593ja.fsf@evledraar.gmail.com","subject":"Re: [PATCH 0/1] commit-graph: drop top-level --[no-]progress","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-21T20:38:42Z","receivedAt":"2021-09-21T20:38:49Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Sep 21, 2021 at 08:19:47PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n> On Mon, Sep 20 2021, Taylor Blau wrote:\n>\n> > On Mon, Sep 20, 2021 at 02:24:04PM -0700, Junio C Hamano wrote:\n> >> Taylor Blau <me@ttaylorr.com> writes:\n> >>\n> >> > An open question is whether the same should be done for the multi-pack-index\n> >> > command, whose top-level support for `--[no-]progress` was released in v2.32.0\n> >> > with 60ca94769c (builtin/multi-pack-index.c: split sub-commands, 2021-03-30).\n> >>\n> >> We do not mind too much about \"breaking backward compatibility\" by\n> >> removing the mistaken \"git multi-pack-index --progress cmd\", I would\n> >> say.  It's not like people would type it once every day and removing\n> >> the \"support\" will break their finger-memory.\n> >\n> > OK; if we don't mind then we could do something like the following on\n> > top. But if we're OK to remove the top-level `--progress` option from\n> > the commit-graph and multi-pack-index builtins at any time, then both\n> > patches become far less urgent.\n>\n> I think just taking both this and your commit-graph patches is the right\n> thing to do at this point. I.e. we almost entirely take:\n>\n>     git [git-opts] <cmd> [cmd-opts]\n>\n> Or:\n>\n>     git [git-opts] <cmd> <subcmd> [subcmd-opts]\n>\n> And almost never:\n>\n>     git [git-opts] <cmd> [global-subcmd-opts] <subcmd> [subcmd-opts]\n>\n> A notable exception is the --object-dir (I think I found out from\n> Derrick at some point why that was even needed v.s. the top-level\n> --git-dir, but I can't remember).\n\nThere's a good explanation in:\n\n    https://lore.kernel.org/git/22366f81-65a6-55d1-706c-59f877127be0@gmail.com/\n\nand a lot of related discussion happening throughout that whole\nsub-thread. The gist is that it's to be able to treat directories that\nlook like they are a repository's object store (but don't actually\nbelong to any real repository) as if they are an alternate.\n\n> But just as a *general* comment on where our UI should and shouldn't be\n> headed, I find your [1] an entirely unconvincing reply to [2]. I.e.:\n\nI think that's a fine argument in the other direction. But to be fair, I\nconsider the top-level '-c foo.bar=baz' to be different than a\nsub-command of the `commit-graph` builtin supporting `--progress`.\n\nPerhaps you consider these the same, and I could even understand why.\nBut to me, at least, I would be disappointed if we introduced a new\nsub-command of commit-graph which didn't generate a progress meter,\nwhile still accepting `--progress`.\n\nIn other words, as a user, I would be somewhat confused if I didn't\nknow any better to have asked for `--progress` in a mode which no\nprogress will be generated. I imagine it would be confusing not to see\nany output *and* not have `--progress` be rejected as an unrecognized\noption.\n\nAnyway. To be honest, I find this whole discussion a little too\ntheoretical for my taste. I think the patch(es) that I wrote for\ncommit-graph and multi-pack-index seem relatively uncontroversial, and\n(at least in the commit-graph case) fix a real problem that we could\navoid leaking out into a release.\n\nThanks,\nTaylor\n"},{"id":"436731","messageId":"xmqqa6k48tr6.fsf@gitster.g","threadId":"56533","inReplyTo":"YUj/h3xucy4JR7B1@nand.local","subject":"Re: [PATCH 0/1] commit-graph: drop top-level --[no-]progress","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-22T16:22:53Z","receivedAt":"2021-09-22T16:22:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> Subject: [PATCH] builtin/multi-pack-index.c: disable top-level --[no-]progress\n>\n> In a similar spirit as the previous patch, let sub-commands which\n> support showing or hiding a progress meter handle parsing the\n> `--progress` or `--no-progress` option, but do not expose it as an\n> option to the top-level `multi-pack-index` builtin.\n>\n> Signed-off-by: Taylor Blau <me@ttaylorr.com>\n> ---\n\nHmph, so a few older releases accepts --progress at a wrong point on\nthe command line but going forward we will correct it?\n\nOK.  Then I buy the mention of \"as the _previous_ patch\" above ;-)\n\nThanks.\n\n>  Documentation/git-multi-pack-index.txt |  6 ++---\n>  builtin/multi-pack-index.c             | 31 +++++++++++++++++++++-----\n>  t/t5319-multi-pack-index.sh            | 12 +++++-----\n>  3 files changed, 35 insertions(+), 14 deletions(-)\n>\n> diff --git a/Documentation/git-multi-pack-index.txt b/Documentation/git-multi-pack-index.txt\n> index ffd601bc17..5ba4bd5166 100644\n> --- a/Documentation/git-multi-pack-index.txt\n> +++ b/Documentation/git-multi-pack-index.txt\n> @@ -9,8 +9,7 @@ git-multi-pack-index - Write and verify multi-pack-indexes\n>  SYNOPSIS\n>  --------\n>  [verse]\n> -'git multi-pack-index' [--object-dir=<dir>] [--[no-]progress]\n> -\t[--preferred-pack=<pack>] <subcommand>\n> +'git multi-pack-index' [--object-dir=<dir>] <sub-command>\n>\n>  DESCRIPTION\n>  -----------\n> @@ -26,7 +25,8 @@ OPTIONS\n>\n>  --[no-]progress::\n>  \tTurn progress on/off explicitly. If neither is specified, progress is\n> -\tshown if standard error is connected to a terminal.\n> +\tshown if standard error is connected to a terminal. Supported by\n> +\tsub-commands `write`, `verify`, `expire`, and `repack.\n>\n>  The following subcommands are available:\n>\n> diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c\n> index 649aa5f9ab..5f11a3067d 100644\n> --- a/builtin/multi-pack-index.c\n> +++ b/builtin/multi-pack-index.c\n> @@ -52,7 +52,6 @@ static struct opts_multi_pack_index {\n>  static struct option common_opts[] = {\n>  \tOPT_FILENAME(0, \"object-dir\", &opts.object_dir,\n>  \t  N_(\"object directory containing set of packfile and pack-index pairs\")),\n> -\tOPT_BIT(0, \"progress\", &opts.flags, N_(\"force progress reporting\"), MIDX_PROGRESS),\n>  \tOPT_END(),\n>  };\n>\n> @@ -68,6 +67,8 @@ static int cmd_multi_pack_index_write(int argc, const char **argv)\n>  \t\tOPT_STRING(0, \"preferred-pack\", &opts.preferred_pack,\n>  \t\t\t   N_(\"preferred-pack\"),\n>  \t\t\t   N_(\"pack for reuse when computing a multi-pack bitmap\")),\n> +\t\tOPT_BIT(0, \"progress\", &opts.flags,\n> +\t\t\tN_(\"force progress reporting\"), MIDX_PROGRESS),\n>  \t\tOPT_END(),\n>  \t};\n>\n> @@ -75,6 +76,8 @@ static int cmd_multi_pack_index_write(int argc, const char **argv)\n>\n>  \ttrace2_cmd_mode(argv[0]);\n>\n> +\tif (isatty(2))\n> +\t\topts.flags |= MIDX_PROGRESS;\n>  \targc = parse_options(argc, argv, NULL,\n>  \t\t\t     options, builtin_multi_pack_index_write_usage,\n>  \t\t\t     PARSE_OPT_KEEP_UNKNOWN);\n> @@ -90,10 +93,18 @@ static int cmd_multi_pack_index_write(int argc, const char **argv)\n>\n>  static int cmd_multi_pack_index_verify(int argc, const char **argv)\n>  {\n> -\tstruct option *options = common_opts;\n> +\tstruct option *options;\n> +\tstatic struct option builtin_multi_pack_index_verify_options[] = {\n> +\t\tOPT_BIT(0, \"progress\", &opts.flags,\n> +\t\t\tN_(\"force progress reporting\"), MIDX_PROGRESS),\n> +\t\tOPT_END(),\n> +\t};\n> +\toptions = add_common_options(builtin_multi_pack_index_verify_options);\n>\n>  \ttrace2_cmd_mode(argv[0]);\n>\n> +\tif (isatty(2))\n> +\t\topts.flags |= MIDX_PROGRESS;\n>  \targc = parse_options(argc, argv, NULL,\n>  \t\t\t     options, builtin_multi_pack_index_verify_usage,\n>  \t\t\t     PARSE_OPT_KEEP_UNKNOWN);\n> @@ -106,10 +117,18 @@ static int cmd_multi_pack_index_verify(int argc, const char **argv)\n>\n>  static int cmd_multi_pack_index_expire(int argc, const char **argv)\n>  {\n> -\tstruct option *options = common_opts;\n> +\tstruct option *options;\n> +\tstatic struct option builtin_multi_pack_index_expire_options[] = {\n> +\t\tOPT_BIT(0, \"progress\", &opts.flags,\n> +\t\t\tN_(\"force progress reporting\"), MIDX_PROGRESS),\n> +\t\tOPT_END(),\n> +\t};\n> +\toptions = add_common_options(builtin_multi_pack_index_expire_options);\n>\n>  \ttrace2_cmd_mode(argv[0]);\n>\n> +\tif (isatty(2))\n> +\t\topts.flags |= MIDX_PROGRESS;\n>  \targc = parse_options(argc, argv, NULL,\n>  \t\t\t     options, builtin_multi_pack_index_expire_usage,\n>  \t\t\t     PARSE_OPT_KEEP_UNKNOWN);\n> @@ -126,6 +145,8 @@ static int cmd_multi_pack_index_repack(int argc, const char **argv)\n>  \tstatic struct option builtin_multi_pack_index_repack_options[] = {\n>  \t\tOPT_MAGNITUDE(0, \"batch-size\", &opts.batch_size,\n>  \t\t  N_(\"during repack, collect pack-files of smaller size into a batch that is larger than this size\")),\n> +\t\tOPT_BIT(0, \"progress\", &opts.flags,\n> +\t\t  N_(\"force progress reporting\"), MIDX_PROGRESS),\n>  \t\tOPT_END(),\n>  \t};\n>\n> @@ -133,6 +154,8 @@ static int cmd_multi_pack_index_repack(int argc, const char **argv)\n>\n>  \ttrace2_cmd_mode(argv[0]);\n>\n> +\tif (isatty(2))\n> +\t\topts.flags |= MIDX_PROGRESS;\n>  \targc = parse_options(argc, argv, NULL,\n>  \t\t\t     options,\n>  \t\t\t     builtin_multi_pack_index_repack_usage,\n> @@ -154,8 +177,6 @@ int cmd_multi_pack_index(int argc, const char **argv,\n>\n>  \tgit_config(git_default_config, NULL);\n>\n> -\tif (isatty(2))\n> -\t\topts.flags |= MIDX_PROGRESS;\n>  \targc = parse_options(argc, argv, prefix,\n>  \t\t\t     builtin_multi_pack_index_options,\n>  \t\t\t     builtin_multi_pack_index_usage,\n> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\n> index 3d4d9f10c3..86b7de2281 100755\n> --- a/t/t5319-multi-pack-index.sh\n> +++ b/t/t5319-multi-pack-index.sh\n> @@ -174,12 +174,12 @@ test_expect_success 'write progress off for redirected stderr' '\n>  '\n>\n>  test_expect_success 'write force progress on for stderr' '\n> -\tGIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir --progress write 2>err &&\n> +\tGIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir write --progress 2>err &&\n>  \ttest_file_not_empty err\n>  '\n>\n>  test_expect_success 'write with the --no-progress option' '\n> -\tGIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir --no-progress write 2>err &&\n> +\tGIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir write --no-progress 2>err &&\n>  \ttest_line_count = 0 err\n>  '\n>\n> @@ -429,12 +429,12 @@ test_expect_success 'repack progress off for redirected stderr' '\n>  '\n>\n>  test_expect_success 'repack force progress on for stderr' '\n> -\tGIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir --progress repack 2>err &&\n> +\tGIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir repack --progress 2>err &&\n>  \ttest_file_not_empty err\n>  '\n>\n>  test_expect_success 'repack with the --no-progress option' '\n> -\tGIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir --no-progress repack 2>err &&\n> +\tGIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir repack --no-progress 2>err &&\n>  \ttest_line_count = 0 err\n>  '\n>\n> @@ -618,7 +618,7 @@ test_expect_success 'expire progress off for redirected stderr' '\n>  test_expect_success 'expire force progress on for stderr' '\n>  \t(\n>  \t\tcd dup &&\n> -\t\tGIT_PROGRESS_DELAY=0 git multi-pack-index --progress expire 2>err &&\n> +\t\tGIT_PROGRESS_DELAY=0 git multi-pack-index expire --progress 2>err &&\n>  \t\ttest_file_not_empty err\n>  \t)\n>  '\n> @@ -626,7 +626,7 @@ test_expect_success 'expire force progress on for stderr' '\n>  test_expect_success 'expire with the --no-progress option' '\n>  \t(\n>  \t\tcd dup &&\n> -\t\tGIT_PROGRESS_DELAY=0 git multi-pack-index --no-progress expire 2>err &&\n> +\t\tGIT_PROGRESS_DELAY=0 git multi-pack-index expire --no-progress 2>err &&\n>  \t\ttest_line_count = 0 err\n>  \t)\n>  '\n> --\n> 2.33.0.96.g73915697e6\n"}]}