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

From: Jeff King <peff@peff.net>
Date: Sep 17, 2021, 22:03 UTC
Message-ID: <YUUQzswYL5x74Tps@coredump.intra.peff.net>
In-Reply-To: <20210917211337.GC2118053@szeder.dev>

On Fri, Sep 17, 2021 at 11:13:37PM +0200, SZEDER Gábor wrote:

Show 14 quoted lines

> 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