Re: [PATCH v3 09/12] get_short_oid / peel_onion: ^{tree} should be tree, not treeish
Ævar Arnfjörð Bjarmason 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?
Re: [PATCH v3 09/12] get_short_oid / peel_onion: ^{tree} should be tree, not treeish
On Thu, May 03 2018, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason 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 ^{} peel syntax won't
give you *only* the 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:
'{caret}{}', 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 '' recursively until
an object of type '' is found or the object cannot be
dereferenced anymore (in which case, barf).
For example, if '' is a commit-ish, '{caret}\{commit\}'
describes the corresponding commit object.
Similarly, if '' is a tree-ish, '{caret}\{tree\}'
describes the corresponding tree object.
'{caret}0'
is a short-hand for '{caret}\{commit\}'.
Note "until an object of 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 ''
given a request like $short^{}, but rather returning something
matching $short because it could be a container for .
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.
Re: [PATCH v3 09/12] get_short_oid / peel_onion: ^{tree} should be tree, not treeish
Ævar Arnfjörð Bjarmason writes:
> 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
Exactly. The former asks "I (think I) know 7452 can be used to name
an object of type $t, with peeling if necessary--give me the underlying
object of type $t". In short, the fact that you can write "$X^{$t}"
says that $X is a $t-ish (otherwise $X cannot be used as a stand-in
for an object of type $t) and that you fully expect that $X can merely
be of type $t-ish and not exactly $t (otherwise you wouldn't be
making sure to coerce $X into $t with ^{$t} notation).
In *THAT* context, disambiguation help that lists objects whose name
begins with "7452" you gave, hoping that it is a unique enough
prefix when it wasn't in reality, *MUST* give $t-ish; restricting it
to $t makes the help mostly useless.
> 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?
There is no such feature either before or after the disambiguation
help. I am not saying there shouldn't exist such a feature. I am
saying that breaking the existing feature and making it useless is
not the way to add such a feature.
Re: [PATCH v3 09/12] get_short_oid / peel_onion: ^{tree} should be tree, not treeish
On Fri, May 04 2018, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason writes:
>
>> 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
>
> Exactly. The former asks "I (think I) know 7452 can be used to name
> an object of type $t, with peeling if necessary--give me the underlying
> object of type $t".
Right, and I'm with you so far, this makes sense to me for all existing
uses of the peel syntax, otherwise v2.17.0^{tree} wouldn't be the same
as rev-parse v2.17.0^{tree}^{tree}...
> In short, the fact that you can write "$X^{$t}"
> says that $X is a $t-ish (otherwise $X cannot be used as a stand-in
> for an object of type $t) and that you fully expect that $X can merely
> be of type $t-ish and not exactly $t (otherwise you wouldn't be
> making sure to coerce $X into $t with ^{$t} notation).
>
> In *THAT* context, disambiguation help that lists objects whose name
> begins with "7452" you gave, hoping that it is a unique enough
> prefix when it wasn't in reality, *MUST* give $t-ish; restricting it
> to $t makes the help mostly useless.
>
>> 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?
>
> There is no such feature either before or after the disambiguation
> help. I am not saying there shouldn't exist such a feature. I am
> saying that breaking the existing feature and making it useless is
> not the way to add such a feature.
I still don't get how what you're proposing is going to be consistent,
but let's fully enumerate the output of 7452 with my patch to take that
case-by-case[1]:
^{tag}:
7452b4b5786778d5d87f5c90a94fab8936502e20
^{commit}:
hint: 74521eee4c commit 2007-12-01 - git-gui: install-sh from automake
does not like -m755
hint: 745224e04a commit 2014-06-18 - refs.c: SSE2 optimizations for
check_refname_component
^{tree}:
hint: 7452336aa3 tree
hint: 74524f384d tree
hint: 7452813bcd tree
hint: 7452b1a701 tree
hint: 7452b73c42 tree
hint: 7452ca1557 tree
^{blob}:
hint: 7452001351 blob
hint: 745254665d blob
hint: 7452a572c1 blob
hint: 7452b9fd21 blob
hint: 7452db13c8 blob
hint: 7452fce0da blob
And[2]:
core.disambiguate=tag:
[same as ^{tag]
core.disambiguate=commit:
[same as ^{commit}]
core.disambiguate=committish:
hint: 7452b4b578 tag v2.1.0
hint: 74521eee4c commit 2007-12-01 - git-gui: install-sh from automake
does not like -m755
hint: 745224e04a commit 2014-06-18 - refs.c: SSE2 optimizations for
check_refname_component
core.disambiguate=tree:
[same as ^{tree}]
core.disambiguate=treeish (same as $sha1:)
hint: 7452b4b578 tag v2.1.0
hint: 74521eee4c commit 2007-12-01 - git-gui: install-sh from automake
does not like -m755
hint: 745224e04a commit 2014-06-18 - refs.c: SSE2 optimizations for
check_refname_component
hint: 7452336aa3 tree
hint: 74524f384d tree
hint: 7452813bcd tree
hint: 7452b1a701 tree
hint: 7452b73c42 tree
hint: 7452ca1557 tree
core.disambiguate=blob:
[same as ^{blob}]
So from my understanding of what you're saying you'd like to list tag,
commits and trees given $sha1^{tree}, because they're all types that can
be used to reach a tree.
I don't think that's very useful, yes it would "break" existing
disambiguations, but this is such an obscure (and purely manual)
use-case than I think that's fine.
Because I think to the extent anyone's going to use this it's because
they know they have e.g. a short blob, commit etc. SHA-1 they're not
going to use it because they have some short $SHA they know is a tree,
and then want all SHA-1s of that *and* random tag & commit objects that
happen to have the same object prefix just because tags and commits can
also point to trees.
How does that make any sense? The entire reason for using the normal
peel syntax is because you e.g. have v2.17.0 and want to get to the
^{tree} or the ^{commit} tht v2.17.0 directly points to. That's entirely
orthogonal to what the disambiguation is doing. There with your proposed
semantics you're peeling 7452 as 7452^{tree} because (IMO) you're
looking for trees, just to get some entirely unrelated commits and tags.
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, comm
Re: [PATCH v3 09/12] get_short_oid / peel_onion: ^{tree} should be tree, not treeish
Ævar Arnfjörð Bjarmason writes:
> Right, and I'm with you so far, this makes sense to me for all existing
> uses of the peel syntax, otherwise v2.17.0^{tree} wouldn't be the same
> as rev-parse v2.17.0^{tree}^{tree}...
More importantly, you could spell v2.17.0 part of the above with a
short hexadecimal string. And that string should be naming some
tree-ish, the most important thing being that it is *NOT* required
to be a tree (and practically, it is likely that the user has a
tree-ish that is *NOT* a tree).
I guess I have a reaction to the title
"get_short_oid/peel_onion: ^{tree} should be tree"
"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.
> case-by-case[1]:
>
> ^{tag}:
> 7452b4b5786778d5d87f5c90a94fab8936502e20
I take it as "git rev-parse 7452^{tag}" output (similarly ^{$type}
for the rest)? That probably is desirable, as blobs, trees and
commits cannot be peeled down to a tag.
> ^{commit}:
> hint: 74521eee4c commit 2007-12-01 - git-gui: install-sh from automake
> does not like -m755
> hint: 745224e04a commit 2014-06-18 - refs.c: SSE2 optimizations for
> check_refname_component
If 7452 points at a commit, that tag itself should also be given as
a possible object the user may have meant in the "hint" thing. I
agree it is a good idea to exclude trees and blobs from the hint,
for the same reason why I think it makes sense to exclude blobs,
trees and commits from hints for a X in "X^{tag}" above.
> ^{tree}:
> hint: 7452336aa3 tree
> hint: 74524f384d tree
> hint: 7452813bcd tree
> hint: 7452b1a701 tree
> hint: 7452b73c42 tree
> hint: 7452ca1557 tree
Again, if there is a commit or a tag (that points at a commit or a
tree) whose name begins with 7452, it should be included in the hint
above. Not having blobs in the hint of course makes sense, as a
blob cannot be X in "X^{tree}".
> And[2]:
>
> core.disambiguate=tag:
> [same as ^{tag]
> core.disambiguate=commit:
> [same as ^{commit}]
When core.disambiguate tells us to "interprete hexadecimal literals
to name commit objects only", giving only commits in hints: section
makes sense, because we are explicitly saying that "when I say 7452,
I do not mean any tag whose name begins with 7452", so "sorry, your
request is not explicit enough---there are two commits and a tag
that begin with that prefix" is not helpful---it should stop at "you
may have meant one of these two commits" and not mention any tag.
Re: [PATCH v3 09/12] get_short_oid / peel_onion: ^{tree} should be tree, not treeish
On Mon, May 07, 2018 at 01:08:46PM +0900, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason writes:
>
> > Right, and I'm with you so far, this makes sense to me for all existing
> > uses of the peel syntax, otherwise v2.17.0^{tree} wouldn't be the same
> > as rev-parse v2.17.0^{tree}^{tree}...
>
> More importantly, you could spell v2.17.0 part of the above with a
> short hexadecimal string. And that string should be naming some
> tree-ish, the most important thing being that it is *NOT* required
> to be a tree (and practically, it is likely that the user has a
> tree-ish that is *NOT* a tree).
>
> I guess I have a reaction to the title
>
> "get_short_oid/peel_onion: ^{tree} should be tree"
>
> "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".
-Peff
Re: [PATCH v3 09/12] get_short_oid / peel_onion: ^{tree} should be tree, not treeish
On Tue, May 08 2018, Jeff King wrote:
> On Mon, May 07, 2018 at 01:08:46PM +0900, Junio C Hamano wrote:
>
>> Ævar Arnfjörð Bjarmason writes:
>>
>> > Right, and I'm with you so far, this makes sense to me for all existing
>> > uses of the peel syntax, otherwise v2.17.0^{tree} wouldn't be the same
>> > as rev-parse v2.17.0^{tree}^{tree}...
>>
>> More importantly, you could spell v2.17.0 part of the above with a
>> short hexadecimal string. And that string should be naming some
>> tree-ish, the most important thing being that it is *NOT* required
>> to be a tree (and practically, it is likely that the user has a
>> tree-ish that is *NOT* a tree).
>>
>> I guess I have a reaction to the title
>>
>> "get_short_oid/peel_onion: ^{tree} should be tree"
>>
>> "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.
I think a response to the part after "leaving that aside" of my upthread
E-Mail
(https://public-inbox.org/git/87lgczyfq6@evledraar.gmail.com/) would
help me out.
Not to belabor the point, but here's a patch I came up with to
revisions.txt that's a WIP version of something that would describe the
worldview after this v3:
diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt
index dfcc49c72c..0bf68f4ad2 100644
--- a/Documentation/revisions.txt
+++ b/Documentation/revisions.txt
@@ -143,29 +143,52 @@ thing no matter the case.
'{caret}1{caret}1{caret}1'. See below for an illustration of
the usage of this form.
'{caret}{}', 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 '' recursively until
an object of type '' is found or the object cannot be
- dereferenced anymore (in which case, barf).
+ dereferenced anymore (in which case either return that object type, or
barf).
For example, if '' is a commit-ish, '{caret}\{commit\}'
describes the corresponding commit object.
Similarly, if '' is a tree-ish, '{caret}\{tree\}'
describes the corresponding tree object.
'{caret}0'
is a short-hand for '{caret}\{commit\}'.
+
'rev{caret}\{object\}' can be used to make sure 'rev' names an
object that exists, without requiring 'rev' to be a tag, and
without dereferencing 'rev'; because a tag is already an object,
it does not have to be dereferenced even once to get to an object.
+
'rev{caret}\{tag\}' can be used to ensure that 'rev' identifies an
existing tag object.
++
+Object types that don't have a 1=1 mapping to other object types
+cannot be dereferenced with the peel syntax, and will return an
+error. E.g. '{caret}{commit}' or '{caret}{tree}' is
+allowed because a tag can only point to one commit, and a commit can
+only point to one tree. But '{caret}{blob}' will always
+produce an error since trees in general don't 1=1 map to blobs, even
+though the specific '' in question might only contain one
+blob. Note that '{caret}{blob}' is not an error if '' is
+a tag that points directly to a blob, since that again becomes
+unambiguous.
++
+'{caret}{}' takes on a different meaning when '' is a
+SHA-1 that's ambiguous within the object store. In that case we don't
+have a 1=1 mapping anymore. E.g. e8f2 in git.git can refer to multiple
+objects of all the different object types. In that case
+{caret}{} should always be an error to be consistent with the
+logic above, but that wouldn't be useful to anybody. Instead it'll
+fall back to being selector syntax for the given object types,
+e.g. e8f2{caret}{tag} will (as of writing this) return the v2.17.0
+tag, and {caret}{commit}, {caret}{tree} and {caret}{blob} will return
+commit, tree and blob objects, respectively.
+
[...]
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.
So it would be very useful to me if you or someone who understands the
behavior you & Junio seem to want
Re: [PATCH v3 09/12] get_short_oid / peel_onion: ^{tree} should be tree, not treeish
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?
> I think a response to the part after "leaving that aside" of my upthread
> E-Mail
> (https://public-inbox.org/git/87lgczyfq6@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.
> 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.
-Peff
Re: [PATCH v3 09/12] get_short_oid / peel_onion: ^{tree} should be tree, not treeish
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^{} doesn't mean do the
peel itself in X any way, rather it means list all those objects
matching X where a subsequent X^{} 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@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 disambi
Re: [PATCH v3 09/12] get_short_oid / peel_onion: ^{tree} should be tree, not treeish
Ævar Arnfjörð Bjarmason writes:
>> 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^{} doesn't mean do the
> peel itself in X any way, rather it means list all those objects
> matching X where a subsequent X^{} 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^{} 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^{}, i.e. the
user expects that X names a single object, that can be peeled to
.
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 .
When diagnosing such an error, we would give hints. The hint would
show possible objects that the user could have meant with X. The
^{} 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 , the short and ambiguous prefix
could mean any of these four objects, but when given with
suffix, e.g. ^{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 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 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).
Re: [PATCH v3 09/12] get_short_oid / peel_onion: ^{tree} should be tree, not treeish
On Thu, May 10, 2018 at 01:21:19PM +0900, Junio C Hamano wrote:
> When diagnosing such an error, we would give hints. The hint would
> show possible objects that the user could have meant with X. The
> ^{} 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 , the short and ambiguous prefix
> could mean any of these four objects, but when given with
> suffix, e.g. ^{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 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 actually wondered this while writing an earlier response, and so I
happen to know: when we are looking for a treeish, the disambiguator
will actually peel a candidate tag and only accept one that peels to a
tree or commit. So we would omit the tag-to-blob entirely from
consideration (both as a candidate for ambiguity, and in the hint list).
-Peff
