Re: Bugreport: Prefix - is ignored when sorting (on committerdate)

From: Jeff King <peff@peff.net>
Date: Jan 10, 2023, 12:18 UTC
Message-ID: <Y71Xnx2vmznV913I@coredump.intra.peff.net>
In-Reply-To: <CAN0heSprKD35P-D7GTVQ20eRsj0AriYEA7iCDnrA9GuiwX0snw@mail.gmail.com>

On Tue, Jan 10, 2023 at 11:54:16AM +0100, Martin Ågren wrote:

Show 23 quoted lines

> On Tue, 10 Jan 2023 at 10:50, Fredrik Öberg <fredrik@bakskuru.se> wrote:
> > - Create a new repository
> > - Add a new file to the repository and commit
> > - Create a tag
> > $ git tag -a TagOne -m ""
> > - Update the file and commit the change
> > - Create a tag
> > $ git tag -a TagTwo -m ""
> > - List tags by committerdate:
> > $ git tag -l --sort=committerdate
> > (oldest tag is listed first)
> > - List tags by reversed committerdate:
> > $ git tag -l --sort=-committerdate
> > (oldest tag it still listed first)
> [...]
> 
> This bisects to 7c5045fc18 ("ref-filter: apply fallback refname sort
> only after all user sorts", 2020-05-03). I've cc-ed the author. That
> commit does change the behavior for the kind of test repo you describe.
> 
> That said, you're using "committerdate" here. If you use "taggerdate"
> (or "creatordate") I think you'll get the output you expect, even for
> newer Git versions. Does that help?

Yes, I think that's the crux of the issue here; there is no date sorting happening at all, before or after that patch. Depending on what you want to do, I think "creatordate" is probably the most common option (it's better than taggerdate in that it handles lightweight tags, too). But if you really want committer dates, then "--sort=-=*committerdate" would work.

Show 10 quoted lines

> Since you just have two commits in the reproducer, there's a strong
> correlation between tag names and the timestamps involved. You actually
> end up sorting by refname: because there is no committerdate for these
> tags, the refnames are compared as a fallback. While the old code then
> applied the reversal ('-') to *that*, the new code first fails to find
> any difference, so doesn't have anything to reverse, then falls back to
> comparing the refnames, at which point it doesn't consider reversing the
> result.
> 
> All of this is based on my understanding. I could obviously be wrong.

I think that explanation is exactly correct. The only difference before and after that patch is whether the "-" is applied to the fallback refname sort.

Show 7 quoted lines

> I suppose it could be argued that the '-' should be applied to the
> fallback as well, e.g., to uphold some sort of "using '-' should give
> the same result as piping the whole thing through tac" (i.e., respecting
> `s->reverse` in `compare_refs()`, if you're following along in
> 7c5045fc18). With multiple sort keys, some with '-' and some
> without, we'd grab the '-' from the first key. It seems like that could
> make sense, actually.

I dunno. Just because you are reverse-sorting on one field doesn't necessarily imply that you want the tie-breaker to reverse-sort, too. I get that it's kind of a "DWIM" when there's a single sorting key, but I think the multi-key behavior is harder to explain. In:

  git for-each-ref --sort=committername --sort=-committerdate

should a refname tie breaker follow the reverse-chronological sort, or the in-order name sort? The date sort is the primary key here, so respecting it would be most like "tac". But the inner tie-breaker is breaking ties on committername, so should it be most like that?

I could see it depending on exactly what you're trying to do. Which leads me to think the rule should be as simple as possible. You can always do:

  git for-each-ref --sort=-refname --sort=-committerdate

to specify exactly what you want.

-Peff