Re: [RFC PATCH] rev-list: clarify --abbrev and --abbrev-commit usage

From: Jeff King <peff@peff.net>
Date: Jun 19, 2019, 21:21 UTC
Message-ID: <20190619212159.GA6571@sigill.intra.peff.net>
In-Reply-To: <20190614225654.GD233791@google.com>

On Fri, Jun 14, 2019 at 03:56:54PM -0700, Emily Shaffer wrote:

Show 14 quoted lines

> > Ah, I see. I don't consider "|" to indicate an exclusion to the point
> > that the options are rejected. Only that you wouldn't want to use both,
> > because one counteracts the other. So every "--no-foo" is mutually
> > exclusive with "--foo" in the sense that one override the other. But the
> > outcome is "last one wins", and not "whoops, we cannot figure out what
> > you meant". And that's what the original:
> > 
> >       --abbrev=<n> | --no-abbrev
> > 
> > before your patch was trying to say (and I suspect there are many other
> > cases of "|" with this kind of last-one-wins behavior).
> 
> For what it's worth, in this case it's not last-one-wins - --no-abbrev
> always wins:

Ah, thanks for pointing that; I hadn't noticed. That _is_ unlike most of the rest of Git. I'm tempted to say it's a bug and should be fixed, but I worry slightly that it could have an unexpected effect.

Show 5 quoted lines

> I think a good solution here is to go and add --abbrev-commit=<n>
> without breaking support for --abbrev=<n>; I'm a little more worried
> about changing --no-abbrev to last-one-wins but I'll take a crack at it
> and see what the test suite says. While I'm at it, I'll check for
> last-one-wins with multiple instances of --abbrev[-commit]=<n>.

I think --abbrev-commit=<n> sounds safe enough, though I worry it may get a bit complicated because we'd presumably want to fall back to the <n> from --abbrev=<n>. I'll see how your patch turns out. :)

I like the idea of changing --no-abbrev to last-one-wins, as above, but the test suite may not give us that much confidence. These kinds of cases are often not well-covered, and we're really worried about the wider world of random scripts people have grown over the last 10 years. Of course if the test suite does break horribly that might give us extra caution, but I'm not sure "the test suite does not break" gives us much confidence.

> Having done so, I'll also change the documentation here in rev-list to:
>  --abbrev-commit[=<n>] [--abbrev=<n>] | --no-abbrev

Yeah, that makes sense.

-Peff