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

Thank you for filling out a Git bug report! Please answer the following questions to help us understand your issue.

What did you do before the bug happened? (Steps to reproduce your issue)
- 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)

What did you expect to happen? (Expected behavior) Using the minus-sign as prefix is supposed to reverse the sorting order, listing the newest item first. I expected the newest tag to be listed first

What happened instead? (Actual behavior) The minus-sign was ignored, and the oldest tag was still listed first

What's different between what you expected and what actually happened? I expected the newest tag to be listed first when the minus-sign was used as a prefix, but the oldest tag was still listed first.

Anything else you want to add: The correct behaviour (reversing sort-order by using the minus-prefix) has been verified in version 2.11.0, 2.11.1, 2.14.2, 2.19.1, 2.19.2, 2.23.0 on CentOS 7.4 version 2.31.1 and 2.35.1 on CentOS 7.4 contains the error version 2.34.1.windows.1 contains the error

Please review the rest of the bug report below. You can delete any lines you don't wish to share.

[System Info] git version: git version 2.34.1.windows.1 cpu: x86_64 built from commit: 2ca94ab318509b3c271e82889938816bad76dfea sizeof-long: 4 sizeof-size_t: 8 shell-path: /bin/sh feature: fsmonitor--daemon uname: Windows 10.0 19042 compiler info: gnuc: 11.2 libc info: no libc information available $SHELL (typically, interactive shell): C:\Program Files\Git\usr\bin\bash.exe

[Enabled Hooks]

Hej Fredrik,

On Tue, 10 Jan 2023 at 10:50, Fredrik Öberg <fredrik@bakskuru.se> wrote:

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?

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 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 might post such a patch, but as noted above, I think what you really want to use is "taggerdate". I hope that helps.

Martin

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

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.

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.

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

On Tue, 10 Jan 2023 at 13:18, Jeff King <peff@peff.net> wrote:

[...]

Indeed. So probably best to leave it as-is, then.

Martin

Thanks for the quick response! I've been playing with creatordate and it seems to work with with both older and newer versions of git.

I am trying to understand what I did wrong. I guess you are saying that I tried to use the field committerdate, which is not available on the tags? And that the handling of this situation has changed since git/2.23? Hence I got different results?

/Fredrik

Den tis 10 jan. 2023 kl 14:05 skrev Martin Ågren <martin.agren@gmail.com>:

On Tue, Jan 10, 2023 at 02:33:14PM +0100, Fredrik Öberg wrote:

Right. The committerdate of a tag is a blank string, so sorting on it was not doing anything.

Sort of. Your committerdate sort was always doing nothing, but due to the buggy way that the sort handled ties in 2.23 and older, it happened to do a reverse refname sort instead of a forward refname sort. Now it does a forward refname sort to break ties.

But the most important change for you is to actually do a real date sort, so the tie-breaking should not be important either way.

-Peff