On Fri, Dec 06, 2024 at 12:01:41PM +0900, Junio C Hamano wrote:
> Jeff King <peff@xxxxxxxx> writes:
>
> > 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@xxxxxxxxxx>
> > 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?
Oh, whoops. I had originally intended to just write the commit message
and leave him with credit, but then I ended up changing approach.
Leaving him as the author was an oversight.
> > 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?
Urgh, yes, you are correct. I can resend, but I think we should resolve
the questions below.
> 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
Right, exactly.
> > 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.
I think you could argue that they are mutually incompatible. But we have
never marked them as such, nor do we do any sort of last-one-wins.
They are two distinct options, but in --exact-match mode, --always is
simply ignored. Which I think is a bug.
> 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 there can be a more expansive reading of --exact-match (or of
--candidates=0), which is: only output a tag that matches exactly. And
then --always is orthogonal to that. There is no other output to
produce, so we show the commit object itself.
Now that more expansive reading is not what --exact-match says above.
But it is the only thing that makes sense to me for --candidates=0, and
the two are synonyms.
> 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?
I think there are really two questions here:
1. Is the current behavior of "describe --exact-match --always" a bug?
I'll grant that probably nobody cares deeply, which is why the
interaction has not been noticed for all of these years. I think
the semantics this patch gives are the only ones that make sense,
but I also don't care that deeply. But...
2. What should we do about the new regression caused by limiting the
candidate list? I.e., my earlier patches in this topic make us
behave as if --candidates=<n> was given when there are fewer tags
in the repo. That runs afoul of the special-casing of
--candidates=0 when there are no tags in the repo (or you limit the
candidates to zero via --match).
If we are not going to address (1) as this patch does, then we need
another solution. We can internally hold an extra variable to
distinguish the number of user-requested candidates from the number
of actual candidates available. But I think my solution to (1) here
harmonizes the --candidates=0 case with --always, and then the
auto-adjusted max-candidates case just falls out naturally.
-Peff
