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

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

[Fredrik Öberg]

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]

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

[Martin Ågren]

Hej Fredrik,

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)

> 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

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

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

[Jeff King]

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

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

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

> 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

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

[Martin Ågren]

On Tue, 10 Jan 2023 at 13:18, Jeff King <peff@peff.net> wrote:
>
> On Tue, Jan 10, 2023 at 11:54:16AM +0100, Martin Ågren wrote:
>
> > 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
[...]

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

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

Martin

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

[Fredrik Öberg]

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, 10 Jan 2023 at 13:18, Jeff King <peff@peff.net> wrote:
> >
> > On Tue, Jan 10, 2023 at 11:54:16AM +0100, Martin Ågren wrote:
> >
> > > 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
> [...]
>
> > 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.
>
> Indeed. So probably best to leave it as-is, then.
>
> Martin

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

[Jeff King]

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

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

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

> And that the handling of this situation has changed since
> git/2.23? Hence I got different results?

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