Re: Bug: Git sees branch as valid commit ref and works; should fail

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> 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.

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