for-each-ref output order change in 2.7.0

6 messages, 4 authors, 2016-06-15 · open the first message on its own page

for-each-ref output order change in 2.7.0

From: Bryan Turner <hidden>
Date: 2016-06-15 23:07:40

In one of our tests, we have a set of branches whose names are all
special characters (%, @, etc). Most of them branches have identical
tip commits and just have different names. In 2.7.0, when ordering by
-committerdate, the branches are now returned in a different order. I
don't think this is a bug, based on the commit it bisects to, but I'm
wondering if someone can confirm.

2.6.5 and prior (tested all the way back to 1.7.6, so this was
consistent for a long time):

refs/heads/!@#% -> Tue Jan 3 17:04:06 2012 +1100
refs/heads/!@#$% -> Tue Jan 3 17:00:51 2012 +1100
refs/heads/% -> Tue Jan 3 17:00:51 2012 +1100
refs/heads/@#$% -> Tue Jan 3 17:00:51 2012 +1100
refs/heads/@#% -> Tue Jan 3 17:00:51 2012 +1100

2.7.0:

refs/heads/!@#% -> Tue Jan 3 17:04:06 2012 +1100
refs/heads/@#% -> Tue Jan 3 17:00:51 2012 +1100
refs/heads/@#$% -> Tue Jan 3 17:00:51 2012 +1100
refs/heads/% -> Tue Jan 3 17:00:51 2012 +1100
refs/heads/!@#$% -> Tue Jan 3 17:00:51 2012 +1100

I've bisected this back to:

bturner@ubuntu:~/Development/oss/git/git$ git bisect bad
9e468334b41c1d1fc715de177ef1f61a36c1cf01 is the first bad commit
commit 9e468334b41c1d1fc715de177ef1f61a36c1cf01
Author: Karthik Nayak [off-list ref]
Date:   Fri Oct 30 14:15:28 2015 +0530

    ref-filter: fallback on alphabetical comparison

The message for that commit indicates that sorting numerics (which I
assume is the implementation for committerdate) now falls back on
alphabetical for identical values, suggesting this order change is
actually intentional and correct. Is that right?

(Note: The alphabetical-ness of the branch names is reversed, which
seems logical given my original sort was -committerdate. A
--sort=refname looks like this.

refs/heads/!@#$% -> Tue Jan 3 17:00:51 2012 +1100
refs/heads/!@#% -> Tue Jan 3 17:04:06 2012 +1100
refs/heads/% -> Tue Jan 3 17:00:51 2012 +1100
refs/heads/@#$% - >Tue Jan 3 17:00:51 2012 +1100
refs/heads/@#% -> Tue Jan 3 17:00:51 2012 +1100

That's probably more correct too.)

Best regards,
Bryan Turner

Re: for-each-ref output order change in 2.7.0

From: Matthieu Moy <hidden>
Date: 2016-06-15 23:07:40

Hi,

Cc-ing Karthik to draw his attention on the message.

----- Original Message -----
In one of our tests, we have a set of branches whose names are all
special characters (%, @, etc). Most of them branches have identical
tip commits and just have different names. In 2.7.0, when ordering by
-committerdate, the branches are now returned in a different order. I
don't think this is a bug, based on the commit it bisects to, but I'm
wondering if someone can confirm.

2.6.5 and prior (tested all the way back to 1.7.6, so this was
consistent for a long time):

refs/heads/!@#% -> Tue Jan 3 17:04:06 2012 +1100
refs/heads/!@#$% -> Tue Jan 3 17:00:51 2012 +1100
refs/heads/% -> Tue Jan 3 17:00:51 2012 +1100
refs/heads/@#$% -> Tue Jan 3 17:00:51 2012 +1100
refs/heads/@#% -> Tue Jan 3 17:00:51 2012 +1100

2.7.0:

refs/heads/!@#% -> Tue Jan 3 17:04:06 2012 +1100
refs/heads/@#% -> Tue Jan 3 17:00:51 2012 +1100
refs/heads/@#$% -> Tue Jan 3 17:00:51 2012 +1100
refs/heads/% -> Tue Jan 3 17:00:51 2012 +1100
refs/heads/!@#$% -> Tue Jan 3 17:00:51 2012 +1100

I've bisected this back to:

bturner@ubuntu:~/Development/oss/git/git$ git bisect bad
9e468334b41c1d1fc715de177ef1f61a36c1cf01 is the first bad commit
commit 9e468334b41c1d1fc715de177ef1f61a36c1cf01
Author: Karthik Nayak [off-list ref]
Date:   Fri Oct 30 14:15:28 2015 +0530

    ref-filter: fallback on alphabetical comparison

The message for that commit indicates that sorting numerics (which I
assume is the implementation for committerdate) now falls back on
alphabetical for identical values, suggesting this order change is
actually intentional and correct.
And also that the previous order was arbitrary (just letting the sort
algorithm chose which one to put first in case of equality on the main
sorting criterion), so the fact that it was stable previously is more
or less just luck. Now it should be stable.
Is that right?

(Note: The alphabetical-ness of the branch names is reversed, which
seems logical given my original sort was -committerdate. A
--sort=refname looks like this.

refs/heads/!@#$% -> Tue Jan 3 17:00:51 2012 +1100
refs/heads/!@#% -> Tue Jan 3 17:04:06 2012 +1100
refs/heads/% -> Tue Jan 3 17:00:51 2012 +1100
refs/heads/@#$% - >Tue Jan 3 17:00:51 2012 +1100
refs/heads/@#% -> Tue Jan 3 17:00:51 2012 +1100

That's probably more correct too.)

Best regards,
Bryan Turner
-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: for-each-ref output order change in 2.7.0

From: Karthik Nayak <hidden>
Date: 2016-06-15 23:07:40

Hello,

On Sat, Jan 9, 2016 at 9:10 PM, Matthieu Moy
[off-list ref] wrote:
Hi,

Cc-ing Karthik to draw his attention on the message.
Thanks, I didn't really notice it.
----- Original Message -----
quoted
In one of our tests, we have a set of branches whose names are all
special characters (%, @, etc). Most of them branches have identical
tip commits and just have different names. In 2.7.0, when ordering by
-committerdate, the branches are now returned in a different order. I
don't think this is a bug, based on the commit it bisects to, but I'm
wondering if someone can confirm.

2.6.5 and prior (tested all the way back to 1.7.6, so this was
consistent for a long time):

refs/heads/!@#% -> Tue Jan 3 17:04:06 2012 +1100
refs/heads/!@#$% -> Tue Jan 3 17:00:51 2012 +1100
refs/heads/% -> Tue Jan 3 17:00:51 2012 +1100
refs/heads/@#$% -> Tue Jan 3 17:00:51 2012 +1100
refs/heads/@#% -> Tue Jan 3 17:00:51 2012 +1100

2.7.0:

refs/heads/!@#% -> Tue Jan 3 17:04:06 2012 +1100
refs/heads/@#% -> Tue Jan 3 17:00:51 2012 +1100
refs/heads/@#$% -> Tue Jan 3 17:00:51 2012 +1100
refs/heads/% -> Tue Jan 3 17:00:51 2012 +1100
refs/heads/!@#$% -> Tue Jan 3 17:00:51 2012 +1100

I've bisected this back to:

bturner@ubuntu:~/Development/oss/git/git$ git bisect bad
9e468334b41c1d1fc715de177ef1f61a36c1cf01 is the first bad commit
commit 9e468334b41c1d1fc715de177ef1f61a36c1cf01
Author: Karthik Nayak [off-list ref]
Date:   Fri Oct 30 14:15:28 2015 +0530

    ref-filter: fallback on alphabetical comparison

The message for that commit indicates that sorting numerics (which I
assume is the implementation for committerdate) now falls back on
alphabetical for identical values, suggesting this order change is
actually intentional and correct.
And also that the previous order was arbitrary (just letting the sort
algorithm chose which one to put first in case of equality on the main
sorting criterion), so the fact that it was stable previously is more
or less just luck. Now it should be stable.
quoted
Is that right?
Yup, absolutely.
quoted
(Note: The alphabetical-ness of the branch names is reversed, which
seems logical given my original sort was -committerdate. A
--sort=refname looks like this.

refs/heads/!@#$% -> Tue Jan 3 17:00:51 2012 +1100
refs/heads/!@#% -> Tue Jan 3 17:04:06 2012 +1100
refs/heads/% -> Tue Jan 3 17:00:51 2012 +1100
refs/heads/@#$% - >Tue Jan 3 17:00:51 2012 +1100
refs/heads/@#% -> Tue Jan 3 17:00:51 2012 +1100

That's probably more correct too.)

Best regards,
Bryan Turner
This is correct as per the patch, But I'm wondering if this is desired.
I.E when sorting in reverse order should the fallback (alphabetical sort)
also be in reverse order?

-- 
Regards,
Karthik Nayak

Re: for-each-ref output order change in 2.7.0

From: Johannes Sixt <hidden>
Date: 2016-06-15 23:07:40

Am 09.01.2016 um 18:21 schrieb Karthik Nayak:
quoted
quoted
(Note: The alphabetical-ness of the branch names is reversed, which
seems logical given my original sort was -committerdate. A
--sort=refname looks like this.

refs/heads/!@#$% -> Tue Jan 3 17:00:51 2012 +1100
refs/heads/!@#% -> Tue Jan 3 17:04:06 2012 +1100
refs/heads/% -> Tue Jan 3 17:00:51 2012 +1100
refs/heads/@#$% - >Tue Jan 3 17:00:51 2012 +1100
refs/heads/@#% -> Tue Jan 3 17:00:51 2012 +1100

That's probably more correct too.)

Best regards,
Bryan Turner
This is correct as per the patch, But I'm wondering if this is desired.
I.E when sorting in reverse order should the fallback (alphabetical sort)
also be in reverse order?
IMO, the fallback sorting should be in reverse order only when the user 
explicitley asked for reverse order. Just because committer date implies 
some "reverse" ordering should not imply that refs with the same 
committer date should also be listed in reverse alphabetical order.

-- Hannes

Re: for-each-ref output order change in 2.7.0

From: Karthik Nayak <hidden>
Date: 2016-06-15 23:07:40

On Sat, Jan 9, 2016 at 11:30 PM, Johannes Sixt [off-list ref] wrote:
Am 09.01.2016 um 18:21 schrieb Karthik Nayak:
quoted
quoted
quoted
(Note: The alphabetical-ness of the branch names is reversed, which
seems logical given my original sort was -committerdate. A
--sort=refname looks like this.

refs/heads/!@#$% -> Tue Jan 3 17:00:51 2012 +1100
refs/heads/!@#% -> Tue Jan 3 17:04:06 2012 +1100
refs/heads/% -> Tue Jan 3 17:00:51 2012 +1100
refs/heads/@#$% - >Tue Jan 3 17:00:51 2012 +1100
refs/heads/@#% -> Tue Jan 3 17:00:51 2012 +1100

That's probably more correct too.)

Best regards,
Bryan Turner

This is correct as per the patch, But I'm wondering if this is desired.
I.E when sorting in reverse order should the fallback (alphabetical sort)
also be in reverse order?

IMO, the fallback sorting should be in reverse order only when the user
explicitley asked for reverse order. Just because committer date implies
some "reverse" ordering should not imply that refs with the same committer
date should also be listed in reverse alphabetical order.

-- Hannes
I was thinking along the same lines. But do we want to expose the fallback to
the user (i.e let them choose if its reversible or not)? If not Its only a small
change required:
diff --git a/ref-filter.c b/ref-filter.c
index cc850b0..59d43d7 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1554,7 +1554,7 @@ static int cmp_ref_sorting(struct ref_sorting
*s, struct ref_array_item *a, stru
                if (va->ul < vb->ul)
                        cmp = -1;
                else if (va->ul == vb->ul)
-                       cmp = strcmp(a->refname, b->refname);
+                       return strcmp(a->refname, b->refname);
                else
                        cmp = 1;
        }

I could send a patch, as soon as we decide if we want to stick something
simple like this or expose the fallback sort to the user.

-- 
Regards,
Karthik Nayak

Re: for-each-ref output order change in 2.7.0

From: Johannes Sixt <hidden>
Date: 2016-06-15 23:07:40

Am 09.01.2016 um 22:29 schrieb Karthik Nayak:
On Sat, Jan 9, 2016 at 11:30 PM, Johannes Sixt [off-list ref] wrote:
quoted
Am 09.01.2016 um 18:21 schrieb Karthik Nayak:
quoted
quoted
quoted
(Note: The alphabetical-ness of the branch names is reversed, which
seems logical given my original sort was -committerdate. A
--sort=refname looks like this.
After reading up on branch sorting, I notice that the single dash in 
front of committerdate is not a typo, but a request to sort in reverse. 
Therefore, the resulting sort order, which was

refs/heads/!@#% -> Tue Jan 3 17:04:06 2012 +1100
refs/heads/@#% -> Tue Jan 3 17:00:51 2012 +1100
refs/heads/@#$% -> Tue Jan 3 17:00:51 2012 +1100
refs/heads/% -> Tue Jan 3 17:00:51 2012 +1100
refs/heads/!@#$% -> Tue Jan 3 17:00:51 2012 +1100

is totally correct.
quoted
quoted
quoted
quoted
refs/heads/!@#$% -> Tue Jan 3 17:00:51 2012 +1100
refs/heads/!@#% -> Tue Jan 3 17:04:06 2012 +1100
refs/heads/% -> Tue Jan 3 17:00:51 2012 +1100
refs/heads/@#$% - >Tue Jan 3 17:00:51 2012 +1100
refs/heads/@#% -> Tue Jan 3 17:00:51 2012 +1100

That's probably more correct too.)
But I don't know what would be "more" correct here. It's simply correct.
quoted
quoted
This is correct as per the patch, But I'm wondering if this is desired.
I.E when sorting in reverse order should the fallback (alphabetical sort)
also be in reverse order?

IMO, the fallback sorting should be in reverse order only when the user
explicitley asked for reverse order. Just because committer date implies
some "reverse" ordering should not imply that refs with the same committer
date should also be listed in reverse alphabetical order.
I was wrong here. Sorting by committerdate does not imply reverse-ness. 
I just did not know enough about the --sort options when I wrote this 
paragraph.
I was thinking along the same lines. But do we want to expose the fallback to
the user (i.e let them choose if its reversible or not)?
No, we do not want to expose the fallback to the user. And as far as I 
can see, no further change is required.

-- Hannes
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help