# t3203: --sort=objectsize failure on Windows 5 messages from 2015-10-24 to 2015-10-25. Participants: Johannes Sixt, Karthik Nayak, Eric Sunshine. Thread: https://gitlist.dev/t/40627 ## Johannes Sixt, 2015-10-24 07:49 Subject: t3203: --sort=objectsize failure on Windows Message-ID: <562B3807.2030203@kdbg.org> URL: https://gitlist.dev/e/562B3807.2030203%40kdbg.org ``` On Windows, I observe a failure of the test case 'git branch `--sort` option' introduced by aedcb7dc (branch.c: use 'ref-filter' APIs). The reason is that the resulting order generated by qsort is unspecified for entries that compare equal. In fact, the test case sorts 4 entries where there are only 2 different values. To achieve a deterministic order, perhaps cmp_ref_sorting() should fall back to alphabetic comparison if the other criterion (when used) compares equal? -- Hannes ``` ## Karthik Nayak, 2015-10-24 14:25 Subject: Re: t3203: --sort=objectsize failure on Windows Message-ID: URL: https://gitlist.dev/e/CAOLa%3DZRB6_jyyg-M7h8EMtNt96VhXF2hvvKp9OGMvXyJEZB1mw%40mail.gmail.com In-Reply-To: <562B3807.2030203@kdbg.org> ``` On Sat, Oct 24, 2015 at 1:19 PM, Johannes Sixt wrote: > On Windows, I observe a failure of the test case 'git branch `--sort` > option' introduced by aedcb7dc (branch.c: use 'ref-filter' APIs). The reason > is that the resulting order generated by qsort is unspecified for entries > that compare equal. In fact, the test case sorts 4 entries where there are > only 2 different values. Thanks for reporting the error. Although I couldn't reproduce this on a Linux environment. I get what the problem is. > > To achieve a deterministic order, perhaps cmp_ref_sorting() should fall back > to alphabetic comparison if the other criterion (when used) compares equal? > Seems like a simple and good way to go around this. will reply with a patch. -- Regards, Karthik Nayak ``` ## Karthik Nayak, 2015-10-24 14:42 Subject: [PATCH] ref-filter: fallback on alphabetical comparison Message-ID: <1445697770-29331-1-git-send-email-Karthik.188@gmail.com> URL: https://gitlist.dev/e/1445697770-29331-1-git-send-email-Karthik.188%40gmail.com In-Reply-To: <562B3807.2030203@kdbg.org> ``` In ref-filter.c the comparison of refs while sorting is handled by cmp_ref_sorting() function. When sorting as per numerical values (e.g. --sort=objectsize) there is no fallback comparison when both refs hold the same value. This can cause unexpected results as pointed out by Johannes Sixt ($gmane/280117). Hence, fallback to alphabetical comparison based on the refname whenever the other criterion is equal. Fix the test in t3203 in this regard. Signed-off-by: Karthik Nayak --- ref-filter.c | 2 +- t/t3203-branch-output.sh | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ref-filter.c b/ref-filter.c index 046e73b..7b33cb8 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -1698,7 +1698,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 = 0; + cmp = strcmp(a->refname, b->refname); else cmp = 1; } diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh index f77971c..9f2d482 100755 --- a/t/t3203-branch-output.sh +++ b/t/t3203-branch-output.sh @@ -158,8 +158,8 @@ EOF test_expect_success 'git branch `--sort` option' ' cat >expect <<-\EOF && - branch-two * (HEAD detached from fromtag) + branch-two branch-one master EOF -- 2.6.2 ``` ## Eric Sunshine, 2015-10-25 00:18 Subject: Re: [PATCH] ref-filter: fallback on alphabetical comparison Message-ID: URL: https://gitlist.dev/e/CAPig%2BcRnDNFCQs6qspKDEz6C0Rum8W0tSMxejw%2BWOBD%3DXHrDxw%40mail.gmail.com In-Reply-To: <1445697770-29331-1-git-send-email-Karthik.188@gmail.com> ``` On Sat, Oct 24, 2015 at 10:42 AM, Karthik Nayak wrote: > In ref-filter.c the comparison of refs while sorting is handled by > cmp_ref_sorting() function. When sorting as per numerical values > (e.g. --sort=objectsize) there is no fallback comparison when both refs > hold the same value. This can cause unexpected results as pointed out > by Johannes Sixt ($gmane/280117). Please make the commit message self-contained by describing the "unexpected results" here rather than directing readers to chase down the information elsewhere themselves. > Hence, fallback to alphabetical comparison based on the refname whenever > the other criterion is equal. Fix the test in t3203 in this regard. > > Signed-off-by: Karthik Nayak It would not be amiss to add a Reported-by: to credit j6t. > --- > diff --git a/ref-filter.c b/ref-filter.c > index 046e73b..7b33cb8 100644 > --- a/ref-filter.c > +++ b/ref-filter.c > @@ -1698,7 +1698,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 = 0; > + cmp = strcmp(a->refname, b->refname); > else > cmp = 1; > } > diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh > index f77971c..9f2d482 100755 > --- a/t/t3203-branch-output.sh > +++ b/t/t3203-branch-output.sh > @@ -158,8 +158,8 @@ EOF > > test_expect_success 'git branch `--sort` option' ' > cat >expect <<-\EOF && > - branch-two > * (HEAD detached from fromtag) > + branch-two > branch-one > master > EOF > -- > 2.6.2 ``` ## Karthik Nayak, 2015-10-25 07:15 Subject: Re: [PATCH] ref-filter: fallback on alphabetical comparison Message-ID: URL: https://gitlist.dev/e/CAOLa%3DZSS3QK8wkKUGCtLqhoZATCxJxpYBvPCUf37KHVwZeRsOg%40mail.gmail.com In-Reply-To: ``` On Sun, Oct 25, 2015 at 5:48 AM, Eric Sunshine wrote: > On Sat, Oct 24, 2015 at 10:42 AM, Karthik Nayak wrote: >> In ref-filter.c the comparison of refs while sorting is handled by >> cmp_ref_sorting() function. When sorting as per numerical values >> (e.g. --sort=objectsize) there is no fallback comparison when both refs >> hold the same value. This can cause unexpected results as pointed out >> by Johannes Sixt ($gmane/280117). > > Please make the commit message self-contained by describing the > "unexpected results" here rather than directing readers to chase down > the information elsewhere themselves. > Will do. >> Hence, fallback to alphabetical comparison based on the refname whenever >> the other criterion is equal. Fix the test in t3203 in this regard. >> >> Signed-off-by: Karthik Nayak > > It would not be amiss to add a Reported-by: to credit j6t. > Definitely. Thanks for the quick review :) -- Regards, Karthik Nayak ```