t3203: --sort=objectsize failure on Windows

t3203: --sort=objectsize failure on Windows

[Johannes Sixt]

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

Re: t3203: --sort=objectsize failure on Windows

[Karthik Nayak]

On Sat, Oct 24, 2015 at 1:19 PM, Johannes Sixt <j6t@kdbg.org> 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

[PATCH] ref-filter: fallback on alphabetical comparison

[Karthik Nayak]

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 <Karthik.188@gmail.com>
---
 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

Re: [PATCH] ref-filter: fallback on alphabetical comparison

[Eric Sunshine]

On Sat, Oct 24, 2015 at 10:42 AM, Karthik Nayak <karthik.188@gmail.com> 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 <Karthik.188@gmail.com>

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

Re: [PATCH] ref-filter: fallback on alphabetical comparison

[Karthik Nayak]

On Sun, Oct 25, 2015 at 5:48 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Sat, Oct 24, 2015 at 10:42 AM, Karthik Nayak <karthik.188@gmail.com> 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 <Karthik.188@gmail.com>
>
> It would not be amiss to add a Reported-by: to credit j6t.
>

Definitely. Thanks for the quick review :)

-- 
Regards,
Karthik Nayak