{"thread":{"id":"40657","subject":"[PATCH v3] ref-filter: fallback on alphabetical comparison","startedAt":"2015-10-27T19:00:56Z","lastAt":"2015-10-29T14:17:35Z","messageCount":5,"participants":["Karthik Nayak","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"272334","messageId":"1445972456-5621-1-git-send-email-Karthik.188@gmail.com","threadId":"40657","inReplyTo":null,"subject":"[PATCH v3] ref-filter: fallback on alphabetical comparison","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-10-27T19:00:56Z","receivedAt":"2015-10-27T19:00:56Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"In ref-filter.c the comparison of refs while sorting is handled by\ncmp_ref_sorting() function. When sorting as per numerical values\n(e.g. --sort=objectsize) there is no fallback comparison when both\nrefs hold the same value. This can cause unexpected results (i.e. the\norder of listing refs with equal values cannot be pre-determined) as\npointed out by Johannes Sixt ($gmane/280117).\n\nHence, fallback to alphabetical comparison based on the refname\nwhenever the other criterion is equal. Fix the test in t3203 in this\nregard.\n\nReported-by: Johannes Sixt \nSigned-off-by: Karthik Nayak \n---\n ref-filter.c | 2 +-\n t/t3203-branch-output.sh | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 046e73b..7b33cb8 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1698,7 +1698,7 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru\n \t\tif (va->ul < vb->ul)\n \t\t\tcmp = -1;\n \t\telse if (va->ul == vb->ul)\n-\t\t\tcmp = 0;\n+\t\t\tcmp = strcmp(a->refname, b->refname);\n \t\telse\n \t\t\tcmp = 1;\n \t}\ndiff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh\nindex f77971c..9f2d482 100755\n--- a/t/t3203-branch-output.sh\n+++ b/t/t3203-branch-output.sh\n@@ -158,8 +158,8 @@ EOF\n \n test_expect_success 'git branch `--sort` option' '\n \tcat >expect <<-\\EOF &&\n-\t branch-two\n \t* (HEAD detached from fromtag)\n+\t branch-two\n \t branch-one\n \t master\n \tEOF\n-- \n2.6.2\n"},{"id":"272338","messageId":"xmqq7fm8ozu9.fsf@gitster.mtv.corp.google.com","threadId":"40657","inReplyTo":"1445972456-5621-1-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v3] ref-filter: fallback on alphabetical comparison","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-27T19:50:38Z","receivedAt":"2015-10-27T19:50:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> In ref-filter.c the comparison of refs while sorting is handled by\n> cmp_ref_sorting() function. When sorting as per numerical values\n> (e.g. --sort=objectsize) there is no fallback comparison when both\n> refs hold the same value. This can cause unexpected results (i.e. the\n> order of listing refs with equal values cannot be pre-determined) as\n> pointed out by Johannes Sixt ($gmane/280117).\n\nAs we use qsort(), if two members compare as equal, their order in\nthe sorted array is undefined. I think this is a good change.\n\n> Hence, fallback to alphabetical comparison based on the refname\n> whenever the other criterion is equal. Fix the test in t3203 in this\n> regard.\n\nIt is unclear what \"in this regard\" is. Do you mean this (I am not\nsuggesting you to spell these out in a very detailed way in the\nfinal log message; I am deliberately being detailed here to help me\nunderstand what you really mean)?\n\n A test in t3203 was expecting that branch-two sorts before HEAD,\n which happened to be how qsort(3) on Linux sorted the array, but\n (1) that outcome was not even guaranteed, and (2) once we start\n breaking ties with the refname, \"HEAD\" should sort before\n \"branch-two\" so the original expectation was inconsistent with\n the criterion we now use.\n\n Update it to match the new world order, which we can now depend\n on being stable.\n\nI am not sure about \"HEAD\" and \"branch-two\" in the above (it may be\ncomparison between \"HEAD\" and \"refs/heads/branch-two\", for example).\n\n> Reported-by: Johannes Sixt \n> Signed-off-by: Karthik Nayak \n> ---\n> ref-filter.c | 2 +-\n> t/t3203-branch-output.sh | 2 +-\n> 2 files changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/ref-filter.c b/ref-filter.c\n> index 046e73b..7b33cb8 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -1698,7 +1698,7 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru\n> \t\tif (va->ul < vb->ul)\n> \t\t\tcmp = -1;\n> \t\telse if (va->ul == vb->ul)\n> -\t\t\tcmp = 0;\n> +\t\t\tcmp = strcmp(a->refname, b->refname);\n> \t\telse\n> \t\t\tcmp = 1;\n> \t}\n> diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh\n> index f77971c..9f2d482 100755\n> --- a/t/t3203-branch-output.sh\n> +++ b/t/t3203-branch-output.sh\n> @@ -158,8 +158,8 @@ EOF\n> \n> test_expect_success 'git branch `--sort` option' '\n> \tcat >expect <<-\\EOF &&\n> -\t branch-two\n> \t* (HEAD detached from fromtag)\n> +\t branch-two\n> \t branch-one\n> \t master\n> \tEOF\n"},{"id":"272431","messageId":"CAOLa=ZSF5bJocJkMG70x0kTaU139pj0-ONypC=V2Tdnu3yQtow@mail.gmail.com","threadId":"40657","inReplyTo":"xmqq7fm8ozu9.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3] ref-filter: fallback on alphabetical comparison","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-10-28T18:55:27Z","receivedAt":"2015-10-28T18:55:27Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Wed, Oct 28, 2015 at 1:20 AM, Junio C Hamano wrote:\n>> Hence, fallback to alphabetical comparison based on the refname\n>> whenever the other criterion is equal. Fix the test in t3203 in this\n>> regard.\n>\n> It is unclear what \"in this regard\" is. Do you mean this (I am not\n> suggesting you to spell these out in a very detailed way in the\n> final log message; I am deliberately being detailed here to help me\n> understand what you really mean)?\n>\n> A test in t3203 was expecting that branch-two sorts before HEAD,\n> which happened to be how qsort(3) on Linux sorted the array, but\n> (1) that outcome was not even guaranteed, and (2) once we start\n> breaking ties with the refname, \"HEAD\" should sort before\n> \"branch-two\" so the original expectation was inconsistent with\n> the criterion we now use.\n>\n\nExactly what you're saying, they happened to have the same objectsize.\nHence sorting them would put them together, but since we compare the\nrefname's the \"HEAD\" ref would come before \"branch-two\".\n\n> Update it to match the new world order, which we can now depend\n> on being stable.\n>\n> I am not sure about \"HEAD\" and \"branch-two\" in the above (it may be\n> comparison between \"HEAD\" and \"refs/heads/branch-two\", for example).\n>\n\nIt actually is, we consider \"refs/heads/branch-two rather then the shortened\nversion of this. It makes sense to classify refs this way, even though this\nwas a side effect of this commit.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"272439","messageId":"xmqqy4emskod.fsf@gitster.mtv.corp.google.com","threadId":"40657","inReplyTo":"CAOLa=ZSF5bJocJkMG70x0kTaU139pj0-ONypC=V2Tdnu3yQtow@mail.gmail.com","subject":"Re: [PATCH v3] ref-filter: fallback on alphabetical comparison","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-28T22:16:50Z","receivedAt":"2015-10-28T22:16:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> On Wed, Oct 28, 2015 at 1:20 AM, Junio C Hamano wrote:\n>>> Hence, fallback to alphabetical comparison based on the refname\n>>> whenever the other criterion is equal. Fix the test in t3203 in this\n>>> regard.\n>>\n>> It is unclear what \"in this regard\" is. Do you mean this (I am not\n>> suggesting you to spell these out in a very detailed way in the\n>> final log message; I am deliberately being detailed here to help me\n>> understand what you really mean)?\n>>\n>> A test in t3203 was expecting that branch-two sorts before HEAD,\n>> which happened to be how qsort(3) on Linux sorted the array, but\n>> (1) that outcome was not even guaranteed, and (2) once we start\n>> breaking ties with the refname, \"HEAD\" should sort before\n>> \"branch-two\" so the original expectation was inconsistent with\n>> the criterion we now use.\n>>\n>\n> Exactly what you're saying, they happened to have the same objectsize.\n> Hence sorting them would put them together, but since we compare the\n> refname's the \"HEAD\" ref would come before \"branch-two\".\n>\n>> Update it to match the new world order, which we can now depend\n>> on being stable.\n>>\n>> I am not sure about \"HEAD\" and \"branch-two\" in the above (it may be\n>> comparison between \"HEAD\" and \"refs/heads/branch-two\", for example).\n>\n> It actually is, we consider \"refs/heads/branch-two rather then the shortened\n> version of this. It makes sense to classify refs this way, even though this\n> was a side effect of this commit.\n\nNow these are enough bits of info, that can and needs to be\ncondenced into an updated log message to help future readers.\n\nThanks.\n"},{"id":"272479","messageId":"CAOLa=ZTKQb2RyBKN8+xmqvOhDwbfRjDKpNthdLT-wgCtGcB8-g@mail.gmail.com","threadId":"40657","inReplyTo":"xmqqy4emskod.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3] ref-filter: fallback on alphabetical comparison","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-10-29T14:17:35Z","receivedAt":"2015-10-29T14:17:35Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Thu, Oct 29, 2015 at 3:46 AM, Junio C Hamano wrote:\n> Karthik Nayak writes:\n>\n>> On Wed, Oct 28, 2015 at 1:20 AM, Junio C Hamano wrote:\n>>>> Hence, fallback to alphabetical comparison based on the refname\n>>>> whenever the other criterion is equal. Fix the test in t3203 in this\n>>>> regard.\n>>>\n>>> It is unclear what \"in this regard\" is. Do you mean this (I am not\n>>> suggesting you to spell these out in a very detailed way in the\n>>> final log message; I am deliberately being detailed here to help me\n>>> understand what you really mean)?\n>>>\n>>> A test in t3203 was expecting that branch-two sorts before HEAD,\n>>> which happened to be how qsort(3) on Linux sorted the array, but\n>>> (1) that outcome was not even guaranteed, and (2) once we start\n>>> breaking ties with the refname, \"HEAD\" should sort before\n>>> \"branch-two\" so the original expectation was inconsistent with\n>>> the criterion we now use.\n>>>\n>>\n>> Exactly what you're saying, they happened to have the same objectsize.\n>> Hence sorting them would put them together, but since we compare the\n>> refname's the \"HEAD\" ref would come before \"branch-two\".\n>>\n>>> Update it to match the new world order, which we can now depend\n>>> on being stable.\n>>>\n>>> I am not sure about \"HEAD\" and \"branch-two\" in the above (it may be\n>>> comparison between \"HEAD\" and \"refs/heads/branch-two\", for example).\n>>\n>> It actually is, we consider \"refs/heads/branch-two rather then the shortened\n>> version of this. It makes sense to classify refs this way, even though this\n>> was a side effect of this commit.\n>\n> Now these are enough bits of info, that can and needs to be\n> condenced into an updated log message to help future readers.\n>\n> Thanks.\n\nWill update and send, thanks :)\n\n-- \nRegards,\nKarthik Nayak\n"}]}