Re: [PATCH v3 09/12] get_short_oid / peel_onion: ^{tree} should be tree, not treeish

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: May 3, 2018, 07:28 UTC
Message-ID: <87wowlxko8.fsf@evledraar.gmail.com>
In-Reply-To: <xmqqzi1htij7.fsf@gitster-ct.c.googlers.com>

On Thu, May 03 2018, Junio C Hamano wrote:

Show 33 quoted lines

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> But ^{tree} shows just the trees, but would previously be equivalent
>> to the above:
>>
>>     $ git rev-parse e8f2^{tree}
>>     error: short SHA1 e8f2 is ambiguous
>>     hint: The candidates are:
>>     hint:   e8f2093055 tree
>>     hint:   e8f25a3a50 tree
>>     hint:   e8f28d537c tree
>>     hint:   e8f2cf6ec0 tree
>>     [...]
>
> When a user says "git $cmd e8f2^{tree}", the user is telling Git
> that s/he knows e8f2 *is* a tree-ish, but for whatever reason $cmd
> wants a tree and does not accept an arbitrary tree-ish---that is the
> whole piont of appending ^{tree} as a suffix.  A useful hint in such
> a case would be "oh, you said e8f2 is a tree-ish, but there are more
> than one tree-ish, so let me show them to you to help you decide
> which one among them is the one you meant".  When $cmd is rev-parse,
> I would even say that the user is saying "I know e8f2 is a tree-ish,
> and I know it not a tree--it merely is a tree-ish.  I want the tree
> that e8f2 thing points at".
>
> Limiting that hint to show only real trees does not make any sense
> to me.  I do not think we care _too_ deeply, because most of the
> time, command line location that expects a tree-ish can be given a
> tree-ish, so there is not much reason to use ^{tree} suffix these
> days.  But in a case where it _does_ matter, I think this change
> makes the "hint" almost useless.
>
> Or am I misleading what you wanted to achieve with this patch?

The reason I'm doing this is because I found it confusing that I can't do:

    for t in tag commit tree blob; do ./git --exec-path=$PWD rev-parse 7452^{$t}; done

And get, respectively, only the SHAs that match the respective type, but currently (with released git) you can do:

    for t in tag commit committish treeish tree blob; do git -c core.disambiguate=$t rev-parse 7452; done

And while =tag doesn't work the others do (inluding =blob), so core.disambiguate=tree gives you just trees, but ^{tree} gives you treeish.

Why should ^{tree} be giving me ^{treeish} but =tree be giving me trees, and =treeish be synonymous with ^{tree}?

There's no other cases I know of where the ^{<type>} peel syntax won't give you *only* the <type> you asked for. See peel_onion() -> peel_to_type() and how get_oid_1() will short-circuit if it has an answer, and then finally fall back to this get_short_oid() codepath.

Looking at the code & git log maybe it'll do that internally, but when you peel a tag or commit ^{tree} will only ever find one thing, unlike this disambiguation case where we can match multiple things.

So:

1) Am I missing some subtlety or am I correct that there was no way to
get git to return more than one SHA-1 for ^{commit} or ^{tree} before
this disambiguation feature was added?

2) I think the behavior I've implemented is consistent with how the peel
syntax has been documented in revisions.txt:

    '<rev>{caret}{<type>}', e.g. 'v0.99.8{caret}\{commit\}'::
      A suffix '{caret}' followed by an object type name enclosed in
      brace pair means dereference the object at '<rev>' recursively until
      an object of type '<type>' is found or the object cannot be
      dereferenced anymore (in which case, barf).
      For example, if '<rev>' is a commit-ish, '<rev>{caret}\{commit\}'
      describes the corresponding commit object.
      Similarly, if '<rev>' is a tree-ish, '<rev>{caret}\{tree\}'
      describes the corresponding tree object.
      '<rev>{caret}0'
      is a short-hand for '<rev>{caret}\{commit\}'.

Note "until an object of type '<type>' is found". I.e. my mental model of this has been that yes you can *start* the search at a different object (e.g. tag -> tree), but it'll only ever return the tree. The disambiguation implementation has been inconsistent with this as documented, because it hasn't been drilling don to an object of '<type>' given a request like $short^{<type>}, but rather returning something matching $short because it could be a container for <type>.

Anyway, I'm not saying I'm right. This is the first time I really look at sha1-name.c in any detail. But the above describes the thought process behind this patch.