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

[Emily Shaffer <emilyshaffer@google.com>]

On Fri, Jun 14, 2019 at 12:18:41PM -0400, Jeff King wrote:
> On Thu, Jun 13, 2019 at 03:15:41PM -0700, Emily Shaffer wrote:
> 
> > I thought this was odd when I was working on the other rev-list changes
> > - --abbrev doesn't do anything on its own. It looks like it does work by
> > itself in other commands, but apparently not in rev-list.
> > 
> > Listed this patch as RFC because maybe instead it's better to fix
> > something so --abbrev can be used alone, or teach --abbrev-commit=<n>.
> > It looks like `git log --abbrev=5` also doesn't work the way one might
> > expect, which makes sense to me, as they use the same internals for
> > option parsing (parse_revisions()).
> > 
> > The manpages for log and rev-list both correctly indicate that
> > --abbrev=<n> is an optional addition to --abbrev-commit. `git log -h` is
> > generated by parse-options tooling and doesn't cover --abbrev-commit at
> > all, but `git rev-list` doesn't use an option parser on its own and the
> > usage is hardcoded.
> 
> Yeah, "--abbrev" is a bit tricky here. It is really "when you abbrev, do
> it to this level". In "log", that means that "git log --abbrev=5 --raw"
> _does_ do something useful (it abbreviates the blob hashes). And then
> you may add "--abbrev-commit" on top to ask to abbreviate the commit
> ids.
> 
> And rev-list follows that same pattern, except that rev-list _never_
> shows diff output. You'd traditionally do (and this is how log was
> implemented once upon a time):
> 
>   git rev-list HEAD | git diff-tree --stdin --abbrev=5 --raw
> 
> But even there, we are not seeing the commit id output by rev-list. It
> goes to diff-tree, which then formats it separately. So if you wanted
> abbreviated commits there, you'd add "--abbrev-commit" to the diff-tree
> invocation, not rev-list!
> 
> So no, I cannot see a way in which "rev-list --abbrev" is useful without
> "--abbrev-commit". Which means that perhaps the former should imply the
> latter.

Maybe it should; maybe this patch is a good excuse to do so, and enforce
mutual exclusion of --abbrev-commit/--abbrev and --no-abbrev. Maybe it's
also a good time to add --abbrev-commit=<length>?

> 
> And as you noticed in your other patch, there is no way to abbreviate
> "--objects" output at all. I am not sure I have ever seen a good use for
> that. Though to be honest, I am not sure that "--abbrev" is really all
> that useful in the first place. Machine-readable output should never
> abbreviate, and human-readable ones generally already do.
> 
> But at any rate, before making any behavior changes it may make sense to
> think about how they'd interact with "rev-list --objects" abbreviation,
> if it were to be added.
> 
> As for the patch itself:
> 
> > diff --git a/builtin/rev-list.c b/builtin/rev-list.c
> > index 9f31837d30..6ae0087b01 100644
> > --- a/builtin/rev-list.c
> > +++ b/builtin/rev-list.c
> > @@ -49,8 +49,8 @@ static const char rev_list_usage[] =
> >  "    --objects | --objects-edge\n"
> >  "    --unpacked\n"
> >  "    --header | --pretty\n"
> > -"    --abbrev=<n> | --no-abbrev\n"
> > -"    --abbrev-commit\n"
> > +"    --abbrev-commit [--abbrev=<n>]\n"
> > +"    --no-abbrev\n"
> 
> So --no-abbrev clears both --abbrev and --abbrev-commit. That sort of
> makes sense, though I did not expect it. But it also means that:
> 
>   --abbrev-commit [--abbrev=<n> | --no-abbrev]
> 
> is not right. Possibly:
> 
>   --abbrev-commit [--abbrev=<n>] | --no-abbrev
> 
> would show the interaction more clearly, but I don't have a strong
> opinion.

I did consider demonstrating it this way, but when both --abbrev-commit
and --no-abbrev are used together, we don't complain or reject the
invocation - which I would expect if the usage states the two options
are mutually exclusive.

I've been trying to think of good reasons not to enforce their mutual
exclusion, and the one I keep coming back to is that --no-abbrev might
be desired to override a git config'd abbreviation length - although I
didn't check to see whether we have one, maybe we would want one later.
And even in that case, I suppose that --abbrev-commit would not be
explicitly added to the call (because we'd infer from the config), or
that if it did need to be explicitly added (like if we need the user to
say they want abbreviation, but we want to use their configured
preferred length) then we could still reject the addition of
--no-abbrev.

So maybe it makes even more sense to take this patch as an opportunity
to make these options mutually exclusive... although that checking I
think would wind up in revision.c, and therefore widen the impact of
the change significantly.

From a practical standpoint, I guess you could consider --abbrev-commit
and --no-abbrev mutually exclusive, but since it's not enforced, I have
a little preference not to document it as though it is...

 - Emily