git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
From: Emily Shaffer <emilyshaffer@google.com>
To: Jeff King <peff@peff.net>
Cc: git@vger.kernel.org
Subject: Re: [RFC PATCH] rev-list: clarify --abbrev and --abbrev-commit usage
Date: Fri, 14 Jun 2019 13:59:50 -0700
Message-ID: <20190614205950.GC233791@google.com> (raw)
In-Reply-To: <20190614161841.GB30083@sigill.intra.peff.net>

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

  reply index

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-13 22:15 Emily Shaffer
2019-06-14 16:09 ` Junio C Hamano
2019-06-14 16:18 ` Jeff King
2019-06-14 20:59   ` Emily Shaffer [this message]
2019-06-14 21:27     ` Jeff King
2019-06-14 22:56       ` Emily Shaffer
2019-06-19 21:21         ` Jeff King
2019-06-19 22:09           ` Emily Shaffer

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190614205950.GC233791@google.com \
    --to=emilyshaffer@google.com \
    --cc=git@vger.kernel.org \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror http://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox