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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Wed, May 09 2018, Jeff King wrote:

> On Tue, May 08, 2018 at 08:53:10PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> >> "X^{tree}" should *RESULT* in a tree, but it should *REQUIRE* X to
>> >> be a tree-ish.  It is unclear "should be tree" is about the former
>> >> and I read (perhaps mis-read) it as saying "it should require X to
>> >> be a tree"---that statement is utterly incorrect as we agreed above.
>> >
>> > FWIW, I had the same feeling as you when reading this, that this commit
>> > (and the one after) are doing the wrong thing. And these paragraphs sum
>> > it up. The "^{tree}" is about asking us to peel to a tree, not about
>> > resolving X in the first place. We can use it as a _hint_ when resolving
>> > X, but the correct hint is "something that can be peeled to a tree", not
>> > "is definitely a tree".
>>
>> Maybe I'm just being dense, but I still don't get from this & Junio's
>> E-Mails what the general rule should be.
>
> Let me try to lay out my thinking a bit more clearly, and then I'll try
> to respond to the points you laid out below.
>
> 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.

    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.

    But core.disambiguate=[tag|commit|tree|blob] is not at all like
    ^{[tag|commit|tree|blob]} and is unlike the peel syntax only going
    to list the objects of the respective type. The config synonym for
    the peel syntax is committish, treeish, and the nonexistent blobish.

>> I think a response to the part after "leaving that aside" of my upthread
>> E-Mail
>> (https://public-inbox.org/git/87lgczyfq6.fsf@evledraar.gmail.com/) would
>> help me out.
>
> I'll quote that bit here:
>
>> But *leaving that aside*, i.e. I don't see why the use-case would make
>> sense. What I *don't* get is why, if you think that, you only want to
>> apply that rule to ^{tree}. I.e. wouldn't it then be consistent to say:
>>
>>     # a)
>>     ^{tag}    = tag
>>     ^{commit} = tag, commit
>>     ^{tree}   = tag, commit, tree
>>     ^{blob}   = tag, blob (blobish)
>
> Yes, that makes sense to me conceptually, and would follow the rule I
> gave above. And I think that's what we do now, with the exception that
> there is no blobish disambiguation. Presumably nobody ever bothered
> because probably because tagged blobs are pretty rare (and obviously
> though trees point to blobs, you cannot disambiguate that way since
> there's no one-to-one correspondence).
>
> So I doubt anybody really cares in practice, but I agree that it would
> improve consistency to write a patch to introduce GET_OID_BLOBISH and
> have "^{blob}" parsing use it.  And possibly add "blobish" to
> core.disambiguate (or is it "blobbish"?), though that's almost certainly
> something nobody would ever use.

Yeah, I'll introduce it for consistency. To clarify I wasn't trying to
make some argument on the basis that we didn't have it, but I was
confused because I couldn't see how the general rule would apply to
^{tree} and not ^{blob}.

>> My understanding of what you two are saying is that somehow the peel
>> semantics should be preserved when we take this beyond the 1=1 mapping
>> case, but I don't see how if we run with that how we wouldn't need to
>> introduce the concept of blobish for consistency as I noted upthread.
>
> Yeah, I think the lack of blobish is a bug, just one that nobody has
> ever really cared about.
>
>> So it would be very useful to me if you or someone who understands the
>> behavior you & Junio seem to want could write a version of the patch I
>> have above where the last paragraph is different, and describes the
>> desired semantics, because I still don't get it. Why would we 1=many
>> peel commits to trees as a special case, but not 1=many do the same for
>> trees & blobs?
>
> I'm not sure I understand the mention of trees in the final sentence.
> AFAICT tree disambiguation is consistent with the peeling rules.

Yeah nevermind that, I was imagining some semantics where because we
dropped the 1=1 mapping ^{tree} would list blobs, but in the worldview
you describe above (if I got it right) that doesn't make sense.

1. Not currently, but I should amend my ^{blob} patch to work like that.