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@xxxxxxxxx/ > > 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@xxxxxxxxx> > --- > 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@xxxxxxxxxxxxxxxxxxx/
