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

From: Jeff King <peff@peff.net>
Date: May 9, 2018, 07:56 UTC
Message-ID: <20180509075632.GA3327@sigill.intra.peff.net>
In-Reply-To: <87a7tax9m1.fsf@evledraar.gmail.com>

On Tue, May 08, 2018 at 08:53:10PM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 14 quoted lines

> >> "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?

> 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:

Show 9 quoted lines

> 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.

> 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.

Show 6 quoted lines

> 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.

-Peff