{"thread":{"id":"32167","subject":"[PATCH] Completion must sort before using uniq","startedAt":"2012-11-22T04:16:09Z","lastAt":"2012-11-25T06:32:52Z","messageCount":8,"participants":["Marc Khouzam","Joachim Schmitz","Felipe Contreras","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"203653","messageId":"CAFj1UpF2wh0imcqW7Ez_J14R_07a_A1-YWESaGrHRNa7Nsv-xg@mail.gmail.com","threadId":"32167","inReplyTo":"1353557598-4820-1-git-send-email-marc.khouzam@gmail.com","subject":"[PATCH] Completion must sort before using uniq","fromName":"Marc Khouzam","fromEmail":"marc.khouzam@gmail.com","sentAt":"2012-11-22T04:16:09Z","receivedAt":"2012-11-22T04:16:09Z","isPatch":true,"sender":{"key":"marc.khouzam@ericsson.com","avatar":"https://gravatar.com/avatar/de564e23ad14e2945f9f1cdb4d0227c935b5b54c39576a304d47caa3e23dcd33?d=mp&s=160"},"body":"The uniq program only works with sorted input. The man page states\n\"uniq prints the unique lines in a sorted file\".\n\nWhen __git_refs use the guess heuristic employed by checkout for\ntracking branches it wants to consider remote branches but only if\nthe branch name is unique. To do that, it calls 'uniq -u'. However\nthe input given to 'uniq -u' is not sorted.\n\nFor example if all available branches are:\n master\n remotes/GitHub/maint\n remotes/GitHub/master\n remotes/origin/maint\n remotes/origin/master\n\nWhen performing completion on 'git checkout ma' the choices given are\n maint\n master\nbut when performing completion on 'git checkout mai', no choices\nappear, which is obviously contradictory.\n\nThe reason is that, when dealing with 'git checkout ma',\n\"__git_refs '' 1\" will find the following list:\n master\n maint\n master\n maint\n master\nwhich, when passed to 'uniq -u' will remain the same.\nBut when dealing with 'git checkout mai', the list will be:\n maint\n maint\nwhich happens to be sorted and will be emptied by 'uniq -u'.\n\nThe solution is to first call 'sort' and then 'uniq -u'.\n\nSigned-off-by: Marc Khouzam \n---\n\nI ran into this by fluke when testing the tcsh completion.\n\nThanks for considering the fix.\n\nMarc\n\n contrib/completion/git-completion.bash | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/contrib/completion/git-completion.bash\nb/contrib/completion/git-completion.bash\nindex bc0657a..85ae419 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -321,7 +321,7 @@ __git_refs ()\n if [[ \"$ref\" == \"$cur\"* ]]; then\n echo \"$ref\"\n fi\n- done | uniq -u\n+ done | sort | uniq -u\n fi\n return\n fi\n--\n1.8.0.1.g9fe2839\n"},{"id":"203688","messageId":"k8nb0t$2pf$1@ger.gmane.org","threadId":"32167","inReplyTo":"CAFj1UpF2wh0imcqW7Ez_J14R_07a_A1-YWESaGrHRNa7Nsv-xg@mail.gmail.com","subject":"Re: [PATCH] Completion must sort before using uniq","fromName":"Joachim Schmitz","fromEmail":"jojo@schmitz-digital.de","sentAt":"2012-11-23T08:09:55Z","receivedAt":"2012-11-23T08:09:55Z","isPatch":true,"sender":{"key":"jojo@schmitz-digital.de","avatar":"https://avatars.githubusercontent.com/u/1786669?v=4"},"body":"Marc Khouzam wrote:\n> The uniq program only works with sorted input. The man page states\n> \"uniq prints the unique lines in a sorted file\".\n...\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n> @@ -321,7 +321,7 @@ __git_refs ()\n> if [[ \"$ref\" == \"$cur\"* ]]; then\n> echo \"$ref\"\n> fi\n> - done | uniq -u\n> + done | sort | uniq -u\n\nIs 'sort -u' not universally available and sufficient here? It is POSIX at \nleast:\nhttp://pubs.opengroup.org/onlinepubs/9699919799/utilities/sort.html\n\nBye, Jojo \n"},{"id":"203689","messageId":"CAMP44s3qpr11JXi-znddAH2BWYbM_kp+nZnTa8CQgCzrBmfzmA@mail.gmail.com","threadId":"32167","inReplyTo":"CAFj1UpF2wh0imcqW7Ez_J14R_07a_A1-YWESaGrHRNa7Nsv-xg@mail.gmail.com","subject":"Re: [PATCH] Completion must sort before using uniq","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-11-23T08:21:43Z","receivedAt":"2012-11-23T08:21:43Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, Nov 22, 2012 at 5:16 AM, Marc Khouzam wrote:\n> The uniq program only works with sorted input. The man page states\n> \"uniq prints the unique lines in a sorted file\".\n>\n> When __git_refs use the guess heuristic employed by checkout for\n> tracking branches it wants to consider remote branches but only if\n> the branch name is unique. To do that, it calls 'uniq -u'. However\n> the input given to 'uniq -u' is not sorted.\n>\n> For example if all available branches are:\n> master\n> remotes/GitHub/maint\n> remotes/GitHub/master\n> remotes/origin/maint\n> remotes/origin/master\n>\n> When performing completion on 'git checkout ma' the choices given are\n> maint\n> master\n> but when performing completion on 'git checkout mai', no choices\n> appear, which is obviously contradictory.\n>\n> The reason is that, when dealing with 'git checkout ma',\n> \"__git_refs '' 1\" will find the following list:\n> master\n> maint\n> master\n> maint\n> master\n> which, when passed to 'uniq -u' will remain the same.\n> But when dealing with 'git checkout mai', the list will be:\n> maint\n> maint\n> which happens to be sorted and will be emptied by 'uniq -u'.\n>\n> The solution is to first call 'sort' and then 'uniq -u'.\n\nThe solution to what? This seems to be the right thing indeed, but you\ndon't explain what is the actual problem that is being solved. What\ndoes the user experience? What would (s)he experience after the patch?\n\n-- \nFelipe Contreras\n"},{"id":"203695","messageId":"CAFj1UpHAqrNvpF+HAxJUPiWAiHbCn=7r1GDw3iMKy8FDW_-D_A@mail.gmail.com","threadId":"32167","inReplyTo":"CAMP44s3qpr11JXi-znddAH2BWYbM_kp+nZnTa8CQgCzrBmfzmA@mail.gmail.com","subject":"[PATCH v2] Completion must sort before using uniq","fromName":"Marc Khouzam","fromEmail":"marc.khouzam@gmail.com","sentAt":"2012-11-23T11:17:07Z","receivedAt":"2012-11-23T11:17:07Z","isPatch":true,"sender":{"key":"marc.khouzam@ericsson.com","avatar":"https://gravatar.com/avatar/de564e23ad14e2945f9f1cdb4d0227c935b5b54c39576a304d47caa3e23dcd33?d=mp&s=160"},"body":"The user can be presented with invalid completion results\nwhen trying to complete a 'git checkout' command. This can happen\nwhen using a branch name prefix that matches multiple remote branches.\nFor example if available branches are:\n master\n remotes/GitHub/maint\n remotes/GitHub/master\n remotes/origin/maint\n remotes/origin/master\n\nWhen performing completion on 'git checkout ma' the user will be\ngiven the choices:\n maint\n master\nHowever, 'git checkout maint' will fail in this case, although\ncompletion previously said 'maint' was valid.\nFurthermore, when performing completion on 'git checkout mai',\nno choices will be suggested. So, the user is first told that the\nbranch name 'maint' is valid, but when trying to complete 'mai'\ninto 'maint', that completion is no longer valid.\n\nThe completion results should never propose 'maint' as a valid\nbranch name, since 'git checkout' will refuse it.\n\nThe reason for this bug is that the uniq program only\nworks with sorted input. The man page states\n\"uniq prints the unique lines in a sorted file\".\n\nWhen __git_refs uses the guess heuristic employed by checkout for\ntracking branches it wants to consider remote branches but only if\nthe branch name is unique. To do that, it calls 'uniq -u'. However\nthe input given to 'uniq -u' is not sorted.\n\nTherefore, in the above example, when dealing with 'git checkout ma',\n\"__git_refs '' 1\" will find the following list:\n master\n maint\n master\n maint\n master\nwhich, when passed to 'uniq -u' will remain the same. Therefore\n'maint' will be wrongly suggested as a valid option.\nWhen dealing with 'git checkout mai', the list will be:\n maint\n maint\nwhich happens to be sorted and will be emptied by 'uniq -u',\nproperly ignoring 'maint'.\n\nA solution for preventing the completion script from suggesting\nsuch invalid branch names is to first call 'sort' and then 'uniq -u'.\n\nSigned-off-by: Marc Khouzam \n---\n\n>> The solution is to first call 'sort' and then 'uniq -u'.\n>\n> The solution to what? This seems to be the right thing indeed, but you\n> don't explain what is the actual problem that is being solved. What\n> does the user experience? What would (s)he experience after the patch?\n\nI have re-worked the commit message to be more clear about the user\nimpacts.\n\nThanks for the feedback.\n\nMarc\n\n contrib/completion/git-completion.bash | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/contrib/completion/git-completion.bash\nb/contrib/completion/git-completion.bash\nindex bc0657a..85ae419 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -321,7 +321,7 @@ __git_refs ()\n if [[ \"$ref\" == \"$cur\"* ]]; then\n echo \"$ref\"\n fi\n- done | uniq -u\n+ done | sort | uniq -u\n fi\n return\n fi\n-- \n1.8.0.1.g9fe2839\n"},{"id":"203696","messageId":"CAMP44s2bMub6T1YcUfsYWPQFU1bY4iU1WfSf+jFa7jSXAKTNaw@mail.gmail.com","threadId":"32167","inReplyTo":"CAFj1UpHAqrNvpF+HAxJUPiWAiHbCn=7r1GDw3iMKy8FDW_-D_A@mail.gmail.com","subject":"Re: [PATCH v2] Completion must sort before using uniq","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-11-23T11:34:20Z","receivedAt":"2012-11-23T11:34:20Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Hi,\n\nMostly cosmetic suggestions, but it looks OK to me.\n\nOn Fri, Nov 23, 2012 at 12:17 PM, Marc Khouzam wrote:\n> The user can be presented with invalid completion results\n> when trying to complete a 'git checkout' command. This can happen\n> when using a branch name prefix that matches multiple remote branches.\n\nSpace here.\n\n> For example if available branches are:\n\nFor example; <- separation\n\n> master\n> remotes/GitHub/maint\n> remotes/GitHub/master\n> remotes/origin/maint\n> remotes/origin/master\n>\n> When performing completion on 'git checkout ma' the user will be\n> given the choices:\n> maint\n> master\n\nSpace.\n\n> However, 'git checkout maint' will fail in this case, although\n> completion previously said 'maint' was valid.\n\nSpace (or continue paragraph).\n\n> Furthermore, when performing completion on 'git checkout mai',\n> no choices will be suggested. So, the user is first told that the\n> branch name 'maint' is valid, but when trying to complete 'mai'\n> into 'maint', that completion is no longer valid.\n>\n> The completion results should never propose 'maint' as a valid\n> branch name, since 'git checkout' will refuse it.\n\nWith this explanation the patch looks good to me.\n\n> The reason for this bug is that the uniq program only\n> works with sorted input. The man page states\n> \"uniq prints the unique lines in a sorted file\".\n>\n> When __git_refs uses the guess heuristic employed by checkout for\n> tracking branches it wants to consider remote branches but only if\n> the branch name is unique. To do that, it calls 'uniq -u'. However\n> the input given to 'uniq -u' is not sorted.\n>\n> Therefore, in the above example, when dealing with 'git checkout ma',\n> \"__git_refs '' 1\" will find the following list:\n> master\n> maint\n> master\n> maint\n> master\n\nSpace.\n\n> which, when passed to 'uniq -u' will remain the same. Therefore\n> 'maint' will be wrongly suggested as a valid option.\n\nSpace.\n\n> When dealing with 'git checkout mai', the list will be:\n> maint\n> maint\n\nSpace.\n\n> which happens to be sorted and will be emptied by 'uniq -u',\n> properly ignoring 'maint'.\n>\n> A solution for preventing the completion script from suggesting\n> such invalid branch names is to first call 'sort' and then 'uniq -u'.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"203701","messageId":"CAFj1UpH8h6c7xHuRG6F+pLy5YMvsJ0QdXsotCpLKnht0PsdiNw@mail.gmail.com","threadId":"32167","inReplyTo":"CAMP44s2bMub6T1YcUfsYWPQFU1bY4iU1WfSf+jFa7jSXAKTNaw@mail.gmail.com","subject":"[PATCH v3] Completion must sort before using uniq","fromName":"Marc Khouzam","fromEmail":"marc.khouzam@gmail.com","sentAt":"2012-11-23T14:02:22Z","receivedAt":"2012-11-23T14:02:22Z","isPatch":true,"sender":{"key":"marc.khouzam@ericsson.com","avatar":"https://gravatar.com/avatar/de564e23ad14e2945f9f1cdb4d0227c935b5b54c39576a304d47caa3e23dcd33?d=mp&s=160"},"body":"The user can be presented with invalid completion results\nwhen trying to complete a 'git checkout' command. This can happen\nwhen using a branch name prefix that matches multiple remote branches.\n\nFor example, if available branches are:\n master\n remotes/GitHub/maint\n remotes/GitHub/master\n remotes/origin/maint\n remotes/origin/master\n\nWhen performing completion on 'git checkout ma' the user will be\ngiven the choices:\n maint\n master\n\nHowever, 'git checkout maint' will fail in this case, although\ncompletion previously said 'maint' was valid. Furthermore, when\nperforming completion on 'git checkout mai', no choices will be\nsuggested. So, the user is first told that the branch name\n'maint' is valid, but when trying to complete 'mai' into 'maint',\nthat completion is no longer valid.\n\nThe completion results should never propose 'maint' as a valid\nbranch name, since 'git checkout' will refuse it.\n\nThe reason for this bug is that the uniq program only\nworks with sorted input. The man page states\n\"uniq prints the unique lines in a sorted file\".\n\nWhen __git_refs uses the guess heuristic employed by checkout for\ntracking branches it wants to consider remote branches but only if\nthe branch name is unique. To do that, it calls 'uniq -u'. However\nthe input given to 'uniq -u' is not sorted.\n\nTherefore, in the above example, when dealing with 'git checkout ma',\n\"__git_refs '' 1\" will find the following list:\n master\n maint\n master\n maint\n master\n\nwhich, when passed to 'uniq -u' will remain the same. Therefore\n'maint' will be wrongly suggested as a valid option.\n\nWhen dealing with 'git checkout mai', the list will be:\n maint\n maint\n\nwhich happens to be sorted and will be emptied by 'uniq -u',\nproperly ignoring 'maint'.\n\nA solution for preventing the completion script from suggesting\nsuch invalid branch names is to first call 'sort' and then 'uniq -u'.\n\nSigned-off-by: Marc Khouzam \n---\n\n> Mostly cosmetic suggestions, but it looks OK to me.\n\nThanks for the suggestions, I updated the commit message.\n\n> With this explanation the patch looks good to me.\n\nThanks for the quick review.\n\nMarc\n\n contrib/completion/git-completion.bash | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/contrib/completion/git-completion.bash\nb/contrib/completion/git-completion.bash\nindex bc0657a..85ae419 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -321,7 +321,7 @@ __git_refs ()\n if [[ \"$ref\" == \"$cur\"* ]]; then\n echo \"$ref\"\n fi\n- done | uniq -u\n+ done | sort | uniq -u\n fi\n return\n fi\n--\n1.8.0.1.g9fe2839\n"},{"id":"203715","messageId":"CAMP44s2ZgbZkjnrNA0ry-koZbTtqRgO5JrJtk-UtDuvdR0k0PA@mail.gmail.com","threadId":"32167","inReplyTo":"CAFj1UpH8h6c7xHuRG6F+pLy5YMvsJ0QdXsotCpLKnht0PsdiNw@mail.gmail.com","subject":"Re: [PATCH v3] Completion must sort before using uniq","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-11-23T19:21:15Z","receivedAt":"2012-11-23T19:21:15Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Fri, Nov 23, 2012 at 3:02 PM, Marc Khouzam wrote:\n> The user can be presented with invalid completion results\n> when trying to complete a 'git checkout' command. This can happen\n> when using a branch name prefix that matches multiple remote branches.\n>\n> For example, if available branches are:\n> master\n> remotes/GitHub/maint\n> remotes/GitHub/master\n> remotes/origin/maint\n> remotes/origin/master\n>\n> When performing completion on 'git checkout ma' the user will be\n> given the choices:\n> maint\n> master\n>\n> However, 'git checkout maint' will fail in this case, although\n> completion previously said 'maint' was valid. Furthermore, when\n> performing completion on 'git checkout mai', no choices will be\n> suggested. So, the user is first told that the branch name\n> 'maint' is valid, but when trying to complete 'mai' into 'maint',\n> that completion is no longer valid.\n>\n> The completion results should never propose 'maint' as a valid\n> branch name, since 'git checkout' will refuse it.\n>\n> The reason for this bug is that the uniq program only\n> works with sorted input. The man page states\n> \"uniq prints the unique lines in a sorted file\".\n>\n> When __git_refs uses the guess heuristic employed by checkout for\n> tracking branches it wants to consider remote branches but only if\n> the branch name is unique. To do that, it calls 'uniq -u'. However\n> the input given to 'uniq -u' is not sorted.\n>\n> Therefore, in the above example, when dealing with 'git checkout ma',\n> \"__git_refs '' 1\" will find the following list:\n> master\n> maint\n> master\n> maint\n> master\n>\n> which, when passed to 'uniq -u' will remain the same. Therefore\n> 'maint' will be wrongly suggested as a valid option.\n>\n> When dealing with 'git checkout mai', the list will be:\n> maint\n> maint\n>\n> which happens to be sorted and will be emptied by 'uniq -u',\n> properly ignoring 'maint'.\n>\n> A solution for preventing the completion script from suggesting\n> such invalid branch names is to first call 'sort' and then 'uniq -u'.\n>\n> Signed-off-by: Marc Khouzam \n\nLooks good. Reviewed-by: Felipe Contreras \n\n-- \nFelipe Contreras\n"},{"id":"203777","messageId":"7vtxsexkdn.fsf@alter.siamese.dyndns.org","threadId":"32167","inReplyTo":"CAFj1UpH8h6c7xHuRG6F+pLy5YMvsJ0QdXsotCpLKnht0PsdiNw@mail.gmail.com","subject":"Re: [PATCH v3] Completion must sort before using uniq","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-11-25T06:32:52Z","receivedAt":"2012-11-25T06:32:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Marc Khouzam writes:\n\n> The user can be presented with invalid completion results\n> when trying to complete a 'git checkout' command. This can happen\n> when using a branch name prefix that matches multiple remote branches.\n>\n> For example, if available branches are:\n> master\n> remotes/GitHub/maint\n> remotes/GitHub/master\n> remotes/origin/maint\n> remotes/origin/master\n>\n> When performing completion on 'git checkout ma' the user will be\n> given the choices:\n> maint\n> master\n> ...\n> When dealing with 'git checkout mai', the list will be:\n> maint\n> maint\n\nWow, the description feels a tad repetitive for a one liner (it\ndescribes \"uniq -u\" without pre-sort is wrong at least three times),\nit would be better than no log message ;-)\n\nI originally thought \"uniq -u\" was a misspelled \"sort -u\" when I\nfirst saw your shorter version, but reading the code to see what is\nfed to the command made it immediately obvious \"sort | uniq -u\" is\nthe right fix. With the above explanation, you do not even need to\nread the code to see what is fed to the command (it is explained ;-).\n\nSo, thanks for the fix.\n"}]}