Re: [PATCH 3/5] commit-graph: use parse_options_concat()

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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/