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

From: Junio C Hamano <gitster@pobox.com>
Date: May 10, 2018, 04:21 UTC
Message-ID: <xmqqa7t8kuo0.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <878t8txfyf.fsf@evledraar.gmail.com>

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Show 20 quoted lines

>> Before we had any disambiguation code, resolving X^{tree} really was two
>> independent steps: resolve X, and then peel it to a tree. When we added
>> the disambiguation code, the goal was to provide a hint to the first
>> step in such a way that we could never eliminate any resolutions that
>> the user _might_ have meant. But it's OK to take a situation where every
>> case but one would result in an error, and assume the user meant that
>> case. Sort of a "do no harm" rule.
>>
>> By disambiguating with just a tree and not a tree-ish, that hint is now
>> eliminating possibilities that would have worked in the second step,
>> which violates the rule.
>>
>> Does thinking about it that way make more sense?
>
> Okey, so to rephrase that to make sure I understand it. It would be
> documented as something like this:
>
>     When the short SHA-1 X is ambiguous X^{<type>} doesn't mean do the
>     peel itself in X any way, rather it means list all those objects
>     matching X where a subsequent X^{<type>} wouldn't be an error.

With the understanding that this paragraph is written primarily for your own enlightenment, I wouldn't complain too much, but if you meant this to become part of end-user documentation, I have a strong issue with the verb "list" used here.

X^{<type>} never means to "list" anything (FWIW just X does not mean to "list" anything, either). It just means that the user wants to specify a single object whose object name is X^{<type>}, i.e. the user expects that X names a single object, that can be peeled to <type>.

Now, it is an error when (1) X does not specify a single object in the first place, or (2) the single object cannot be peeled to <type>.

When diagnosing such an error, we would give hints. The hint would show possible objects that the user could have meant with X. The ^{<type>} suffix given to it may be used to limit the hints to subset of the objects that the user could have meant with X; e.g. when there is an object of each of type blob, tree, commit, and tag, whose name begins with 7777, the short and ambiguous prefix 7777 could mean any of these four objects, but when given with suffix, e.g. 7777^{tree}, it makes useless for the hint to include the blob object, as it can never peel down to a tree object.

If the tag whose name begins with 7777 in this example points directly to a blob, excluding that tag from the hint would make the hint more useful. I do not offhand know what the code does right now. I wouldn't call it a bug if such a tag is included in the hint, but if a change stops such a tag from getting included, I would call such a change an improvement.

>     I.e. X^{commit} will list tags and commits, since both can be peeled
>     to reveal a commit, X^{tree} will similarly list tags, commits and
>     trees, and X^{blob} will list tags and blobs[1], and X^{tag} will
>     only list tags.

Modulo the use of "list", which I have trouble is as it makes it sound as if listing something is the purpose of the notation, I think we are on the same page up to this point

I think the best way to explain core.disambiguate to readers is to make them rethink what I meant with "the user expects that X names a single object" in the early part of the above response.

Without constraint, Git understood that the user used 7777 to name any one of the objects of all four types. With core.disambiguate, the user can tell Git "when I give potentially ambiguous object name with a short prefix, assume that only a commit or a tag whose name begins with the prefix is what I expected the short prefix to name uniquely", so Git understood that the user wanted to name either a commit or a tag. It would still trigger an error as it does not uniquely name an object (for which an attempt to apply the ^{tree} peeling would further be made).