Re: [PATCH] describe: drop early return for max_candidates == 0

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 6, 2024, 03:01 UTC
Message-ID: <xmqqser1zf8q.fsf@gitster.g>
In-Reply-To: <20241205201449.GA2635755@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 6 quoted lines

> Actually, after thinking on this a bit more, I think the solution below
> is a bit more elegant. This can go on top of jk/describe-perf.
>
> -- >8 --
> From: Josh Steadmon <steadmon@google.com>
> Subject: [PATCH] describe: drop early return for max_candidates == 0

OK, so the patch authorship still blames Josh. But there is no sign-off because ... the approach to the fix is so different that blaming Josh for this implementation is no longer appropriate?

> Reported-by: Josh Steadmon <steadmon@google.com>
> Signed-off-by: Jeff King <peff@peff.net>

If so, please take the authorship yourself.

Show 13 quoted lines

> Before we even start the describe algorithm, we check to see if
> max_candidates is 0 and bail immediately if we did not find an exact
> match. This comes from 2c33f75754 (Teach git-describe --exact-match to
> avoid expensive tag searches, 2008-02-24), since the the --exact-match
> option just sets max_candidates to 0.
> ...
> So this:
>
>   git describe --exact-match --always
>
> and likewise:
>
>   git describe --exact-match --candidates=0

Did the latter mean to say "git decribe --candidates=0 --always", as the earlier paragraph explains that "--exact" affects the number of candidates?

Without this patch, all three give the same result:

    $ git describe --exact-match --always HEAD
    fatal: no tag exactly matches '59d18088fe8ace4bf18ade27eeef3664fb6b0878'
    $ git describe --exact-match --candidates=0 HEAD
    fatal: no tag exactly matches '59d18088fe8ace4bf18ade27eeef3664fb6b0878'
    $ git describe --candidates=0 --always HEAD
    fatal: no tag exactly matches '59d18088fe8ace4bf18ade27eeef3664fb6b0878'

With this patch, we instead get this:

    $ ./git describe --exact-match --always HEAD
    59d18088fe
    $ ./git describe --exact-match --candidates=0 HEAD
    fatal: No tags can describe '59d18088fe8ace4bf18ade27eeef3664fb6b0878'.
    Try --always, or create some tags.
    $ ./git describe --candidates=0 --always HEAD
    59d18088fe

Show 5 quoted lines

> But this interacts badly with the --always option (ironically added only
> a week later in da2478dbb0 (describe --always: fall back to showing an
> abbreviated object name, 2008-03-02)). With --always, we'd still want to
> show the hash rather than calling die().
> ...

> has always been broken.

Hmph, I am not sure if the behaviour is _broken_ in the first place.

The user asks with "--exact-match" that a result based on some ref that does not directly point at the object being described is *not* acceptable, so with or without "--always", it looks to me that it is doing the right thing, if there is no exact match (or there is no tag and the user only allowed tag to describe the objects) and the result is "no tag exactly matches object X" failure.

Or is our position that these mutually incompatible options, namely "--exact-match" and "--always", follow the "last one wins" rule? The implementation does not seem to say so.

If the earlier request is to describe only as exact tag (and fail if there is no appropriate tag), but then we changed our mind and ask to fall back to an abbreviation, this one is understandable:

    $ ./git describe --exact-match --always HEAD
    59d18088fe

But then this is not. The last thing we explicitly told the command is that we accept only the exact match, but this one does not fail, which seems like a bug:

    $ ./git describe --always --exact-match HEAD
    59d18088fe

So I am not sure if the "buggy" behaviour is buggy to begin with. The way these two are documented can be read both ways,

    --exact-match::
            Only output exact matches (a tag directly references the
            supplied commit).  This is a synonym for --candidates=0.

    --always::
            Show uniquely abbreviated commit object as fallback.

but my reading is when you give both and when the object given is not directly pointed at by any existing tag, "ONLY output exact matches" cannot be satisified. And "show as fallback" cannot be satisfied within the constraint that the command is allowed "only output exact matches".

I think the complexity from the point of view of a calling script to deal with either behaviour is probably similar. If you ask for "--exact-match" and there is no exact match, you can ask rev-parse to give a shortened one, and you know which one you are giving the user. We can change what "--exact-match + --candidate=0" combination means to let it fallback, but then you'd need to check the output to see if you got an exact tag or a fallback, and for that you'd probably need to ask "show-ref refs/tags/$output" or something.

So I am not sure if it is worth changing the behaviour this late in the game?