{"thread":{"id":"32134","subject":"[PATCH 0/7] completion: fix expansion issues with compgen","startedAt":"2012-11-17T11:05:46Z","lastAt":"2012-11-18T09:39:02Z","messageCount":26,"participants":["SZEDER Gábor","Felipe Contreras","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"203403","messageId":"1353150353-29874-1-git-send-email-szeder@ira.uka.de","threadId":"32134","inReplyTo":null,"subject":"[PATCH 0/7] completion: fix expansion issues with compgen","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2012-11-17T11:05:46Z","receivedAt":"2012-11-17T11:05:46Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"This series fixes the expansion issues with compgen reported by Jeroen\nMeijer a while ago in\n\n http://article.gmane.org/gmane.comp.version-control.git/201596\n\nThe problem is that the compgen Bash-builtin performs expansion on all\nwords in the wordlist given to its -W option, breaking Git's completion\nscript when e.g. refs or filenames contain expandable substrings. Since\ncompgen has no option to turn off this expansion and we only use a small\nsubset of compgen's functionality, this series replaces compgen in\n__gitcomp() and __gitcomp_nl() by some shell logic and a small awk script,\nrespectively.\n\nPatches 1 and 2 are test enhancements and fixes, while 3 and 4 add new\ntests to make sure I don't break anything and to demonstrate the issues,\nrespectively. The actual bugfixes are in patches 5 and 6. Finally, patch\n7 is a cleanup made possible by patch 6 (could be squashed into 6).\n\nIn the future we might want/need to fill COMPREPLY directly when\ncompleting a path in the : notation instead using\n__gitcomp_nl() there, because paths can contain newlines, too.\n\nCompared to Felipe's series from yesterday, this series has more tests,\nmore descriptive commit messages, and it's faster. However, it doesn't\ninclude his 1/5 (completion: get rid of empty COMPREPLY assignments).\n\n\nFootnote: check the patchlist below, and notice how format-patch put\npatches 4 and 5 into the same line.\n\nSZEDER Gábor (7):\n completion: make the 'basic' test more tester-friendly\n completion: fix args of run_completion() test helper\n completion: add tests for the __gitcomp_nl() completion helper\n function\n completion: add tests for invalid variable name among completion words completion: fix expansion issues in __gitcomp_nl()\n completion: fix expansion issue in __gitcomp()\n completion: remove the now unused __gitcomp_1() internal helper\n function\n\n contrib/completion/git-completion.bash | 57 ++++++++-------\n t/t9902-completion.sh | 123 ++++++++++++++++++++++++++++++---\n 2 files changed, 147 insertions(+), 33 deletions(-)\n\n-- \n1.8.0.220.g4d14ece\n"},{"id":"203406","messageId":"1353150353-29874-2-git-send-email-szeder@ira.uka.de","threadId":"32134","inReplyTo":"1353150353-29874-1-git-send-email-szeder@ira.uka.de","subject":"[PATCH 1/7] completion: make the 'basic' test more tester-friendly","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2012-11-17T11:05:47Z","receivedAt":"2012-11-17T11:05:47Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The 'basic' test uses 'grep -q' to filter the resulting possible\ncompletion words while looking for the presence or absence of certain\ngit commands, and relies on grep's exit status to indicate a failure.\n\nThis works fine as long as there are no errors. However, in case of a\nfailure it doesn't give any indication whatsoever about the reason of\nthe failure, i.e. which condition failed.\n\nTo make testers' life easier provide some output about the failed\ncondition: store the results of the filtering in a file and compare\nits contents to the expected results by the good old test_cmp helper.\nHowever, to actually get output from test_cmp in case of an error we\nmust make sure that test_cmp is always executed. Since in case of an\nerror grep's exit code aborts the test immediately, before running the\nsubsequent test_cmp, do the filtering using sed instead of grep.\n\nSigned-off-by: SZEDER Gábor \n---\n t/t9902-completion.sh | 16 ++++++++++++----\n 1 file changed, 12 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex 8fa025f9..b56759f7 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -158,14 +158,22 @@ test_expect_success '__gitcomp - suffix' '\n test_expect_success 'basic' '\n \trun_completion \"git \\\"\\\"\" &&\n \t# built-in\n-\tgrep -q \"^add \\$\" out &&\n+\techo \"add \" >expected &&\n+\tsed -n \"/^add \\$/p\" out >out2 &&\n+\ttest_cmp expected out2 &&\n \t# script\n-\tgrep -q \"^filter-branch \\$\" out &&\n+\techo \"filter-branch \" >expected &&\n+\tsed -n \"/^filter-branch \\$/p\" out >out2 &&\n+\ttest_cmp expected out2 &&\n \t# plumbing\n-\t! grep -q \"^ls-files \\$\" out &&\n+\t>expected &&\n+\tsed -n \"/^ls-files \\$/p\" out >out2 &&\n+\ttest_cmp expected out2 &&\n \n \trun_completion \"git f\" &&\n-\t! grep -q -v \"^f\" out\n+\t>expected &&\n+\tsed -n \"/^[^f]/p\" out >out2 &&\n+\ttest_cmp expected out2\n '\n \n test_expect_success 'double dash \"git\" itself' '\n-- \n1.8.0.220.g4d14ece\n"},{"id":"203404","messageId":"1353150353-29874-3-git-send-email-szeder@ira.uka.de","threadId":"32134","inReplyTo":"1353150353-29874-1-git-send-email-szeder@ira.uka.de","subject":"[PATCH 2/7] completion: fix args of run_completion() test helper","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2012-11-17T11:05:48Z","receivedAt":"2012-11-17T11:05:48Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"To simulate that the user hit 'git , the 'basic' test sets up the\nrather strange command line containing the two words\n\n git \"\"\n\ni.e. the second word on the command line consists of two double\nquotes. This is not what happens for real, however, because after\n'git ' the second word on the command line is just an empty\nstring. Luckily, the test works nevertheless.\n\nFix this by passing the command line to run_completion() as separate\nwords.\n\nSigned-off-by: SZEDER Gábor \n---\n t/t9902-completion.sh | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex b56759f7..3af75872 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -49,7 +49,7 @@ run_completion ()\n {\n \tlocal -a COMPREPLY _words\n \tlocal _cword\n-\t_words=( $1 )\n+\t_words=( \"$@\" )\n \t(( _cword = ${#_words[@]} - 1 ))\n \t__git_wrap__git_main && print_comp\n }\n@@ -57,7 +57,7 @@ run_completion ()\n test_completion ()\n {\n \ttest $# -gt 1 && echo \"$2\" > expected\n-\trun_completion \"$@\" &&\n+\trun_completion $1 &&\n \ttest_cmp expected out\n }\n \n@@ -156,7 +156,7 @@ test_expect_success '__gitcomp - suffix' '\n '\n \n test_expect_success 'basic' '\n-\trun_completion \"git \\\"\\\"\" &&\n+\trun_completion git \"\" &&\n \t# built-in\n \techo \"add \" >expected &&\n \tsed -n \"/^add \\$/p\" out >out2 &&\n@@ -170,7 +170,7 @@ test_expect_success 'basic' '\n \tsed -n \"/^ls-files \\$/p\" out >out2 &&\n \ttest_cmp expected out2 &&\n \n-\trun_completion \"git f\" &&\n+\trun_completion git f &&\n \t>expected &&\n \tsed -n \"/^[^f]/p\" out >out2 &&\n \ttest_cmp expected out2\n-- \n1.8.0.220.g4d14ece\n"},{"id":"203405","messageId":"1353150353-29874-4-git-send-email-szeder@ira.uka.de","threadId":"32134","inReplyTo":"1353150353-29874-1-git-send-email-szeder@ira.uka.de","subject":"[PATCH 3/7] completion: add tests for the __gitcomp_nl() completion helper function","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2012-11-17T11:05:49Z","receivedAt":"2012-11-17T11:05:49Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Test __gitcomp_nl()'s basic functionality, i.e. that trailing space,\nprefix, and suffix are added correctly.\n\nSigned-off-by: SZEDER Gábor \n---\n t/t9902-completion.sh | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 84 insertions(+)\n\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex 3af75872..32b3e8c4 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -155,6 +155,90 @@ test_expect_success '__gitcomp - suffix' '\n \ttest_cmp expected out\n '\n \n+test_expect_success '__gitcomp_nl - trailing space' '\n+\tsed -e \"s/Z$//\" >expected <<-\\EOF &&\n+\tmaint Z\n+\tmaster Z\n+\tEOF\n+\t(\n+\t\tdeclare -a COMPREPLY &&\n+\t\trefs=\"$(cat <<-\\EOF\n+\t\t\tmaint\n+\t\t\tmaster\n+\t\t\tnext\n+\t\t\tpu\n+\t\t\tEOF\n+\t\t)\" &&\n+\t\tcur=\"m\" &&\n+\t\t__gitcomp_nl \"$refs\" &&\n+\t\tprint_comp\n+\t) &&\n+\ttest_cmp expected out\n+'\n+\n+test_expect_success '__gitcomp_nl - prefix' '\n+\tsed -e \"s/Z$//\" >expected <<-\\EOF &&\n+\t--fixup=maint Z\n+\t--fixup=master Z\n+\tEOF\n+\t(\n+\t\tdeclare -a COMPREPLY &&\n+\t\trefs=\"$(cat <<-\\EOF\n+\t\t\tmaint\n+\t\t\tmaster\n+\t\t\tnext\n+\t\t\tpu\n+\t\t\tEOF\n+\t\t)\" &&\n+\t\tcur=\"--fixup=m\" &&\n+\t\t__gitcomp_nl \"$refs\" \"--fixup=\" \"m\" &&\n+\t\tprint_comp\n+\t) &&\n+\ttest_cmp expected out\n+'\n+\n+test_expect_success '__gitcomp_nl - suffix' '\n+\tsed -e \"s/Z$//\" >expected <<-\\EOF &&\n+\tbranch.maint.Z\n+\tbranch.master.Z\n+\tEOF\n+\t(\n+\t\tdeclare -a COMPREPLY &&\n+\t\trefs=\"$(cat <<-\\EOF\n+\t\t\tmaint\n+\t\t\tmaster\n+\t\t\tnext\n+\t\t\tpu\n+\t\t\tEOF\n+\t\t)\" &&\n+\t\tcur=\"branch.ma\" &&\n+\t\t__gitcomp_nl \"$refs\" \"branch.\" \"ma\" \".\" &&\n+\t\tprint_comp\n+\t) &&\n+\ttest_cmp expected out\n+'\n+\n+test_expect_success '__gitcomp_nl - no suffix' '\n+\tsed -e \"s/Z$//\" >expected <<-\\EOF &&\n+\tmaintZ\n+\tmasterZ\n+\tEOF\n+\t(\n+\t\tdeclare -a COMPREPLY &&\n+\t\trefs=\"$(cat <<-\\EOF\n+\t\t\tmaint\n+\t\t\tmaster\n+\t\t\tnext\n+\t\t\tpu\n+\t\t\tEOF\n+\t\t)\" &&\n+\t\tcur=\"ma\" &&\n+\t\t__gitcomp_nl \"$refs\" \"\" \"ma\" \"\" &&\n+\t\tprint_comp\n+\t) &&\n+\ttest_cmp expected out\n+'\n+\n test_expect_success 'basic' '\n \trun_completion git \"\" &&\n \t# built-in\n-- \n1.8.0.220.g4d14ece\n"},{"id":"203407","messageId":"1353150353-29874-5-git-send-email-szeder@ira.uka.de","threadId":"32134","inReplyTo":"1353150353-29874-1-git-send-email-szeder@ira.uka.de","subject":"[PATCH 4/7] completion: add tests for invalid variable name among completion words","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2012-11-17T11:05:50Z","receivedAt":"2012-11-17T11:05:50Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The compgen Bash-builtin performs expansion on all words in the\nwordlist given to its -W option, breaking Git's completion script in\nvarious ways if one of those words can be expanded. The breakage can\nbe simply bogus possible completion words, but it can also be a\nfailure:\n\n $ git branch '${foo.bar}'\n $ git checkout \n bash: ${foo.bar}: bad substitution\n\n${foo.bar} is an invalid variable name, which triggers the failure\nwhen compgen attempts to expand it, completely breaking refs\ncompletion. The same applies to e.g. completing the :\nnotation when a filename contains a similar expandable substring.\n\nSince both __gitcomp() and __gitcomp_nl() rely on compgen, both are\naffected by this issue. So add a simple test for each of them to\ncheck that such a word doesn't cause failures (but don't check the\nresulting possible completion words for now, because that should be\nquoted properly, and that's a separate topic).\n\nReported-by: Jeroen Meijer \nSigned-off-by: SZEDER Gábor \n---\n t/t9902-completion.sh | 13 +++++++++++++\n 1 file changed, 13 insertions(+)\n\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex 32b3e8c4..a108ec1c 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -71,6 +71,7 @@ test_completion_long ()\n }\n \n newline=$'\\n'\n+invalid_variable_name=\"${foo.bar}\"\n \n test_expect_success '__gitcomp - trailing space - options' '\n \tsed -e \"s/Z$//\" >expected <<-\\EOF &&\n@@ -155,6 +156,12 @@ test_expect_success '__gitcomp - suffix' '\n \ttest_cmp expected out\n '\n \n+test_expect_failure '__gitcomp - doesnt fail because of invalid variable name' '\n+\t(\n+\t\t__gitcomp \"$invalid_variable_name\"\n+\t)\n+'\n+\n test_expect_success '__gitcomp_nl - trailing space' '\n \tsed -e \"s/Z$//\" >expected <<-\\EOF &&\n \tmaint Z\n@@ -239,6 +246,12 @@ test_expect_success '__gitcomp_nl - no suffix' '\n \ttest_cmp expected out\n '\n \n+test_expect_failure '__gitcomp_nl - doesnt fail because of invalid variable name' '\n+\t(\n+\t\t__gitcomp_nl \"$invalid_variable_name\"\n+\t)\n+'\n+\n test_expect_success 'basic' '\n \trun_completion git \"\" &&\n \t# built-in\n-- \n1.8.0.220.g4d14ece\n"},{"id":"203410","messageId":"1353150353-29874-6-git-send-email-szeder@ira.uka.de","threadId":"32134","inReplyTo":"1353150353-29874-1-git-send-email-szeder@ira.uka.de","subject":"[PATCH 5/7] completion: fix expansion issues in __gitcomp_nl()","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2012-11-17T11:05:51Z","receivedAt":"2012-11-17T11:05:51Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The compgen Bash-builtin performs expansion on all words in the\nwordlist given to its -W option, breaking Git's completion script when\nrefs or filenames passed to __gitcomp_nl() contain expandable\nsubstrings. At least one user can't use ref completion at all in a\nrepository, which contains tags with metacharacters like\n\n Build%20V%20${bamboo.custom.jiraversion.name}%20Build%20721\n\nSuch a ref causes a failure in compgen as it tries to expand the\nvariable with invalid name.\n\nUnfortunately, compgen has no option to turn off this expansion.\nHowever, in __gitcomp_nl() we use only a small subset of compgen's\nfunctionality, namely the filtering of matching possible completion\nwords and adding a prefix and/or suffix to each of those words. So,\nto avoid compgen's problematic expansion we get rid of compgen, and\nimplement the equivalent functionality in a small awk script. The\nreason for using awk instead of sed is that awk allows plain (i.e.\nnon-regexp) string comparison, making quoting of regexp characters in\nthe current word unnecessary.\n\nNote, that such expandable words need quoting to be of any use on the\ncommand line. This patch doesn't perform that quoting, but it could\nbe implemented later on top by extending the awk script to unquote\n$cur and to quote matching words. Nonetheless, this patch still a\nstep forward, because at least refs can be completed even in the\npresence of one with metacharacters.\n\nAlso update the function's description a bit.\n\nReported-by: Jeroen Meijer \nSigned-off-by: SZEDER Gábor \n---\n\nawk doesn't have a prefixcmp(). I'm no awk wizard, so I'm not sure this\nis the right way to do it.\n\n contrib/completion/git-completion.bash | 17 +++++++++++++----\n t/t9902-completion.sh | 4 ++--\n 2 files changed, 15 insertions(+), 6 deletions(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex bc0657a2..65196ddd 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -249,19 +249,28 @@ __gitcomp ()\n \tesac\n }\n \n-# Generates completion reply with compgen from newline-separated possible\n-# completion words by appending a space to all of them.\n+# Generates completion reply for the word in $cur from newline-separated\n+# possible completion words, appending a space to all of them.\n # It accepts 1 to 4 arguments:\n # 1: List of possible completion words, separated by a single newline.\n # 2: A prefix to be added to each possible completion word (optional).\n-# 3: Generate possible completion matches for this word (optional).\n+# 3: Generate possible completion matches for this word instead of $cur\n+# (optional).\n # 4: A suffix to be appended to each possible completion word instead of\n # the default space (optional). If specified but empty, nothing is\n # appended.\n __gitcomp_nl ()\n {\n \tlocal IFS=$'\\n'\n-\tCOMPREPLY=($(compgen -P \"${2-}\" -S \"${4- }\" -W \"$1\" -- \"${3-$cur}\"))\n+\tCOMPREPLY=($(awk -v pfx=\"${2-}\" -v sfx=\"${4- }\" -v cur=\"${3-$cur}\" '\n+\t\tBEGIN {\n+\t\t\tFS=\"\\n\";\n+\t\t\tlen=length(cur);\n+\t\t}\n+\t\t{\n+\t\t\tif (cur == substr($1, 1, len))\n+\t\t\t\tprint pfx$1sfx;\n+\t\t}' <<< \"$1\" ))\n }\n \n __git_heads ()\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex a108ec1c..fa289324 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -246,7 +246,7 @@ test_expect_success '__gitcomp_nl - no suffix' '\n \ttest_cmp expected out\n '\n \n-test_expect_failure '__gitcomp_nl - doesnt fail because of invalid variable name' '\n+test_expect_success '__gitcomp_nl - doesnt fail because of invalid variable name' '\n \t(\n \t\t__gitcomp_nl \"$invalid_variable_name\"\n \t)\n@@ -383,7 +383,7 @@ test_expect_success 'complete tree filename with spaces' '\n \tEOF\n '\n \n-test_expect_failure 'complete tree filename with metacharacters' '\n+test_expect_success 'complete tree filename with metacharacters' '\n \techo content >\"name with \\${meta}\" &&\n \tgit add . &&\n \tgit commit -m meta &&\n-- \n1.8.0.220.g4d14ece\n"},{"id":"203408","messageId":"1353150353-29874-7-git-send-email-szeder@ira.uka.de","threadId":"32134","inReplyTo":"1353150353-29874-1-git-send-email-szeder@ira.uka.de","subject":"[PATCH 6/7] completion: fix expansion issue in __gitcomp()","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2012-11-17T11:05:52Z","receivedAt":"2012-11-17T11:05:52Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The compgen Bash-builtin performs expansion on all words in the\nwordlist given to its -W option, breaking Git's completion script when\nwords passed to __gitcomp() contain expandable substrings.\n\nIn __gitcomp() we only use a small subset ot compgen's functionality,\nnamely the filtering of matching possible completion words and adding\na prefix to each of those words; suffix is added by the __gitcomp_1()\nhelper function instead. Now, since we already have to iterate over\nall words in the wordlist to append a trailing space if necessary, we\ncan check in the same loop whether a word matches the word to be\ncompleted and store matching words with possible prefix and/or suffix\nin the COMPREPLY array. This way we achieve the same functionality\nwithout the compgen builtin and, more importantly, without it's\nproblematic expansions.\n\nAn additional benefit, especially for Windows/MSysGit users, is the\nloss of two subshells, one to run __gitcomp_1() and the other to run\nthe compgen builtin.\n\nNote, that like the previous patch for __gitcomp_nl(), this patch\ndoesn't quote expandable words either, but that could be implemented\nlater on top by unquoting $cur and then quoting what get stored in\nCOMPREPLY.\n\nAlso update the function's description a bit.\n\nSigned-off-by: SZEDER Gábor \n---\n contrib/completion/git-completion.bash | 27 ++++++++++++++++++++-------\n t/t9902-completion.sh | 2 +-\n 2 files changed, 21 insertions(+), 8 deletions(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 65196ddd..283ef99b 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -225,12 +225,13 @@ _get_comp_words_by_ref ()\n fi\n fi\n \n-# Generates completion reply with compgen, appending a space to possible\n-# completion words, if necessary.\n+# Generates completion reply for the word in $cur, appending a space to\n+# possible completion words, if necessary.\n # It accepts 1 to 4 arguments:\n # 1: List of possible completion words.\n # 2: A prefix to be added to each possible completion word (optional).\n-# 3: Generate possible completion matches for this word (optional).\n+# 3: Generate possible completion matches for this word instead of $cur\n+# (optional).\n # 4: A suffix to be appended to each possible completion word (optional).\n __gitcomp ()\n {\n@@ -241,10 +242,22 @@ __gitcomp ()\n \t\tCOMPREPLY=()\n \t\t;;\n \t*)\n-\t\tlocal IFS=$'\\n'\n-\t\tCOMPREPLY=($(compgen -P \"${2-}\" \\\n-\t\t\t-W \"$(__gitcomp_1 \"${1-}\" \"${4-}\")\" \\\n-\t\t\t-- \"$cur_\"))\n+\t\tlocal i=0 c IFS=$' \\t\\n'\n+\t\tfor c in $1; do\n+\t\t\tcase $c in\n+\t\t\t\"$cur_\"*)\n+\t\t\t\tc=\"$c${4-}\"\n+\t\t\t\tcase $c in\n+\t\t\t\t--*=*|*.) ;;\n+\t\t\t\t*) c=\"$c \" ;;\n+\t\t\t\tesac\n+\t\t\t\tCOMPREPLY[$i]=\"${2-}$c\"\n+\t\t\t\ti=$((++i))\n+\t\t\t\t;;\n+\t\t\t*)\n+\t\t\t\t;;\n+\t\t\tesac\n+\t\tdone\n \t\t;;\n \tesac\n }\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex fa289324..d08f4259 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -156,7 +156,7 @@ test_expect_success '__gitcomp - suffix' '\n \ttest_cmp expected out\n '\n \n-test_expect_failure '__gitcomp - doesnt fail because of invalid variable name' '\n+test_expect_success '__gitcomp - doesnt fail because of invalid variable name' '\n \t(\n \t\t__gitcomp \"$invalid_variable_name\"\n \t)\n-- \n1.8.0.220.g4d14ece\n"},{"id":"203409","messageId":"1353150353-29874-8-git-send-email-szeder@ira.uka.de","threadId":"32134","inReplyTo":"1353150353-29874-1-git-send-email-szeder@ira.uka.de","subject":"[PATCH 7/7] completion: remove the now unused __gitcomp_1() internal helper function","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2012-11-17T11:05:53Z","receivedAt":"2012-11-17T11:05:53Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Since the __gitcomp_1() helper is basically only an implementation\ndetail of __gitcomp() that exists solely for performance reasons (see\nab02dfe5 (bash completion: Improve responsiveness of git-log\ncompletion, 2008-07-13)), it's quite unlikely that anyone uses or ever\nused it besides __gitcomp(). And now __gitcomp() doesn't need it\nanymore either, so delete it.\n\nSigned-off-by: SZEDER Gábor \n---\n contrib/completion/git-completion.bash | 13 -------------\n 1 file changed, 13 deletions(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 283ef99b..a281b28d 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -56,19 +56,6 @@ __gitdir ()\n \tfi\n }\n \n-__gitcomp_1 ()\n-{\n-\tlocal c IFS=$' \\t\\n'\n-\tfor c in $1; do\n-\t\tc=\"$c$2\"\n-\t\tcase $c in\n-\t\t--*=*|*.) ;;\n-\t\t*) c=\"$c \" ;;\n-\t\tesac\n-\t\tprintf '%s\\n' \"$c\"\n-\tdone\n-}\n-\n # The following function is based on code from:\n #\n # bash_completion - programmable completion functions for bash 3.2+\n-- \n1.8.0.220.g4d14ece\n"},{"id":"203412","messageId":"CAMP44s3J3e_bcyoQmcdQno59dPJuJ4=7ej=-eseE5j2tteD=dA@mail.gmail.com","threadId":"32134","inReplyTo":"1353150353-29874-7-git-send-email-szeder@ira.uka.de","subject":"Re: [PATCH 6/7] completion: fix expansion issue in __gitcomp()","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-11-17T11:39:24Z","receivedAt":"2012-11-17T11:39:24Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Nov 17, 2012 at 12:05 PM, SZEDER Gábor wrote:\n\n> -# Generates completion reply with compgen, appending a space to possible\n> -# completion words, if necessary.\n> +# Generates completion reply for the word in $cur, appending a space to\n> +# possible completion words, if necessary.\n> # It accepts 1 to 4 arguments:\n> # 1: List of possible completion words.\n> # 2: A prefix to be added to each possible completion word (optional).\n> -# 3: Generate possible completion matches for this word (optional).\n> +# 3: Generate possible completion matches for this word instead of $cur\n> +# (optional).\n> # 4: A suffix to be appended to each possible completion word (optional).\n> __gitcomp ()\n> {\n> @@ -241,10 +242,22 @@ __gitcomp ()\n> COMPREPLY=()\n> ;;\n> *)\n> - local IFS=$'\\n'\n> - COMPREPLY=($(compgen -P \"${2-}\" \\\n> - -W \"$(__gitcomp_1 \"${1-}\" \"${4-}\")\" \\\n> - -- \"$cur_\"))\n> + local i=0 c IFS=$' \\t\\n'\n> + for c in $1; do\n> + case $c in\n> + \"$cur_\"*)\n> + c=\"$c${4-}\"\n> + case $c in\n> + --*=*|*.) ;;\n> + *) c=\"$c \" ;;\n> + esac\n> + COMPREPLY[$i]=\"${2-}$c\"\n> + i=$((++i))\n> + ;;\n> + *)\n> + ;;\n> + esac\n> + done\n\nThis is not quite the same as before, is it? Before the suffix would\nbe taken into consideration for the comparison with $cur_, but not any\nmore.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"203415","messageId":"CAMP44s1ZgNM1WXPu_-q9aFkz8Ui3czwcUqHWvs7Yspi_p9kuNQ@mail.gmail.com","threadId":"32134","inReplyTo":"1353150353-29874-6-git-send-email-szeder@ira.uka.de","subject":"Re: [PATCH 5/7] completion: fix expansion issues in __gitcomp_nl()","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-11-17T11:50:39Z","receivedAt":"2012-11-17T11:50:39Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Nov 17, 2012 at 12:05 PM, SZEDER Gábor wrote:\n\n> __gitcomp_nl ()\n> {\n> local IFS=$'\\n'\n> - COMPREPLY=($(compgen -P \"${2-}\" -S \"${4- }\" -W \"$1\" -- \"${3-$cur}\"))\n> + COMPREPLY=($(awk -v pfx=\"${2-}\" -v sfx=\"${4- }\" -v cur=\"${3-$cur}\" '\n> + BEGIN {\n> + FS=\"\\n\";\n> + len=length(cur);\n> + }\n> + {\n> + if (cur == substr($1, 1, len))\n> + print pfx$1sfx;\n> + }' <<< \"$1\" ))\n> }\n\nDoes this really perform better than my alternative?\n\n+ for x in $1; do\n+ if [[ \"$x\" = \"$3\"* ]]; then\n+ COMPREPLY+=(\"$2$x$4\")\n+ fi\n+ done\n\n-- \nFelipe Contreras\n"},{"id":"203417","messageId":"20121117140925.GF12052@goldbirke","threadId":"32134","inReplyTo":"CAMP44s3J3e_bcyoQmcdQno59dPJuJ4=7ej=-eseE5j2tteD=dA@mail.gmail.com","subject":"Re: [PATCH 6/7] completion: fix expansion issue in __gitcomp()","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2012-11-17T14:09:25Z","receivedAt":"2012-11-17T14:09:25Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Sat, Nov 17, 2012 at 12:39:24PM +0100, Felipe Contreras wrote:\n> On Sat, Nov 17, 2012 at 12:05 PM, SZEDER Gábor wrote:\n> \n> > -# Generates completion reply with compgen, appending a space to possible\n> > -# completion words, if necessary.\n> > +# Generates completion reply for the word in $cur, appending a space to\n> > +# possible completion words, if necessary.\n> > # It accepts 1 to 4 arguments:\n> > # 1: List of possible completion words.\n> > # 2: A prefix to be added to each possible completion word (optional).\n> > -# 3: Generate possible completion matches for this word (optional).\n> > +# 3: Generate possible completion matches for this word instead of $cur\n> > +# (optional).\n> > # 4: A suffix to be appended to each possible completion word (optional).\n> > __gitcomp ()\n> > {\n> > @@ -241,10 +242,22 @@ __gitcomp ()\n> > COMPREPLY=()\n> > ;;\n> > *)\n> > - local IFS=$'\\n'\n> > - COMPREPLY=($(compgen -P \"${2-}\" \\\n> > - -W \"$(__gitcomp_1 \"${1-}\" \"${4-}\")\" \\\n> > - -- \"$cur_\"))\n> > + local i=0 c IFS=$' \\t\\n'\n> > + for c in $1; do\n> > + case $c in\n> > + \"$cur_\"*)\n> > + c=\"$c${4-}\"\n> > + case $c in\n> > + --*=*|*.) ;;\n> > + *) c=\"$c \" ;;\n> > + esac\n> > + COMPREPLY[$i]=\"${2-}$c\"\n> > + i=$((++i))\n> > + ;;\n> > + *)\n> > + ;;\n> > + esac\n> > + done\n> \n> This is not quite the same as before, is it? Before the suffix would\n> be taken into consideration for the comparison with $cur_, but not any\n> more.\n\nThat's a good catch, thanks.\n\nI remember it puzzled me that the suffix is considered in the\ncomparison (and that a trailing space would be appended even after a\ngiven suffix, too, so there seems to be no way to disable the trailing\nspace). However, currently it doesn't make a difference for us,\nbecause afaics we never specify a suffix for __gitcomp(). There were\ntwo instances in _git_config() where we specified \".\" as suffix, but\nthose two were converted to __gitcomp_nl(). I changed those callsites\nback to __gitcomp() and tried to provoke wrong behavior with the above\npatch, but couldn't.\n\nAnyway, it's better to err on the safe side, so here's the fixup.\n\n-- >8 --\nSubject: [PATCH] fixup! completion: fix expansion issue in __gitcomp()\n\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 b/contrib/completion/git-completion.bash\nindex a1bf732f..29818fb5 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -231,9 +231,9 @@ __gitcomp ()\n \t*)\n \t\tlocal i=0 c IFS=$' \\t\\n'\n \t\tfor c in $1; do\n+\t\t\tc=\"$c${4-}\"\n \t\t\tcase $c in\n \t\t\t\"$cur_\"*)\n-\t\t\t\tc=\"$c${4-}\"\n \t\t\t\tcase $c in\n \t\t\t\t--*=*|*.) ;;\n \t\t\t\t*) c=\"$c \" ;;\n-- \n1.8.0.220.g4d14ece\n"},{"id":"203420","messageId":"20121117141422.GI12052@goldbirke","threadId":"32134","inReplyTo":"CAMP44s1ZgNM1WXPu_-q9aFkz8Ui3czwcUqHWvs7Yspi_p9kuNQ@mail.gmail.com","subject":"Re: [PATCH 5/7] completion: fix expansion issues in __gitcomp_nl()","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2012-11-17T14:14:22Z","receivedAt":"2012-11-17T14:14:22Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Sat, Nov 17, 2012 at 12:50:39PM +0100, Felipe Contreras wrote:\n> On Sat, Nov 17, 2012 at 12:05 PM, SZEDER Gábor wrote:\n> \n> > __gitcomp_nl ()\n> > {\n> > local IFS=$'\\n'\n> > - COMPREPLY=($(compgen -P \"${2-}\" -S \"${4- }\" -W \"$1\" -- \"${3-$cur}\"))\n> > + COMPREPLY=($(awk -v pfx=\"${2-}\" -v sfx=\"${4- }\" -v cur=\"${3-$cur}\" '\n> > + BEGIN {\n> > + FS=\"\\n\";\n> > + len=length(cur);\n> > + }\n> > + {\n> > + if (cur == substr($1, 1, len))\n> > + print pfx$1sfx;\n> > + }' <<< \"$1\" ))\n> > }\n> \n> Does this really perform better than my alternative?\n> \n> + for x in $1; do\n> + if [[ \"$x\" = \"$3\"* ]]; then\n> + COMPREPLY+=(\"$2$x$4\")\n> + fi\n> + done\n\nIt does:\n\n My version:\n\n $ refs=\"$(for i in {0..9999} ; do echo branch$i ; done)\"\n $ time __gitcomp_nl \"$refs\"\n\n real 0m0.109s\n user 0m0.096s\n sys 0m0.012s\n\n Yours:\n\n $ time __gitcomp_nl \"$refs\"\n\n real 0m0.321s\n user 0m0.312s\n sys 0m0.008s\n"},{"id":"203430","messageId":"CAMP44s2YOshWkM9n2XxxLw79=-VF8U3Q4ta1D1zgHqWH30zOmw@mail.gmail.com","threadId":"32134","inReplyTo":"20121117141422.GI12052@goldbirke","subject":"Re: [PATCH 5/7] completion: fix expansion issues in __gitcomp_nl()","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-11-17T19:08:07Z","receivedAt":"2012-11-17T19:08:07Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Nov 17, 2012 at 3:14 PM, SZEDER Gábor wrote:\n> On Sat, Nov 17, 2012 at 12:50:39PM +0100, Felipe Contreras wrote:\n>> On Sat, Nov 17, 2012 at 12:05 PM, SZEDER Gábor wrote:\n>>\n>> > __gitcomp_nl ()\n>> > {\n>> > local IFS=$'\\n'\n>> > - COMPREPLY=($(compgen -P \"${2-}\" -S \"${4- }\" -W \"$1\" -- \"${3-$cur}\"))\n>> > + COMPREPLY=($(awk -v pfx=\"${2-}\" -v sfx=\"${4- }\" -v cur=\"${3-$cur}\" '\n>> > + BEGIN {\n>> > + FS=\"\\n\";\n>> > + len=length(cur);\n>> > + }\n>> > + {\n>> > + if (cur == substr($1, 1, len))\n>> > + print pfx$1sfx;\n>> > + }' <<< \"$1\" ))\n>> > }\n>>\n>> Does this really perform better than my alternative?\n>>\n>> + for x in $1; do\n>> + if [[ \"$x\" = \"$3\"* ]]; then\n>> + COMPREPLY+=(\"$2$x$4\")\n>> + fi\n>> + done\n>\n> It does:\n>\n> My version:\n>\n> $ refs=\"$(for i in {0..9999} ; do echo branch$i ; done)\"\n> $ time __gitcomp_nl \"$refs\"\n>\n> real 0m0.109s\n> user 0m0.096s\n> sys 0m0.012s\n>\n> Yours:\n>\n> $ time __gitcomp_nl \"$refs\"\n>\n> real 0m0.321s\n> user 0m0.312s\n> sys 0m0.008s\n\nYeah, for 10000 refs, which is not the common case:\n\n== 1 ==\nSZEDER:\nreal\t0m0.007s\nuser\t0m0.003s\nsys\t0m0.000s\nfelipec:\nreal\t0m0.000s\nuser\t0m0.000s\nsys\t0m0.000s\n== 10 ==\nSZEDER:\nreal\t0m0.004s\nuser\t0m0.003s\nsys\t0m0.001s\nfelipec:\nreal\t0m0.000s\nuser\t0m0.000s\nsys\t0m0.000s\n== 100 ==\nSZEDER:\nreal\t0m0.005s\nuser\t0m0.002s\nsys\t0m0.002s\nfelipec:\nreal\t0m0.002s\nuser\t0m0.002s\nsys\t0m0.000s\n== 1000 ==\nSZEDER:\nreal\t0m0.010s\nuser\t0m0.008s\nsys\t0m0.001s\nfelipec:\nreal\t0m0.018s\nuser\t0m0.017s\nsys\t0m0.001s\n== 10000 ==\nSZEDER:\nreal\t0m0.062s\nuser\t0m0.060s\nsys\t0m0.003s\nfelipec:\nreal\t0m0.175s\nuser\t0m0.174s\nsys\t0m0.000s\n== 100000 ==\nSZEDER:\nreal\t0m0.595s\nuser\t0m0.593s\nsys\t0m0.021s\nfelipec:\nreal\t0m1.848s\nuser\t0m1.843s\nsys\t0m0.003s\n== 1000000 ==\nSZEDER:\nreal\t0m6.258s\nuser\t0m6.241s\nsys\t0m0.215s\nfelipec:\nreal\t0m18.191s\nuser\t0m18.115s\nsys\t0m0.045s\n\n-- \nFelipe Contreras\n"},{"id":"203432","messageId":"CAMP44s1zaxey7TQxGaLtaMUwPTVTmRBn1w6=Zqefy7FJzEepBw@mail.gmail.com","threadId":"32134","inReplyTo":"CAMP44s2YOshWkM9n2XxxLw79=-VF8U3Q4ta1D1zgHqWH30zOmw@mail.gmail.com","subject":"Re: [PATCH 5/7] completion: fix expansion issues in __gitcomp_nl()","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-11-17T19:28:06Z","receivedAt":"2012-11-17T19:28:06Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Nov 17, 2012 at 8:08 PM, Felipe Contreras\n wrote:\n> On Sat, Nov 17, 2012 at 3:14 PM, SZEDER Gábor wrote:\n>> On Sat, Nov 17, 2012 at 12:50:39PM +0100, Felipe Contreras wrote:\n>>> On Sat, Nov 17, 2012 at 12:05 PM, SZEDER Gábor wrote:\n>>>\n>>> > __gitcomp_nl ()\n>>> > {\n>>> > local IFS=$'\\n'\n>>> > - COMPREPLY=($(compgen -P \"${2-}\" -S \"${4- }\" -W \"$1\" -- \"${3-$cur}\"))\n>>> > + COMPREPLY=($(awk -v pfx=\"${2-}\" -v sfx=\"${4- }\" -v cur=\"${3-$cur}\" '\n>>> > + BEGIN {\n>>> > + FS=\"\\n\";\n>>> > + len=length(cur);\n>>> > + }\n>>> > + {\n>>> > + if (cur == substr($1, 1, len))\n>>> > + print pfx$1sfx;\n>>> > + }' <<< \"$1\" ))\n>>> > }\n>>>\n>>> Does this really perform better than my alternative?\n>>>\n>>> + for x in $1; do\n>>> + if [[ \"$x\" = \"$3\"* ]]; then\n>>> + COMPREPLY+=(\"$2$x$4\")\n>>> + fi\n>>> + done\n>>\n>> It does:\n>>\n>> My version:\n>>\n>> $ refs=\"$(for i in {0..9999} ; do echo branch$i ; done)\"\n>> $ time __gitcomp_nl \"$refs\"\n>>\n>> real 0m0.109s\n>> user 0m0.096s\n>> sys 0m0.012s\n>>\n>> Yours:\n>>\n>> $ time __gitcomp_nl \"$refs\"\n>>\n>> real 0m0.321s\n>> user 0m0.312s\n>> sys 0m0.008s\n>\n> Yeah, for 10000 refs, which is not the common case:\n\nAnd this beats both in every case:\n\nwhile read -r x; do\n\tif [[ \"$x\" == \"$3\"* ]]; then\n\t\tCOMPREPLY+=(\"$2$x$4\")\n\tfi\ndone <<< $1\n\n== 1 ==\none:\nreal\t0m0.004s\nuser\t0m0.003s\nsys\t0m0.000s\ntwo:\nreal\t0m0.000s\nuser\t0m0.000s\nsys\t0m0.000s\n== 10 ==\none:\nreal\t0m0.005s\nuser\t0m0.002s\nsys\t0m0.002s\ntwo:\nreal\t0m0.000s\nuser\t0m0.000s\nsys\t0m0.000s\n== 100 ==\none:\nreal\t0m0.005s\nuser\t0m0.004s\nsys\t0m0.000s\ntwo:\nreal\t0m0.001s\nuser\t0m0.000s\nsys\t0m0.000s\n== 1000 ==\none:\nreal\t0m0.010s\nuser\t0m0.008s\nsys\t0m0.001s\ntwo:\nreal\t0m0.004s\nuser\t0m0.004s\nsys\t0m0.000s\n== 10000 ==\none:\nreal\t0m0.061s\nuser\t0m0.057s\nsys\t0m0.005s\ntwo:\nreal\t0m0.045s\nuser\t0m0.044s\nsys\t0m0.000s\n== 100000 ==\none:\nreal\t0m0.582s\nuser\t0m0.575s\nsys\t0m0.022s\ntwo:\nreal\t0m0.487s\nuser\t0m0.482s\nsys\t0m0.004s\n== 1000000 ==\none:\nreal\t0m6.305s\nuser\t0m6.284s\nsys\t0m0.216s\ntwo:\nreal\t0m5.268s\nuser\t0m5.194s\nsys\t0m0.065s\n\n-- \nFelipe Contreras\n"},{"id":"203445","messageId":"20121117230022.GA3815@elie.Belkin","threadId":"32134","inReplyTo":"1353150353-29874-2-git-send-email-szeder@ira.uka.de","subject":"Re: [PATCH 1/7] completion: make the 'basic' test more tester-friendly","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-11-17T23:00:22Z","receivedAt":"2012-11-17T23:00:22Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"SZEDER Gábor wrote:\n\n> The 'basic' test uses 'grep -q' to filter the resulting possible\n> completion words while looking for the presence or absence of certain\n> git commands, and relies on grep's exit status to indicate a failure.\n[...]\n> To make testers' life easier provide some output about the failed\n> condition: store the results of the filtering in a file and compare\n> its contents to the expected results by the good old test_cmp helper.\n\nLooks good. I wonder if this points to the need for a test_grep\nhelper more generally.\n\n[...]\n> \trun_completion \"git f\" &&\n> -\t! grep -q -v \"^f\" out\n> +\t>expected &&\n> +\tsed -n \"/^[^f]/p\" out >out2 &&\n> +\ttest_cmp expected out2\n\nFunctional change: before, this would fail if \"out\" contained a blank\nline, while afterward it does not. I doubt that matters, though.\n\nThanks and hope that helps,\nJonathan\n"},{"id":"203446","messageId":"20121117230249.GB3815@elie.Belkin","threadId":"32134","inReplyTo":"1353150353-29874-3-git-send-email-szeder@ira.uka.de","subject":"Re: [PATCH 2/7] completion: fix args of run_completion() test helper","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-11-17T23:02:49Z","receivedAt":"2012-11-17T23:02:49Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"SZEDER Gábor wrote:\n\n> Fix this by passing the command line to run_completion() as separate\n> words.\n\nGood catch. The change makes sense, the code looks saner after the\nfix, and since this is test code any breakage should be caught\nquickly, so\n\nReviewed-by: Jonathan Nieder \n\nThanks.\n"},{"id":"203447","messageId":"20121117233154.GC3815@elie.Belkin","threadId":"32134","inReplyTo":"1353150353-29874-4-git-send-email-szeder@ira.uka.de","subject":"Re: [PATCH 3/7] completion: add tests for the __gitcomp_nl() completion helper function","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-11-17T23:31:54Z","receivedAt":"2012-11-17T23:31:54Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"SZEDER Gábor wrote:\n\n> --- a/t/t9902-completion.sh\n> +++ b/t/t9902-completion.sh\n> @@ -155,6 +155,90 @@ test_expect_success '__gitcomp - suffix' '\n> \ttest_cmp expected out\n> '\n> \n> +test_expect_success '__gitcomp_nl - trailing space' '\n> +\tsed -e \"s/Z$//\" >expected <<-\\EOF &&\n\n'$' is usually a shell metacharacter, so it would be more comfortable\nto read a version that escapes it:\n\n\tsed -e \"s/Z\\$//\" >expected <<-\\EOF\n\nSince '$/' is not a valid parameter expansion, if I understand\ncorrectly then POSIX leaves the meaning of the quoted string \"s/Z$//\"\nundefined (XCU §2.2.3). Luckily every shell I've tried, including\nbash, keeps the $ unmolested.\n\nOther parts of the file already use the same style, so I wouldn't\nsuggest changing it in this patch.\n\nThanks for some nice tests.\n\nReviewed-by: Jonathan Nieder \n"},{"id":"203448","messageId":"20121117234059.GD3815@elie.Belkin","threadId":"32134","inReplyTo":"1353150353-29874-5-git-send-email-szeder@ira.uka.de","subject":"Re: [PATCH 4/7] completion: add tests for invalid variable name among completion words","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-11-17T23:40:59Z","receivedAt":"2012-11-17T23:40:59Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"SZEDER Gábor wrote:\n\n> The breakage can\n> be simply bogus possible completion words, but it can also be a\n> failure:\n>\n> $ git branch '${foo.bar}'\n> $ git checkout \n> bash: ${foo.bar}: bad substitution\n\nOr arbitrary code execution:\n\n\t$ git branch '$(>kilroy-was-here)'\n\t$ git checkout \n\t$ ls -l kilroy-was-here\n\t-rw-rw-r-- 1 jrn jrn 0 nov 17 15:40 kilroy-was-here\n\nThe final version of this patch should go to maint. Thanks for\ncatching it.\n"},{"id":"203450","messageId":"CAMP44s22aG65Tvg3y7ZSEKqJy+Xuvwt_1BjSBV7vqOz+vgzg3g@mail.gmail.com","threadId":"32134","inReplyTo":"CAMP44s1zaxey7TQxGaLtaMUwPTVTmRBn1w6=Zqefy7FJzEepBw@mail.gmail.com","subject":"Re: [PATCH 5/7] completion: fix expansion issues in __gitcomp_nl()","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-11-18T00:55:26Z","receivedAt":"2012-11-18T00:55:26Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Nov 17, 2012 at 8:28 PM, Felipe Contreras\n wrote:\n> On Sat, Nov 17, 2012 at 8:08 PM, Felipe Contreras\n> wrote:\n>> On Sat, Nov 17, 2012 at 3:14 PM, SZEDER Gábor wrote:\n>>> On Sat, Nov 17, 2012 at 12:50:39PM +0100, Felipe Contreras wrote:\n>>>> On Sat, Nov 17, 2012 at 12:05 PM, SZEDER Gábor wrote:\n>>>>\n>>>> > __gitcomp_nl ()\n>>>> > {\n>>>> > local IFS=$'\\n'\n>>>> > - COMPREPLY=($(compgen -P \"${2-}\" -S \"${4- }\" -W \"$1\" -- \"${3-$cur}\"))\n>>>> > + COMPREPLY=($(awk -v pfx=\"${2-}\" -v sfx=\"${4- }\" -v cur=\"${3-$cur}\" '\n>>>> > + BEGIN {\n>>>> > + FS=\"\\n\";\n>>>> > + len=length(cur);\n>>>> > + }\n>>>> > + {\n>>>> > + if (cur == substr($1, 1, len))\n>>>> > + print pfx$1sfx;\n>>>> > + }' <<< \"$1\" ))\n>>>> > }\n>>>>\n>>>> Does this really perform better than my alternative?\n>>>>\n>>>> + for x in $1; do\n>>>> + if [[ \"$x\" = \"$3\"* ]]; then\n>>>> + COMPREPLY+=(\"$2$x$4\")\n>>>> + fi\n>>>> + done\n>>>\n>>> It does:\n>>>\n>>> My version:\n>>>\n>>> $ refs=\"$(for i in {0..9999} ; do echo branch$i ; done)\"\n>>> $ time __gitcomp_nl \"$refs\"\n>>>\n>>> real 0m0.109s\n>>> user 0m0.096s\n>>> sys 0m0.012s\n>>>\n>>> Yours:\n>>>\n>>> $ time __gitcomp_nl \"$refs\"\n>>>\n>>> real 0m0.321s\n>>> user 0m0.312s\n>>> sys 0m0.008s\n>>\n>> Yeah, for 10000 refs, which is not the common case:\n>\n> And this beats both in every case:\n>\n> while read -r x; do\n> if [[ \"$x\" == \"$3\"* ]]; then\n> COMPREPLY+=(\"$2$x$4\")\n> fi\n> done <<< $1\n\nNevermind that.\n\nHere's another:\n\n\tlocal IFS=$'\\n'\n\tCOMPREPLY=($(printf -- \"$2%s$4\\n\" $1 | grep \"^$2$3\"))\n\n-- \nFelipe Contreras\n"},{"id":"203451","messageId":"CAMP44s1cskzf4TorLOVWCFtauMsujsQzWAZ5ZbtCuDMs=n=G9g@mail.gmail.com","threadId":"32134","inReplyTo":"1353150353-29874-6-git-send-email-szeder@ira.uka.de","subject":"Re: [PATCH 5/7] completion: fix expansion issues in __gitcomp_nl()","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-11-18T00:58:27Z","receivedAt":"2012-11-18T00:58:27Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Nov 17, 2012 at 12:05 PM, SZEDER Gábor wrote:\n\n> __gitcomp_nl ()\n> {\n> local IFS=$'\\n'\n> - COMPREPLY=($(compgen -P \"${2-}\" -S \"${4- }\" -W \"$1\" -- \"${3-$cur}\"))\n> + COMPREPLY=($(awk -v pfx=\"${2-}\" -v sfx=\"${4- }\" -v cur=\"${3-$cur}\" '\n> + BEGIN {\n> + FS=\"\\n\";\n> + len=length(cur);\n> + }\n> + {\n> + if (cur == substr($1, 1, len))\n> + print pfx$1sfx;\n> + }' <<< \"$1\" ))\n> }\n\nThis version is simpler and faster:\n\n\tlocal IFS=$'\\n'\n\tCOMPREPLY=($(awk -v cur=\"${3-$cur}\" -v pre=\"${2-}\" -v suf=\"${4- }\"\n'$0 ~ cur { print pre$0suf }' <<< \"$1\" ))\n\n== 10000 ==\nawk 1:\nreal\t0m0.067s\nuser\t0m0.066s\nsys\t0m0.001s\nawk 2:\nreal\t0m0.057s\nuser\t0m0.055s\nsys\t0m0.002s\n\n-- \nFelipe Contreras\n"},{"id":"203455","messageId":"CAMP44s1-YS_39KfFoRG_K-xvw+iu0_B097bv3kTv=3S2QWwaZA@mail.gmail.com","threadId":"32134","inReplyTo":"1353150353-29874-5-git-send-email-szeder@ira.uka.de","subject":"Re: [PATCH 4/7] completion: add tests for invalid variable name among completion words","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-11-18T08:34:02Z","receivedAt":"2012-11-18T08:34:02Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Nov 17, 2012 at 12:05 PM, SZEDER Gábor wrote:\n\n> @@ -155,6 +156,12 @@ test_expect_success '__gitcomp - suffix' '\n> test_cmp expected out\n> '\n>\n> +test_expect_failure '__gitcomp - doesnt fail because of invalid variable name' '\n> + (\n> + __gitcomp \"$invalid_variable_name\"\n> + )\n> +'\n\nWhy in a subshell?\n\n-- \nFelipe Contreras\n"},{"id":"203456","messageId":"CAMP44s26MkA8QWm_JQkk8M0mM39XK0Fv1CBZfpt52VKt1urkAw@mail.gmail.com","threadId":"32134","inReplyTo":"20121117234059.GD3815@elie.Belkin","subject":"Re: [PATCH 4/7] completion: add tests for invalid variable name among completion words","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-11-18T08:34:19Z","receivedAt":"2012-11-18T08:34:19Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sun, Nov 18, 2012 at 12:40 AM, Jonathan Nieder wrote:\n> SZEDER Gábor wrote:\n>\n>> The breakage can\n>> be simply bogus possible completion words, but it can also be a\n>> failure:\n>>\n>> $ git branch '${foo.bar}'\n>> $ git checkout \n>> bash: ${foo.bar}: bad substitution\n>\n> Or arbitrary code execution:\n>\n> $ git branch '$(>kilroy-was-here)'\n> $ git checkout \n> $ ls -l kilroy-was-here\n> -rw-rw-r-- 1 jrn jrn 0 nov 17 15:40 kilroy-was-here\n>\n> The final version of this patch should go to maint. Thanks for\n> catching it.\n\nShouldn't this go to the commit message?\n\n-- \nFelipe Contreras\n"},{"id":"203457","messageId":"CAMP44s3FmB3F5zjQcVQJDwo7K5m19203Z7D67beHv726g3TL_A@mail.gmail.com","threadId":"32134","inReplyTo":"1353150353-29874-3-git-send-email-szeder@ira.uka.de","subject":"Re: [PATCH 2/7] completion: fix args of run_completion() test helper","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-11-18T08:48:01Z","receivedAt":"2012-11-18T08:48:01Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Nov 17, 2012 at 12:05 PM, SZEDER Gábor wrote:\n\n> test_expect_success 'basic' '\n> - run_completion \"git \\\"\\\"\" &&\n> + run_completion git \"\" &&\n\nI don't like this approach. I prefer\n\nrun_completion \"git \"\n\nSo that it's similar to how test_completion is called:\n\nrun_completion \"git f\"\ntest_completion \"git f\"\n\n-- \nFelipe Contreras\n"},{"id":"203458","messageId":"CAMP44s1CFPt5ZmDuOEbLnsz5ShSSCO3O7Ws0CkMhjrpnkOMdJw@mail.gmail.com","threadId":"32134","inReplyTo":"1353150353-29874-4-git-send-email-szeder@ira.uka.de","subject":"Re: [PATCH 3/7] completion: add tests for the __gitcomp_nl() completion helper function","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-11-18T08:53:57Z","receivedAt":"2012-11-18T08:53:57Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Nov 17, 2012 at 12:05 PM, SZEDER Gábor wrote:\n> Test __gitcomp_nl()'s basic functionality, i.e. that trailing space,\n> prefix, and suffix are added correctly.\n>\n> Signed-off-by: SZEDER Gábor \n> ---\n> t/t9902-completion.sh | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++\n> 1 file changed, 84 insertions(+)\n\nToo much code that can be simplified:\n\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -85,6 +85,22 @@ test_gitcomp ()\n \ttest_cmp expected out\n }\n\n+# Test __gitcomp_nl\n+# Arguments are:\n+# 1: typed text so far (cur)\n+# -: the rest are passed to __gitcomp_nl\n+test_gitcomp_nl ()\n+{\n+\tlocal -a COMPREPLY &&\n+\tsed -e 's/Z$//' > expected &&\n+\tcur=\"$1\" &&\n+\tshift &&\n+\t__gitcomp_nl \"$@\" &&\n+\tprint_comp &&\n+\tcp expected out /tmp\n+\ttest_cmp expected out\n+}\n+\n invalid_variable_name=\"${foo.bar}\"\n\n test_expect_success '__gitcomp - trailing space - options' '\n@@ -134,88 +150,39 @@ test_expect_success '__gitcomp - doesnt fail\nbecause of invalid variable name' '\n \t__gitcomp \"$invalid_variable_name\"\n '\n\n+read -r -d \"\" refs <<-\\EOF\n+maint\n+master\n+next\n+pu\n+EOF\n+\n test_expect_success '__gitcomp_nl - trailing space' '\n-\tsed -e \"s/Z$//\" >expected <<-\\EOF &&\n+\ttest_gitcomp_nl \"m\" \"$refs\" <<-EOF\n \tmaint Z\n \tmaster Z\n \tEOF\n-\t(\n-\t\tdeclare -a COMPREPLY &&\n-\t\trefs=\"$(cat <<-\\EOF\n-\t\t\tmaint\n-\t\t\tmaster\n-\t\t\tnext\n-\t\t\tpu\n-\t\t\tEOF\n-\t\t)\" &&\n-\t\tcur=\"m\" &&\n-\t\t__gitcomp_nl \"$refs\" &&\n-\t\tprint_comp\n-\t) &&\n-\ttest_cmp expected out\n '\n\n test_expect_success '__gitcomp_nl - prefix' '\n-\tsed -e \"s/Z$//\" >expected <<-\\EOF &&\n+\ttest_gitcomp_nl \"--fixup=m\" \"$refs\" \"--fixup=\" \"m\" <<-EOF\n \t--fixup=maint Z\n \t--fixup=master Z\n \tEOF\n-\t(\n-\t\tdeclare -a COMPREPLY &&\n-\t\trefs=\"$(cat <<-\\EOF\n-\t\t\tmaint\n-\t\t\tmaster\n-\t\t\tnext\n-\t\t\tpu\n-\t\t\tEOF\n-\t\t)\" &&\n-\t\tcur=\"--fixup=m\" &&\n-\t\t__gitcomp_nl \"$refs\" \"--fixup=\" \"m\" &&\n-\t\tprint_comp\n-\t) &&\n-\ttest_cmp expected out\n '\n\n test_expect_success '__gitcomp_nl - suffix' '\n-\tsed -e \"s/Z$//\" >expected <<-\\EOF &&\n+\ttest_gitcomp_nl \"branch.ma\" \"$refs\" \"branch.\" \"ma\" \".\" <<-\\EOF\n \tbranch.maint.Z\n \tbranch.master.Z\n \tEOF\n-\t(\n-\t\tdeclare -a COMPREPLY &&\n-\t\trefs=\"$(cat <<-\\EOF\n-\t\t\tmaint\n-\t\t\tmaster\n-\t\t\tnext\n-\t\t\tpu\n-\t\t\tEOF\n-\t\t)\" &&\n-\t\tcur=\"branch.ma\" &&\n-\t\t__gitcomp_nl \"$refs\" \"branch.\" \"ma\" \".\" &&\n-\t\tprint_comp\n-\t) &&\n-\ttest_cmp expected out\n '\n\n test_expect_success '__gitcomp_nl - no suffix' '\n-\tsed -e \"s/Z$//\" >expected <<-\\EOF &&\n+\ttest_gitcomp_nl \"ma\" \"$refs\" \"\" \"ma\" \"\" <<-\\EOF\n \tmaintZ\n \tmasterZ\n \tEOF\n-\t(\n-\t\tdeclare -a COMPREPLY &&\n-\t\trefs=\"$(cat <<-\\EOF\n-\t\t\tmaint\n-\t\t\tmaster\n-\t\t\tnext\n-\t\t\tpu\n-\t\t\tEOF\n-\t\t)\" &&\n-\t\tcur=\"ma\" &&\n-\t\t__gitcomp_nl \"$refs\" \"\" \"ma\" \"\" &&\n-\t\tprint_comp\n-\t) &&\n-\ttest_cmp expected out\n '\n\n test_expect_success '__gitcomp_nl - doesnt fail because of invalid\nvariable name' '\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"203463","messageId":"CAMP44s2JgNcKMmhohdg1Ynz1C6r+HVCLm8XCqs=hhjE2_8vs-A@mail.gmail.com","threadId":"32134","inReplyTo":"20121117230022.GA3815@elie.Belkin","subject":"Re: [PATCH 1/7] completion: make the 'basic' test more tester-friendly","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-11-18T09:38:08Z","receivedAt":"2012-11-18T09:38:08Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sun, Nov 18, 2012 at 12:00 AM, Jonathan Nieder wrote:\n> SZEDER Gábor wrote:\n>\n>> The 'basic' test uses 'grep -q' to filter the resulting possible\n>> completion words while looking for the presence or absence of certain\n>> git commands, and relies on grep's exit status to indicate a failure.\n> [...]\n>> To make testers' life easier provide some output about the failed\n>> condition: store the results of the filtering in a file and compare\n>> its contents to the expected results by the good old test_cmp helper.\n>\n> Looks good. I wonder if this points to the need for a test_grep\n> helper more generally.\n\nIt does.\n\n-- \nFelipe Contreras\n"},{"id":"203464","messageId":"CAMP44s1zibHCFCm_=E+WXmTpX+xvpLWGbcpf32QuWsWAa+Y5uA@mail.gmail.com","threadId":"32134","inReplyTo":"1353150353-29874-2-git-send-email-szeder@ira.uka.de","subject":"Re: [PATCH 1/7] completion: make the 'basic' test more tester-friendly","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-11-18T09:39:02Z","receivedAt":"2012-11-18T09:39:02Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Nov 17, 2012 at 12:05 PM, SZEDER Gábor wrote:\n> The 'basic' test uses 'grep -q' to filter the resulting possible\n> completion words while looking for the presence or absence of certain\n> git commands, and relies on grep's exit status to indicate a failure.\n>\n> This works fine as long as there are no errors. However, in case of a\n> failure it doesn't give any indication whatsoever about the reason of\n> the failure, i.e. which condition failed.\n>\n> To make testers' life easier provide some output about the failed\n> condition: store the results of the filtering in a file and compare\n> its contents to the expected results by the good old test_cmp helper.\n> However, to actually get output from test_cmp in case of an error we\n> must make sure that test_cmp is always executed. Since in case of an\n> error grep's exit code aborts the test immediately, before running the\n> subsequent test_cmp, do the filtering using sed instead of grep.\n>\n> Signed-off-by: SZEDER Gábor \n> ---\n> t/t9902-completion.sh | 16 ++++++++++++----\n> 1 file changed, 12 insertions(+), 4 deletions(-)\n>\n> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\n> index 8fa025f9..b56759f7 100755\n> --- a/t/t9902-completion.sh\n> +++ b/t/t9902-completion.sh\n> @@ -158,14 +158,22 @@ test_expect_success '__gitcomp - suffix' '\n> test_expect_success 'basic' '\n> run_completion \"git \\\"\\\"\" &&\n> # built-in\n> - grep -q \"^add \\$\" out &&\n> + echo \"add \" >expected &&\n> + sed -n \"/^add \\$/p\" out >out2 &&\n> + test_cmp expected out2 &&\n> # script\n> - grep -q \"^filter-branch \\$\" out &&\n> + echo \"filter-branch \" >expected &&\n> + sed -n \"/^filter-branch \\$/p\" out >out2 &&\n> + test_cmp expected out2 &&\n> # plumbing\n> - ! grep -q \"^ls-files \\$\" out &&\n> + >expected &&\n> + sed -n \"/^ls-files \\$/p\" out >out2 &&\n> + test_cmp expected out2 &&\n>\n> run_completion \"git f\" &&\n> - ! grep -q -v \"^f\" out\n> + >expected &&\n> + sed -n \"/^[^f]/p\" out >out2 &&\n> + test_cmp expected out2\n> '\n>\n> test_expect_success 'double dash \"git\" itself' '\n\nIt's not very useful to see a single failure without context. Maybe this:\n\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -191,18 +191,20 @@ test_expect_success '__gitcomp_nl - doesnt fail\nbecause of invalid variable name\n\n test_expect_success 'basic' '\n run_completion \"git \" &&\n+ (\n # built-in\n- echo \"add \" >expected &&\n- sed -n \"/^add \\$/p\" out >out2 &&\n- test_cmp expected out2 &&\n+ sed -n \"/^add $/p\" out &&\n # script\n- echo \"filter-branch \" >expected &&\n- sed -n \"/^filter-branch \\$/p\" out >out2 &&\n- test_cmp expected out2 &&\n+ sed -n \"/^filter-branch $/p\" out &&\n # plumbing\n- >expected &&\n- sed -n \"/^ls-files \\$/p\" out >out2 &&\n- test_cmp expected out2 &&\n+ sed -n \"/^ls-files $/p\" out\n+ ) > actual &&\n+\n+ sed -e 's/Z$//' > expected <<-EOF &&\n+ add Z\n+ filter-branch Z\n+ EOF\n+ test_cmp expected actual &&\n\n run_completion \"git f\" &&\n >expected &&\n\n-- \nFelipe Contreras\n"}]}