Re: [PATCH 1/3] builtin/commit-graph.c: support '--split[=<strategy>]'

From: Martin Ågren <martin.agren@gmail.com>
Date: Feb 6, 2020, 19:15 UTC
Message-ID: <CAN0heSpg6i7m9tT6LM-8C+cOMGOtDYMdpsuGn-weUo23jgJOTQ@mail.gmail.com>
In-Reply-To: <20200204040628.GE5790@syl.local>

On Tue, 4 Feb 2020 at 05:06, Taylor Blau <me@ttaylorr.com> wrote:

Show 24 quoted lines

>
> > Or can you convince me otherwise? From which angle should I look at
> > this?
>
> Heh. This is all very reminiscent of an off-list discussion that I had
> with Peff and Stolee before sending this upstream. Originally, I had
> implemented this as:
>
>   $ git commit-graph write --split --[no-]merge
>
> but we decided that this '--merge' and '--no-merge' requiring '--split'
> seemed to indicate that this was better off as an argument to '--split'.
> Of course, there's no getting around that it is... odd to say
> '--split=no-merge' for exactly the reason you suggest.
>
> Here's another way of looking at it: the presence of '--split' means
> "work with split graph files" and the '[no-]merge' argument means:
> "always/never condense multiple layers".
>
> For me, this not only makes the new option language jive, but makes it
> clearer to me than the combination of '--split', '--split --no-merge'
> and '--split --merge', where the third one is truly bizarre. At least
> condensing the second '--' and making 'merge' an argument to 'split'
> makes it clear that the two work together somehow.

Yes, "--split --merge" sounds no better. :-)

> If you have a different suggestion, I'd certainly love to hear about it
> and discuss. But, at least as far as our internal discussions have gone,
> this is by far the best option that we have been able to come up with.

I can't come up with anything better, so please feel free to carry on (as you already have).

Show 10 quoted lines

> > > -               OPT_BOOL(0, "split", &opts.split,
> > > -                       N_("allow writing an incremental commit-graph file")),
> > > +               OPT_CALLBACK_F(0, "split", &split_opts.flags, NULL,
> > > +                       N_("allow writing an incremental commit-graph file"),
> >
> > This still sounds very boolean. Cramming in the "strategy" might be hard
> > -- is this an argument in favor of having two separate options? ;-)
>
> Heh. Exactly how we started these patches when I originally wrote
> them...

You left this as-is in v2. I don't have any immediate improvements to offer. I could see shortening the original to "use the 'split' file format", in which case maybe one could allude to a strategy here. (I don't think "allow" is really needed, right? Maybe it tries to cover for the situation where there's no commit graph yet, so you might say we wouldn't write an "incremental" one, but the format would still be the same, AFAIU. Anyway, that's outside the scope of this patch.)

Martin