Re: Bug: Git sees branch as valid commit ref and works; should fail
From: Junio C Hamano <hidden>
Date: 2024-08-13 15:41:28
Jeff King [off-list ref] writes:
$ git rev-parse bugfix/mathomp4/trivial-ci-commit-gcc14 cc14d30e332cd06327fe5a81ed26c24140882f42 That narrows it down to the name resolution code. If I step through it in a debugger, the culprit seems to be get_describe_name().
ROTFL. Couldn't resist laughing with wonder, as I totally missed the fact that -gcc14 exactly looks like a valid describe suffix.
In other words, it's a false positive in the name resolver looking for
"describe" names. We'd prefer a real ref of that full name, I think, but
since there isn't one, we prefer the describe resolution rather than
treating it as a path.
I can think of a few ways to make this better:
- we ignore everything before the "-g<hex>" part entirely. Generally
this should be the name of a tag or at least some ref, so we could
perhaps verify that. But part of the point of sticking the hash in
the name is that you might have gotten the name from another source,
and your local one might not have the same tag. So that might be a
bad direction.Sad but you are absolutely right.
- the hash is abbreviated in the usual way, making it as short as
possible while remaining unambiguous. But unless the user goes out
of their way to set core.abbrev to something smaller, the minimum is
always 7. So perhaps get_describe_name() should be a bit more picky
about about that?
That doesn't fix the problem, but it makes it a lot less likely to
trigger in the real world. And anybody who really does somehow end
up with a describe name with 4 characters can always pick the hash
out of the string themselves (or just set core.abbrev in their local
repo to be more permissive).That sounds like not-so-bad a direction to go. But notice that autogenerated preformatted documentation repositories record the corresponding source material using the "describe" name that is instructed to use absolutely minimum 4 chars when possible. I would imagine that it is not only me who does that, assuming that those who care enough about the correspondence between the commits in artifact repository and the commits in source repository would have tags used by the "describe" from the source repository.
quoted hunk
I think the second one is something like this:diff --git a/object-name.c b/object-name.c index 527b853ac4..a90338aa62 100644 --- a/object-name.c +++ b/object-name.c@@ -1276,6 +1276,10 @@ static int get_describe_name(struct repository *r, if (ch == 'g' && cp[-1] == '-') { cp++; len -= cp - name; + if (len < (default_abbrev < 0 ? + FALLBACK_DEFAULT_ABBREV : + default_abbrev)) + return -1; return get_short_oid(r, cp, len, oid, flags); }
So, perhaps we probably would want to allow shorter than the fallback default hexadecimal after "-g" _iff_ the leading "tag" part actually names an existing tag, or something like that. Thanks.