| Message ID | 20210215184118.11306-4-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | commit-graph: parse_options() cleanup | expand |
On Mon, Feb 15, 2021 at 07:41:16PM +0100, Ævar Arnfjörð Bjarmason wrote: > Make use of the parse_options_concat() so we don't need to copy/paste > common options like --object-dir. This is inspired by a similar change > to "checkout" in 2087182272 > (checkout: split options[] array in three pieces, 2019-03-29). > > A minor behavior change here is that now we're going to list both > --object-dir and --progress first, before we'd list --progress along > with other options. "Behavior change" referring only to the output of `git commit-graph -h`, no? Looking at the code (and understanding this whole situation a little bit better), I'd think that this wouldn't cause us to parse anything differently before or after this change, right? > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > builtin/commit-graph.c | 43 ++++++++++++++++++++++++------------------ > 1 file changed, 25 insertions(+), 18 deletions(-) > > diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c > index baead04a03..a7718b2025 100644 > --- a/builtin/commit-graph.c > +++ b/builtin/commit-graph.c > @@ -44,6 +44,21 @@ static struct opts_commit_graph { > int enable_changed_paths; > } opts; > > +static struct option *add_common_options(struct option *prevopts) > +{ > + struct option options[] = { > + 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() > + }; I'm nitpicking, but I wouldn't be sad to see this called "common" instead". Can't this also be declared statically? > + struct option *newopts = parse_options_concat(options, prevopts); > + free(prevopts); > + return newopts; > +} > + > static struct object_directory *find_odb(struct repository *r, > const char *obj_dir) > { > @@ -75,22 +90,20 @@ static int graph_verify(int argc, const char **argv) > int fd; > struct stat st; > int flags = 0; > - > + struct option *options = NULL; > static struct option builtin_commit_graph_verify_options[] = { > - OPT_STRING(0, "object-dir", &opts.obj_dir, > - N_("dir"), > - N_("the object directory to store the graph")), > 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(), > }; > + options = parse_options_dup(builtin_commit_graph_verify_options); Another nitpick, but I'd rather see the initialization of "options" and its declaration be on the same line, after declaring builtin_commit_graph_verify_options. > + options = add_common_options(options); > > trace2_cmd_mode("verify"); > > opts.progress = isatty(2); > argc = parse_options(argc, argv, NULL, > - builtin_commit_graph_verify_options, > + options, > builtin_commit_graph_verify_usage, 0); > > if (!opts.obj_dir) > @@ -205,11 +218,8 @@ static int graph_write(int argc, const char **argv) > int result = 0; > enum commit_graph_write_flags flags = 0; > struct progress *progress = NULL; > - > + struct option *options = NULL; > static struct option builtin_commit_graph_write_options[] = { > - OPT_STRING(0, "object-dir", &opts.obj_dir, > - N_("dir"), > - N_("the object directory to store the graph")), > OPT_BOOL(0, "reachable", &opts.reachable, > N_("start walk at all refs")), > OPT_BOOL(0, "stdin-packs", &opts.stdin_packs, > @@ -220,7 +230,6 @@ static int graph_write(int argc, const char **argv) > N_("include all commits already in the commit-graph file")), > OPT_BOOL(0, "changed-paths", &opts.enable_changed_paths, > N_("enable computation for changed paths")), > - OPT_BOOL(0, "progress", &opts.progress, N_("force progress reporting")), > OPT_CALLBACK_F(0, "split", &write_opts.split_flags, NULL, > N_("allow writing an incremental commit-graph file"), > PARSE_OPT_OPTARG | PARSE_OPT_NONEG, > @@ -236,6 +245,8 @@ static int graph_write(int argc, const char **argv) > 0, write_option_max_new_filters), > OPT_END(), > }; > + options = parse_options_dup(builtin_commit_graph_write_options); > + options = add_common_options(options); > > opts.progress = isatty(2); > opts.enable_changed_paths = -1; > @@ -249,7 +260,7 @@ static int graph_write(int argc, const char **argv) > git_config(git_commit_graph_write_config, &opts); > > argc = parse_options(argc, argv, NULL, > - builtin_commit_graph_write_options, > + options, > builtin_commit_graph_write_usage, 0); > > if (opts.reachable + opts.stdin_packs + opts.stdin_commits > 1) > @@ -312,12 +323,8 @@ static int graph_write(int argc, const char **argv) > > int cmd_commit_graph(int argc, const char **argv, const char *prefix) > { > - static struct option builtin_commit_graph_options[] = { > - OPT_STRING(0, "object-dir", &opts.obj_dir, > - N_("dir"), > - N_("the object directory to store the graph")), > - OPT_END(), > - }; > + struct option *no_options = parse_options_dup(NULL); Hmm. Why bother calling add_common_options at all here? Thanks, Taylor
On Mon, Feb 15, 2021 at 01:51:35PM -0500, Taylor Blau wrote: > Another nitpick, but I'd rather see the initialization of "options" and > its declaration be on the same line, after declaring > builtin_commit_graph_verify_options. Ignore me; the NULL initialization is important. Thanks, Taylor
On Mon, Feb 15 2021, Taylor Blau wrote: > On Mon, Feb 15, 2021 at 07:41:16PM +0100, Ævar Arnfjörð Bjarmason wrote: >> Make use of the parse_options_concat() so we don't need to copy/paste >> common options like --object-dir. This is inspired by a similar change >> to "checkout" in 2087182272 >> (checkout: split options[] array in three pieces, 2019-03-29). >> >> A minor behavior change here is that now we're going to list both >> --object-dir and --progress first, before we'd list --progress along >> with other options. > > "Behavior change" referring only to the output of `git commit-graph -h`, > no? > > Looking at the code (and understanding this whole situation a little bit > better), I'd think that this wouldn't cause us to parse anything > differently before or after this change, right? Indeed, I just mean the "-h" or "--invalid-opt" output changed in the order we show the options in. >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> >> --- >> builtin/commit-graph.c | 43 ++++++++++++++++++++++++------------------ >> 1 file changed, 25 insertions(+), 18 deletions(-) >> >> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c >> index baead04a03..a7718b2025 100644 >> --- a/builtin/commit-graph.c >> +++ b/builtin/commit-graph.c >> @@ -44,6 +44,21 @@ static struct opts_commit_graph { >> int enable_changed_paths; >> } opts; >> >> +static struct option *add_common_options(struct option *prevopts) >> +{ >> + struct option options[] = { >> + 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() >> + }; > > I'm nitpicking, but I wouldn't be sad to see this called "common" > instead". > > Can't this also be declared statically? It happens to work now to do that, but try it in builtin/checkout.c and you'll see it blows up with a wall of "initializer element is not constant". Probably better to be consistent in parse_options() usage than make it safe for that sort of use... >> + struct option *newopts = parse_options_concat(options, prevopts); >> + free(prevopts); >> + return newopts; >> +} >> + >> static struct object_directory *find_odb(struct repository *r, >> const char *obj_dir) >> { >> @@ -75,22 +90,20 @@ static int graph_verify(int argc, const char **argv) >> int fd; >> struct stat st; >> int flags = 0; >> - >> + struct option *options = NULL; >> static struct option builtin_commit_graph_verify_options[] = { >> - OPT_STRING(0, "object-dir", &opts.obj_dir, >> - N_("dir"), >> - N_("the object directory to store the graph")), >> 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(), >> }; >> + options = parse_options_dup(builtin_commit_graph_verify_options); > > Another nitpick, but I'd rather see the initialization of "options" and > its declaration be on the same line, after declaring > builtin_commit_graph_verify_options. As you noted in your own reply "the NULL initialization is important", or more specifically: We're doing this dance here (and in other existing code, e.g. checkout.c) to trampoline from the stack ot the heap. >> + options = add_common_options(options); >> >> trace2_cmd_mode("verify"); >> >> opts.progress = isatty(2); >> argc = parse_options(argc, argv, NULL, >> - builtin_commit_graph_verify_options, >> + options, >> builtin_commit_graph_verify_usage, 0); >> >> if (!opts.obj_dir) >> @@ -205,11 +218,8 @@ static int graph_write(int argc, const char **argv) >> int result = 0; >> enum commit_graph_write_flags flags = 0; >> struct progress *progress = NULL; >> - >> + struct option *options = NULL; >> static struct option builtin_commit_graph_write_options[] = { >> - OPT_STRING(0, "object-dir", &opts.obj_dir, >> - N_("dir"), >> - N_("the object directory to store the graph")), >> OPT_BOOL(0, "reachable", &opts.reachable, >> N_("start walk at all refs")), >> OPT_BOOL(0, "stdin-packs", &opts.stdin_packs, >> @@ -220,7 +230,6 @@ static int graph_write(int argc, const char **argv) >> N_("include all commits already in the commit-graph file")), >> OPT_BOOL(0, "changed-paths", &opts.enable_changed_paths, >> N_("enable computation for changed paths")), >> - OPT_BOOL(0, "progress", &opts.progress, N_("force progress reporting")), >> OPT_CALLBACK_F(0, "split", &write_opts.split_flags, NULL, >> N_("allow writing an incremental commit-graph file"), >> PARSE_OPT_OPTARG | PARSE_OPT_NONEG, >> @@ -236,6 +245,8 @@ static int graph_write(int argc, const char **argv) >> 0, write_option_max_new_filters), >> OPT_END(), >> }; >> + options = parse_options_dup(builtin_commit_graph_write_options); >> + options = add_common_options(options); >> >> opts.progress = isatty(2); >> opts.enable_changed_paths = -1; >> @@ -249,7 +260,7 @@ static int graph_write(int argc, const char **argv) >> git_config(git_commit_graph_write_config, &opts); >> >> argc = parse_options(argc, argv, NULL, >> - builtin_commit_graph_write_options, >> + options, >> builtin_commit_graph_write_usage, 0); >> >> if (opts.reachable + opts.stdin_packs + opts.stdin_commits > 1) >> @@ -312,12 +323,8 @@ static int graph_write(int argc, const char **argv) >> >> int cmd_commit_graph(int argc, const char **argv, const char *prefix) >> { >> - static struct option builtin_commit_graph_options[] = { >> - OPT_STRING(0, "object-dir", &opts.obj_dir, >> - N_("dir"), >> - N_("the object directory to store the graph")), >> - OPT_END(), >> - }; >> + struct option *no_options = parse_options_dup(NULL); > > Hmm. Why bother calling add_common_options at all here? I assume you mean in this line just below what you quoted: struct option *builtin_commit_graph_options = add_common_options(no_options); Do you mean why not do the whole thing in graph_{verify,write}() and only show the usage if we fail here? Yeah arguably that makes more sense, but I wanted to just focus on refactoring existing behavior & get rid of the copy/pasted options rather than start a bigger rewrite of "maybe we shouldn't show this rather useless help info if we die here....".
On Mon, Feb 15, 2021 at 09:39:11PM +0100, Ævar Arnfjörð Bjarmason wrote: > > On Mon, Feb 15 2021, Taylor Blau wrote: > > > On Mon, Feb 15, 2021 at 07:41:16PM +0100, Ævar Arnfjörð Bjarmason wrote: > >> Make use of the parse_options_concat() so we don't need to copy/paste > >> common options like --object-dir. This is inspired by a similar change > >> to "checkout" in 2087182272 > >> (checkout: split options[] array in three pieces, 2019-03-29). > >> > >> A minor behavior change here is that now we're going to list both > >> --object-dir and --progress first, before we'd list --progress along > >> with other options. The final version of this patch that was picked up is at https://public-inbox.org/git/patch-v4-3.7-32cc0d1c7bc-20210823T122854Z-avarab@gmail.com/ I reply to this old version because of the following pieces of the discussion: > > "Behavior change" referring only to the output of `git commit-graph -h`, > > no? > > > > Looking at the code (and understanding this whole situation a little bit > > better), I'd think that this wouldn't cause us to parse anything > > differently before or after this change, right? > > Indeed, I just mean the "-h" or "--invalid-opt" output changed in the > order we show the options in. [...] > but I wanted to just focus on > refactoring existing behavior & get rid of the copy/pasted options No, there is more behavior change: since 84e4484f12 (commit-graph: use parse_options_concat(), 2021-08-23) the 'git commit-graph' command does accept the '--[no-]progress' options as well, but before that only its subcommands did, and 'git commit-graph --progress ...' errored out with "unknown option". Worse, sometimes 'git commit-graph --progress ...' doesn't work as it's supposed to. The patch below descibes the problem and fixes it, but on second thought I don't think that it is the right approach. 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. I'm inclined to think that '--progress' should rather be removed from the common 'git commit-graph' options; luckily it's not too late, because it hasn't been released yet. --- >8 --- Subject: [PATCH] commit-graph: fix 'git commit-graph --[no-]progress ...' Until recenly 'git commit-graph' didn't have a '--progress' option, only its subcommands did, but this changed with 84e4484f12 (commit-graph: use parse_options_concat(), 2021-08-23), and now the 'git commit-graph' command accepts the '--[no-]progress' options as well. Alas, they don't always works as they are supposed to, because the isatty(2) check is only performed in the subcommands, i.e. after the "main" 'git commit-graph' command has parsed its options, and it unconditionally overwrites whatever '--[no-]progress' option might have been given: $ GIT_PROGRESS_DELAY=0 git commit-graph --no-progress write --reachable Collecting referenced commits: 1617, done. Loading known commits in commit graph: 100% (1617/1617), done. [...] $ GIT_PROGRESS_DELAY=0 git commit-graph --progress write 2>out $ wc -c out 0 out Move the isatty(2) check to cmd_commit_graph(), before it calls parse_options(), so 'git commit-graph --[no-]progress' will be able to override it as well. Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> --- builtin/commit-graph.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c index 21fc6e934b..3a873ceaf6 100644 --- a/builtin/commit-graph.c +++ b/builtin/commit-graph.c @@ -101,7 +101,6 @@ static int graph_verify(int argc, const char **argv) trace2_cmd_mode("verify"); - opts.progress = isatty(2); argc = parse_options(argc, argv, NULL, options, builtin_commit_graph_verify_usage, 0); @@ -250,7 +249,6 @@ static int graph_write(int argc, const char **argv) }; struct option *options = add_common_options(builtin_commit_graph_write_options); - opts.progress = isatty(2); opts.enable_changed_paths = -1; write_opts.size_multiple = 2; write_opts.max_commits = 0; @@ -331,6 +329,7 @@ int cmd_commit_graph(int argc, const char **argv, const char *prefix) struct option *builtin_commit_graph_options = common_opts; git_config(git_default_config, NULL); + opts.progress = isatty(2); argc = parse_options(argc, argv, prefix, builtin_commit_graph_options, builtin_commit_graph_usage,
On Fri, Sep 17, 2021 at 11:13:37PM +0200, SZEDER Gábor wrote: > Worse, sometimes 'git commit-graph --progress ...' doesn't work as > it's supposed to. The patch below descibes the problem and fixes it, > but on second thought I don't think that it is the right approach. > > 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. > > I'm inclined to think that '--progress' should rather be removed from > the common 'git commit-graph' options; luckily it's not too late, > because it hasn't been released yet. I wasn't following this series closely, but having seen your fix below, I'm inclined to agree with you. Just because we _can_ allow options before or after sub-commands does not necessarily make it a good idea. There is a distinct meaning to options before/after the command for the base "git" command (e.g., "git -C foo branch" versus "git branch -C foo"), and I think that has been useful overall. > --- >8 --- > > Subject: [PATCH] commit-graph: fix 'git commit-graph --[no-]progress ...' This patch looks like a sensible fix if we don't simply remove the "git commit-graph --progress write" version. -Peff
On Fri, Sep 17 2021, SZEDER Gábor wrote: > On Mon, Feb 15, 2021 at 09:39:11PM +0100, Ævar Arnfjörð Bjarmason wrote: >> >> On Mon, Feb 15 2021, Taylor Blau wrote: >> >> > On Mon, Feb 15, 2021 at 07:41:16PM +0100, Ævar Arnfjörð Bjarmason wrote: >> >> Make use of the parse_options_concat() so we don't need to copy/paste >> >> common options like --object-dir. This is inspired by a similar change >> >> to "checkout" in 2087182272 >> >> (checkout: split options[] array in three pieces, 2019-03-29). >> >> >> >> A minor behavior change here is that now we're going to list both >> >> --object-dir and --progress first, before we'd list --progress along >> >> with other options. > > The final version of this patch that was picked up is at > > https://public-inbox.org/git/patch-v4-3.7-32cc0d1c7bc-20210823T122854Z-avarab@gmail.com/ > > I reply to this old version because of the following pieces of the > discussion: > >> > "Behavior change" referring only to the output of `git commit-graph -h`, >> > no? >> > >> > Looking at the code (and understanding this whole situation a little bit >> > better), I'd think that this wouldn't cause us to parse anything >> > differently before or after this change, right? >> >> Indeed, I just mean the "-h" or "--invalid-opt" output changed in the >> order we show the options in. > > [...] > >> but I wanted to just focus on >> refactoring existing behavior & get rid of the copy/pasted options > > No, there is more behavior change: since 84e4484f12 (commit-graph: use > parse_options_concat(), 2021-08-23) the 'git commit-graph' command > does accept the '--[no-]progress' options as well, but before that > only its subcommands did, and 'git commit-graph --progress ...' > errored out with "unknown option". > > Worse, sometimes 'git commit-graph --progress ...' doesn't work as > it's supposed to. The patch below descibes the problem and fixes it, > but on second thought I don't think that it is the right approach. > > 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. > > I'm inclined to think that '--progress' should rather be removed from > the common 'git commit-graph' options; luckily it's not too late, > because it hasn't been released yet. > > > --- >8 --- > > Subject: [PATCH] commit-graph: fix 'git commit-graph --[no-]progress ...' > > Until recenly 'git commit-graph' didn't have a '--progress' option, > only its subcommands did, but this changed with 84e4484f12 > (commit-graph: use parse_options_concat(), 2021-08-23), and now the > 'git commit-graph' command accepts the '--[no-]progress' options as > well. Alas, they don't always works as they are supposed to, because > the isatty(2) check is only performed in the subcommands, i.e. after > the "main" 'git commit-graph' command has parsed its options, and it > unconditionally overwrites whatever '--[no-]progress' option might > have been given: > > $ GIT_PROGRESS_DELAY=0 git commit-graph --no-progress write --reachable > Collecting referenced commits: 1617, done. > Loading known commits in commit graph: 100% (1617/1617), done. > [...] > $ GIT_PROGRESS_DELAY=0 git commit-graph --progress write 2>out > $ wc -c out > 0 out > > Move the isatty(2) check to cmd_commit_graph(), before it calls > parse_options(), so 'git commit-graph --[no-]progress' will be able to > override it as well. > > Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> > --- > builtin/commit-graph.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c > index 21fc6e934b..3a873ceaf6 100644 > --- a/builtin/commit-graph.c > +++ b/builtin/commit-graph.c > @@ -101,7 +101,6 @@ static int graph_verify(int argc, const char **argv) > > trace2_cmd_mode("verify"); > > - opts.progress = isatty(2); > argc = parse_options(argc, argv, NULL, > options, > builtin_commit_graph_verify_usage, 0); > @@ -250,7 +249,6 @@ static int graph_write(int argc, const char **argv) > }; > struct option *options = add_common_options(builtin_commit_graph_write_options); > > - opts.progress = isatty(2); > opts.enable_changed_paths = -1; > write_opts.size_multiple = 2; > write_opts.max_commits = 0; > @@ -331,6 +329,7 @@ int cmd_commit_graph(int argc, const char **argv, const char *prefix) > struct option *builtin_commit_graph_options = common_opts; > > git_config(git_default_config, NULL); > + opts.progress = isatty(2); > argc = parse_options(argc, argv, prefix, > builtin_commit_graph_options, > builtin_commit_graph_usage, Yes, this was unintentional on my part, sorry, and thanks for cleaning up my mess. However, I have wondered how we should be dealing with these sub-commands in general. In the case of commit-graph we've always documented it at the top-level as OPTIONS, so even though the usage shows: git commit-graph write <options> We've always accepted "--object-dir" after "git commit-graph", and all the other options are documented in their per-subcommand sections. So just from reading the documentation you might think that this (with your fix here) is intentional behavior, and we should just fix the synopsis. Then we have the more recent multi-pack-index which *is* documented as: 'git multi-pack-index' [--object-dir=<dir>] [--[no-]progress] [--preferred-pack=<pack>] <subcommand> So actually, the reason this crept in is probably because I was copying the pattern we've had there since 60ca94769ce (builtin/multi-pack-index.c: split sub-commands, 2021-03-30), my commit message says as much. Given that and multi-pack-index's documented behavior I think that it probably makes sense to keep and document this, and as a follow-up (which I or Taylor could do) change the synopsis accordingly. Aside from whatever bugs have crept or existing behavior, I think it makes sense as UI to do things like: git commit-graph --object-dir=<dir> write --reachable git commit-graph --progress write git commit-graph --progress verify etc., as --progress is a not-subcommand-specific option, not really. We might have a subcommand that doesn't have progress output, but I still think it makes sense to have it in that position, maybe we'll end up adding it later. Brian and I also had a discussion back in April[1] about --object-format, i.e. should we be making every single command support: git hash-object --object-format=sha256 Or (as I suggested) doesn't it make more sense to do: git --object-format=sha256 hash-object Like the --progress option it does mean that you'll end up with commands for whom that'll just be ignored: git --object-format=sha256 version But that's conceptually similar to repo settings, and I don't think it's confusing, the same can be said about e.g.: git -c this.doesNotUse=thisConfig version Having said that for --progress it probably makes sense to eventually have: git --progress commit-graph write I.e. maybe we'd want a top-level option for it, given how many commands have that option and us needing to pass a "do_progress" flag all over the place. Of course we'd need to (silently or not) support it also as: git commit-graph --progress write git commit-graph write --progress Which is the case here. 1. https://lore.kernel.org/git/8735vq2l8a.fsf@evledraar.gmail.com/
On Fri, Sep 17, 2021 at 06:03:58PM -0400, Jeff King wrote: > > I'm inclined to think that '--progress' should rather be removed from > > the common 'git commit-graph' options; luckily it's not too late, > > because it hasn't been released yet. > > I wasn't following this series closely, but having seen your fix below, > I'm inclined to agree with you. Just because we _can_ allow options > before or after sub-commands does not necessarily make it a good idea. > I agree. Suppose we had a "git commit-graph remove" sub-command that removed the commit-graph file (ignoring that there are probably better hypothetical examples than this ;)). It's not obvious what --progress means in the context of that mode. Here's a patch that does what you and Gábor are suggesting as an alternative. Unfortunately, we can't do the same for the multi-pack-index command, since the analogous change there is 60ca94769c (builtin/multi-pack-index.c: split sub-commands, 2021-03-30), which was released in 2.32. Anyway, as promised: --- 8< --- Subject: [PATCH] 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. 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
On Sat, Sep 18 2021, Taylor Blau wrote: > On Fri, Sep 17, 2021 at 06:03:58PM -0400, Jeff King wrote: >> > I'm inclined to think that '--progress' should rather be removed from >> > the common 'git commit-graph' options; luckily it's not too late, >> > because it hasn't been released yet. >> >> I wasn't following this series closely, but having seen your fix below, >> I'm inclined to agree with you. Just because we _can_ allow options >> before or after sub-commands does not necessarily make it a good idea. >> > I agree. Suppose we had a "git commit-graph remove" sub-command that > removed the commit-graph file (ignoring that there are probably better > hypothetical examples than this ;)). It's not obvious what --progress > means in the context of that mode. Well, you might have as many as tens of commit-graph files, and could be running this on AIX where I/O is apparently implemented in terms of carrier pigeon messaging judging by how slow it is :) But as argued in https://lore.kernel.org/git/87zgsad6mn.fsf@evledraar.gmail.com/ I don't see how it's going to be any more confusing to user than "git -c foo=bar version" (the -c does nothing there)> > Here's a patch that does what you and Gábor are suggesting as an > alternative. Unfortunately, we can't do the same for the > multi-pack-index command, since the analogous change there is 60ca94769c > (builtin/multi-pack-index.c: split sub-commands, 2021-03-30), which was > released in 2.32. If we came up with some call about what we want subcommands in general to look like I'd think it would be fine to convert multi-pack-index to it, perhaps with some deprecation period where it would issue a warning() while it understood both forms. > Anyway, as promised: > > --- 8< --- > > Subject: [PATCH] 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. > > 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); This is a good change, but if you're up for bonus points leaves the docs in an odd where we (as noted in [1]) document the --object-dir and --progress options under OPTIONS, but now only take the former before the sub-command. 1. https://lore.kernel.org/git/87zgsad6mn.fsf@evledraar.gmail.com/
On Sat, Sep 18, 2021 at 09:20:38AM +0200, Ævar Arnfjörð Bjarmason wrote: > If we came up with some call about what we want subcommands in general > to look like I'd think it would be fine to convert multi-pack-index to > it, perhaps with some deprecation period where it would issue a > warning() while it understood both forms. I'm not sure about what to do with the multi-pack-index command. Probably going through a deprecation process makes the most sense if we do want to get rid of the top-level `--[no-]progress` there, too. But let's have the discussion elsewhere and not buried in the MIDX bitmaps thread ;). > This is a good change, but if you're up for bonus points leaves the docs > in an odd where we (as noted in [1]) document the --object-dir and > --progress options under OPTIONS, but now only take the former before > the sub-command. Thanks for noticing. I got up and did something in between writing and sending this patch, and had a nagging feeling of forgetting something before I sent. But I couldn't figure out what ;). I'll resubmit this patch (with the doc changes squashed in) as a new thread on the list so we can have the discussion not buried in another thread. Thanks, Taylor
On Sat, Sep 18, 2021 at 11:56:16AM -0400, Taylor Blau wrote: > > This is a good change, but if you're up for bonus points leaves the docs > > in an odd where we (as noted in [1]) document the --object-dir and > > --progress options under OPTIONS, but now only take the former before > > the sub-command. > > Thanks for noticing. I got up and did something in between writing and > sending this patch, and had a nagging feeling of forgetting something > before I sent. But I couldn't figure out what ;). Actually, I stand by the original patch. Yes, the top-level OPTIONS of git-commit-graph(1) mentions `--[no-]progress`, but the synopsis makes clear that those are only accepted after the sub-commands. So I think it's fine as-is. Thanks, Taylor
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c index baead04a03..a7718b2025 100644 --- a/builtin/commit-graph.c +++ b/builtin/commit-graph.c @@ -44,6 +44,21 @@ static struct opts_commit_graph { int enable_changed_paths; } opts; +static struct option *add_common_options(struct option *prevopts) +{ + struct option options[] = { + 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() + }; + struct option *newopts = parse_options_concat(options, prevopts); + free(prevopts); + return newopts; +} + static struct object_directory *find_odb(struct repository *r, const char *obj_dir) { @@ -75,22 +90,20 @@ static int graph_verify(int argc, const char **argv) int fd; struct stat st; int flags = 0; - + struct option *options = NULL; static struct option builtin_commit_graph_verify_options[] = { - OPT_STRING(0, "object-dir", &opts.obj_dir, - N_("dir"), - N_("the object directory to store the graph")), 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(), }; + options = parse_options_dup(builtin_commit_graph_verify_options); + options = add_common_options(options); trace2_cmd_mode("verify"); opts.progress = isatty(2); argc = parse_options(argc, argv, NULL, - builtin_commit_graph_verify_options, + options, builtin_commit_graph_verify_usage, 0); if (!opts.obj_dir) @@ -205,11 +218,8 @@ static int graph_write(int argc, const char **argv) int result = 0; enum commit_graph_write_flags flags = 0; struct progress *progress = NULL; - + struct option *options = NULL; static struct option builtin_commit_graph_write_options[] = { - OPT_STRING(0, "object-dir", &opts.obj_dir, - N_("dir"), - N_("the object directory to store the graph")), OPT_BOOL(0, "reachable", &opts.reachable, N_("start walk at all refs")), OPT_BOOL(0, "stdin-packs", &opts.stdin_packs, @@ -220,7 +230,6 @@ static int graph_write(int argc, const char **argv) N_("include all commits already in the commit-graph file")), OPT_BOOL(0, "changed-paths", &opts.enable_changed_paths, N_("enable computation for changed paths")), - OPT_BOOL(0, "progress", &opts.progress, N_("force progress reporting")), OPT_CALLBACK_F(0, "split", &write_opts.split_flags, NULL, N_("allow writing an incremental commit-graph file"), PARSE_OPT_OPTARG | PARSE_OPT_NONEG, @@ -236,6 +245,8 @@ static int graph_write(int argc, const char **argv) 0, write_option_max_new_filters), OPT_END(), }; + options = parse_options_dup(builtin_commit_graph_write_options); + options = add_common_options(options); opts.progress = isatty(2); opts.enable_changed_paths = -1; @@ -249,7 +260,7 @@ static int graph_write(int argc, const char **argv) git_config(git_commit_graph_write_config, &opts); argc = parse_options(argc, argv, NULL, - builtin_commit_graph_write_options, + options, builtin_commit_graph_write_usage, 0); if (opts.reachable + opts.stdin_packs + opts.stdin_commits > 1) @@ -312,12 +323,8 @@ static int graph_write(int argc, const char **argv) int cmd_commit_graph(int argc, const char **argv, const char *prefix) { - static struct option builtin_commit_graph_options[] = { - OPT_STRING(0, "object-dir", &opts.obj_dir, - N_("dir"), - N_("the object directory to store the graph")), - OPT_END(), - }; + struct option *no_options = parse_options_dup(NULL); + struct option *builtin_commit_graph_options = add_common_options(no_options); git_config(git_default_config, NULL); argc = parse_options(argc, argv, prefix,
Make use of the parse_options_concat() so we don't need to copy/paste common options like --object-dir. This is inspired by a similar change to "checkout" in 2087182272 (checkout: split options[] array in three pieces, 2019-03-29). A minor behavior change here is that now we're going to list both --object-dir and --progress first, before we'd list --progress along with other options. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- builtin/commit-graph.c | 43 ++++++++++++++++++++++++------------------ 1 file changed, 25 insertions(+), 18 deletions(-)