{"thread":{"id":"54854","subject":"[PATCH try2 0/4] completion: bash: a bunch of fixes","startedAt":"2020-12-19T14:07:06Z","lastAt":"2020-12-24T00:22:01Z","messageCount":11,"participants":["Felipe Contreras","Junio C Hamano","SZEDER Gábor"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"412636","messageId":"20201219140621.1961760-1-felipe.contreras@gmail.com","threadId":"54854","inReplyTo":null,"subject":"[PATCH try2 0/4] completion: bash: a bunch of fixes","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-19T14:06:17Z","receivedAt":"2020-12-19T14:07:06Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"This is just the bug fixes from the previous series.\n\nThese should be pretty obvious and straightforward.\n\nFelipe Contreras (4):\n  completion: bash: fix prefix detection in branch.*\n  completion: bash: add correct suffix in variables\n  completion: bash: fix for suboptions with value\n  completion: bash: fix for multiple dash commands\n\n contrib/completion/git-completion.bash | 14 +++++++-------\n t/t9902-completion.sh                  | 22 ++++++++++++++++++++++\n 2 files changed, 29 insertions(+), 7 deletions(-)\n\n-- \n2.30.0.rc0\n\n"},{"id":"412637","messageId":"20201219140621.1961760-2-felipe.contreras@gmail.com","threadId":"54854","inReplyTo":"20201219140621.1961760-1-felipe.contreras@gmail.com","subject":"[PATCH try2 1/4] completion: bash: fix prefix detection in branch.*","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-19T14:06:18Z","receivedAt":"2020-12-19T14:07:23Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Otherwise we are completely ignoring the --cur argument.\n\nThe issue can be tested with:\n\n  git clone --config=branch.<tab>\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n contrib/completion/git-completion.bash | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 7c81e4ba49..b866b68b3c 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -2615,8 +2615,8 @@ __git_complete_config_variable_name ()\n \t\treturn\n \t\t;;\n \tbranch.*)\n-\t\tlocal pfx=\"${cur%.*}.\"\n-\t\tcur_=\"${cur#*.}\"\n+\t\tlocal pfx=\"${cur_%.*}.\"\n+\t\tcur_=\"${cur_#*.}\"\n \t\t__gitcomp_direct \"$(__git_heads \"$pfx\" \"$cur_\" \".\")\"\n \t\t__gitcomp_nl_append $'autoSetupMerge\\nautoSetupRebase\\n' \"$pfx\" \"$cur_\" \"$sfx\"\n \t\treturn\n-- \n2.30.0.rc0\n\n"},{"id":"412638","messageId":"20201219140621.1961760-3-felipe.contreras@gmail.com","threadId":"54854","inReplyTo":"20201219140621.1961760-1-felipe.contreras@gmail.com","subject":"[PATCH try2 2/4] completion: bash: add correct suffix in variables","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-19T14:06:19Z","receivedAt":"2020-12-19T14:07:23Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"__gitcomp automatically adds a suffix, but __gitcomp_nl and others\ndon't, we need to specify a space by default.\n\nCan be tested with:\n\n  git config branch.autoSetupMe<tab>\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n contrib/completion/git-completion.bash | 6 +++---\n t/t9902-completion.sh                  | 7 +++++++\n 2 files changed, 10 insertions(+), 3 deletions(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex b866b68b3c..bbdb46d87e 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -2618,7 +2618,7 @@ __git_complete_config_variable_name ()\n \t\tlocal pfx=\"${cur_%.*}.\"\n \t\tcur_=\"${cur_#*.}\"\n \t\t__gitcomp_direct \"$(__git_heads \"$pfx\" \"$cur_\" \".\")\"\n-\t\t__gitcomp_nl_append $'autoSetupMerge\\nautoSetupRebase\\n' \"$pfx\" \"$cur_\" \"$sfx\"\n+\t\t__gitcomp_nl_append $'autoSetupMerge\\nautoSetupRebase\\n' \"$pfx\" \"$cur_\" \"${sfx- }\"\n \t\treturn\n \t\t;;\n \tguitool.*.*)\n@@ -2652,7 +2652,7 @@ __git_complete_config_variable_name ()\n \t\tlocal pfx=\"${cur_%.*}.\"\n \t\tcur_=\"${cur_#*.}\"\n \t\t__git_compute_all_commands\n-\t\t__gitcomp_nl \"$__git_all_commands\" \"$pfx\" \"$cur_\" \"$sfx\"\n+\t\t__gitcomp_nl \"$__git_all_commands\" \"$pfx\" \"$cur_\" \"${sfx- }\"\n \t\treturn\n \t\t;;\n \tremote.*.*)\n@@ -2668,7 +2668,7 @@ __git_complete_config_variable_name ()\n \t\tlocal pfx=\"${cur_%.*}.\"\n \t\tcur_=\"${cur_#*.}\"\n \t\t__gitcomp_nl \"$(__git_remotes)\" \"$pfx\" \"$cur_\" \".\"\n-\t\t__gitcomp_nl_append \"pushDefault\" \"$pfx\" \"$cur_\" \"$sfx\"\n+\t\t__gitcomp_nl_append \"pushDefault\" \"$pfx\" \"$cur_\" \"${sfx- }\"\n \t\treturn\n \t\t;;\n \turl.*.*)\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex 2be9190425..4a3d3d1597 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -2293,6 +2293,13 @@ test_expect_success 'git config - value' '\n \tEOF\n '\n \n+test_expect_success 'git config - direct completions' '\n+\ttest_completion \"git config branch.autoSetup\" <<-\\EOF\n+\tbranch.autoSetupMerge Z\n+\tbranch.autoSetupRebase Z\n+\tEOF\n+'\n+\n test_expect_success 'git -c - section' '\n \ttest_completion \"git -c br\" <<-\\EOF\n \tbranch.Z\n-- \n2.30.0.rc0\n\n"},{"id":"412639","messageId":"20201219140621.1961760-5-felipe.contreras@gmail.com","threadId":"54854","inReplyTo":"20201219140621.1961760-1-felipe.contreras@gmail.com","subject":"[PATCH try2 4/4] completion: bash: fix for multiple dash commands","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-19T14:06:21Z","receivedAt":"2020-12-19T14:07:23Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Otherwise commands like 'for-each-ref' are not completed.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\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 5b2dff150d..7e893abf43 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -412,7 +412,7 @@ __gitcomp_builtin ()\n \tlocal incl=\"${2-}\"\n \tlocal excl=\"${3-}\"\n \n-\tlocal var=__gitcomp_builtin_\"${cmd/-/_}\"\n+\tlocal var=__gitcomp_builtin_\"${cmd//-/_}\"\n \tlocal options\n \teval \"options=\\${$var-}\"\n \n-- \n2.30.0.rc0\n\n"},{"id":"412640","messageId":"20201219140621.1961760-4-felipe.contreras@gmail.com","threadId":"54854","inReplyTo":"20201219140621.1961760-1-felipe.contreras@gmail.com","subject":"[PATCH try2 3/4] completion: bash: fix for suboptions with value","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-19T14:06:20Z","receivedAt":"2020-12-19T14:07:23Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"We need to ignore options that don't start with -- as well.\n\nDepending on the value of COMP_WORDBREAKS the last word could be\nduplicated otherwise.\n\nCan be tested with:\n\n  git merge -X diff-algorithm=<tab>\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n contrib/completion/git-completion.bash |  2 +-\n t/t9902-completion.sh                  | 15 +++++++++++++++\n 2 files changed, 16 insertions(+), 1 deletion(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex bbdb46d87e..5b2dff150d 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -347,7 +347,7 @@ __gitcomp ()\n \tlocal cur_=\"${3-$cur}\"\n \n \tcase \"$cur_\" in\n-\t--*=)\n+\t*=)\n \t\t;;\n \t--no-*)\n \t\tlocal c i=0 IFS=$' \\t\\n'\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex 4a3d3d1597..4deda01153 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -537,6 +537,15 @@ test_expect_success '__gitcomp - expand/narrow all negative options' '\n \tEOF\n '\n \n+test_expect_success '__gitcomp - equal skip' '\n+\ttest_gitcomp \"--option=\" \"--option=\" <<-\\EOF &&\n+\n+\tEOF\n+\ttest_gitcomp \"option=\" \"option=\" <<-\\EOF\n+\n+\tEOF\n+'\n+\n test_expect_success '__gitcomp - doesnt fail because of invalid variable name' '\n \t__gitcomp \"$invalid_variable_name\"\n '\n@@ -2342,6 +2351,12 @@ test_expect_success 'git clone --config= - value' '\n \tEOF\n '\n \n+test_expect_success 'options with value' '\n+\ttest_completion \"git merge -X diff-algorithm=\" <<-\\EOF\n+\n+\tEOF\n+'\n+\n test_expect_success 'sourcing the completion script clears cached commands' '\n \t__git_compute_all_commands &&\n \tverbose test -n \"$__git_all_commands\" &&\n-- \n2.30.0.rc0\n\n"},{"id":"412913","messageId":"xmqqy2hoanps.fsf@gitster.c.googlers.com","threadId":"54854","inReplyTo":"20201219140621.1961760-1-felipe.contreras@gmail.com","subject":"Re: [PATCH try2 0/4] completion: bash: a bunch of fixes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-23T09:14:23Z","receivedAt":"2020-12-23T09:15:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n> This is just the bug fixes from the previous series.\n>\n> These should be pretty obvious and straightforward.\n>\n> Felipe Contreras (4):\n>   completion: bash: fix prefix detection in branch.*\n>   completion: bash: add correct suffix in variables\n>   completion: bash: fix for suboptions with value\n>   completion: bash: fix for multiple dash commands\n\nIt seems that this tickles some platform specific glitches in the\ntests (the detailed CI report can only be seen when logged in, it\nseems):\n\n    https://github.com/git/git/runs/1597682180#step:5:35614\n\nThe expected output lines have a trailing SP each, while the actual\noutput lines do not, but this failure happens only on macos job and\nnot others.\n\n"},{"id":"412922","messageId":"5fe3484465fac_198be208bf@natae.notmuch","threadId":"54854","inReplyTo":"xmqqy2hoanps.fsf@gitster.c.googlers.com","subject":"Re: [PATCH try2 0/4] completion: bash: a bunch of fixes","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-23T13:38:12Z","receivedAt":"2020-12-23T13:39:12Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Junio C Hamano wrote:\n> Felipe Contreras <felipe.contreras@gmail.com> writes:\n> \n> > This is just the bug fixes from the previous series.\n> >\n> > These should be pretty obvious and straightforward.\n> >\n> > Felipe Contreras (4):\n> >   completion: bash: fix prefix detection in branch.*\n> >   completion: bash: add correct suffix in variables\n> >   completion: bash: fix for suboptions with value\n> >   completion: bash: fix for multiple dash commands\n> \n> It seems that this tickles some platform specific glitches in the\n> tests (the detailed CI report can only be seen when logged in, it\n> seems):\n> \n>     https://github.com/git/git/runs/1597682180#step:5:35614\n\nI found that output very hard to parse, but I think I understand the\nissue. It's interesting.\n\nApaprently macOS uses zsh by default, and in zsh, this:\n\n  local sfx\n  echo \"'${sfx- }'\"\n\nPrints an empty string.\n\nThat's because in zsh \"local sfx\" is effectively \"local sfx=''\" which in\nmy opinion is a bug.\n\nI did catch this issue for zsh some time ago.\n\nI contacted the zsh developers a while ago, and that triggered a huge\ndiscussion, since they don't consider it a bug, but they are working on\ntentative changes. That's another story though.\n\nMy previous series with 26 patches didn't trigger that issue because I have\nfixes on top of of __gitcomp so suffixes work correctly, and the code\ncan eventually be changed to:\n\n  local sfx=\" \"\n\nThat makes the completion work correctly for both bash and zsh.\n\nI see 5 courses of action:\n\n 1. Drop the offending patch: this is wrong because the bug is still\n    there, we are just not checking for it.\n 2. Add a BASH prereq just for that test, or test_expect_unstable (we\n    would need to add extra code for both of those).\n 3. Add the fix, but not the test for the fix.\n 4. Backport my __gitcomp to make the code work for both shells.\n 5. Update the patch series to include all the changes up to the point\n    that is fixed.\n\nFor me obviously 1 and 5 require the least work, but in 1 the bug is\nstill there, and in 5 the patches might get stuck more time than\nnecessary.\n\nEither way I think the real problem is that not enough eyes are looking\nat these patches, so it's unclear if and when they will be merged.\n\nThe issues are real though, and they are present in the current code.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"412925","messageId":"20201223141950.GA23264@szeder.dev","threadId":"54854","inReplyTo":"5fe3484465fac_198be208bf@natae.notmuch","subject":"Re: [PATCH try2 0/4] completion: bash: a bunch of fixes","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2020-12-23T14:19:50Z","receivedAt":"2020-12-23T14:20:54Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Wed, Dec 23, 2020 at 07:38:12AM -0600, Felipe Contreras wrote:\n> Junio C Hamano wrote:\n> > Felipe Contreras <felipe.contreras@gmail.com> writes:\n> > \n> > > This is just the bug fixes from the previous series.\n> > >\n> > > These should be pretty obvious and straightforward.\n> > >\n> > > Felipe Contreras (4):\n> > >   completion: bash: fix prefix detection in branch.*\n> > >   completion: bash: add correct suffix in variables\n> > >   completion: bash: fix for suboptions with value\n> > >   completion: bash: fix for multiple dash commands\n> > \n> > It seems that this tickles some platform specific glitches in the\n> > tests (the detailed CI report can only be seen when logged in, it\n> > seems):\n> > \n> >     https://github.com/git/git/runs/1597682180#step:5:35614\n> \n> I found that output very hard to parse, but I think I understand the\n> issue. It's interesting.\n> \n> Apaprently macOS uses zsh by default,\n\nThe completion and prompt tests are suppsed to be run by Bash, no\nmatter what the default shell, or are skipped, or the setup is broken\n(a non-Bash shell sets $BASH, or 'exec bash' runs zsh).\n\nOur CI jobs use the default Bash version, which in the macOS jobs\nis v3.2.\n\n> and in zsh, this:\n> \n>   local sfx\n>   echo \"'${sfx- }'\"\n> \n> Prints an empty string.\n> \n> That's because in zsh \"local sfx\" is effectively \"local sfx=''\" which in\n> my opinion is a bug.\n\nBash versions up to 4.0-alpha suffered from this bug as well; I believe\nthe relevant changelog entry for 4.0-beta is this:\n\n  e.  Fixed a bug that caused local variables to be created with the empty\n      string for a value rather than no value.\n\nSo the default Bash version on macOS still has this bug, thus\n__gitcomp_nl_append() is invoked with an empty string sfx parameter\ninstead of a space, causing the test failure.  I can reproduce the\ntest failure on Linux using Bash v3.2 (and v3.1 and v3.0), and it\npasses with v4.0 and later versions.\n\n \n> I see 5 courses of action:\n> \n>  1. Drop the offending patch: this is wrong because the bug is still\n>     there, we are just not checking for it.\n>  2. Add a BASH prereq just for that test, or test_expect_unstable (we\n>     would need to add extra code for both of those).\n>  3. Add the fix, but not the test for the fix.\n\nI'm for this option 3: this patch does fix a bug for users of Bash\nv4.0 or later, while it doesn't change the behavior with v3.2 or\nearlier (and with zsh, if I understand correctly).  OTOH, the test\ndoesn't seem to be all that useful: while it does demonstrate the\nissue, it checks only one of those callsites that passed the wrong\nsuffix, and, more importantly, it doesn't protect us from adding\nanother callsites with similarly wrong suffex in the future.\n\nIn any case, the commit message should note that the fix doesn't work\nwith all Bash versions and why.\n\n"},{"id":"412927","messageId":"5fe354c91a6f7_198be208d8@natae.notmuch","threadId":"54854","inReplyTo":"20201223141950.GA23264@szeder.dev","subject":"Re: [PATCH try2 0/4] completion: bash: a bunch of fixes","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-23T14:31:37Z","receivedAt":"2020-12-23T14:32:36Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"SZEDER Gábor wrote:\n> On Wed, Dec 23, 2020 at 07:38:12AM -0600, Felipe Contreras wrote:\n\n> > and in zsh, this:\n> > \n> >   local sfx\n> >   echo \"'${sfx- }'\"\n> > \n> > Prints an empty string.\n> > \n> > That's because in zsh \"local sfx\" is effectively \"local sfx=''\" which in\n> > my opinion is a bug.\n> \n> Bash versions up to 4.0-alpha suffered from this bug as well; I believe\n> the relevant changelog entry for 4.0-beta is this:\n> \n>   e.  Fixed a bug that caused local variables to be created with the empty\n>       string for a value rather than no value.\n> \n> So the default Bash version on macOS still has this bug, thus\n> __gitcomp_nl_append() is invoked with an empty string sfx parameter\n> instead of a space, causing the test failure.  I can reproduce the\n> test failure on Linux using Bash v3.2 (and v3.1 and v3.0), and it\n> passes with v4.0 and later versions.\n\nAhhh, interesting.\n\nSo I'm not the only one who considers it a bug.\n\n> > I see 5 courses of action:\n> > \n> >  1. Drop the offending patch: this is wrong because the bug is still\n> >     there, we are just not checking for it.\n> >  2. Add a BASH prereq just for that test, or test_expect_unstable (we\n> >     would need to add extra code for both of those).\n> >  3. Add the fix, but not the test for the fix.\n> \n> I'm for this option 3: this patch does fix a bug for users of Bash\n> v4.0 or later, while it doesn't change the behavior with v3.2 or\n> earlier (and with zsh, if I understand correctly).  OTOH, the test\n> doesn't seem to be all that useful: while it does demonstrate the\n> issue, it checks only one of those callsites that passed the wrong\n> suffix, and, more importantly, it doesn't protect us from adding\n> another callsites with similarly wrong suffex in the future.\n\nI'm fine with that option.\n\n> In any case, the commit message should note that the fix doesn't work\n> with all Bash versions and why.\n\nYes. I will send a re-roll (unless somebody objects beforehand).\n\n-- \nFelipe Contreras"},{"id":"412948","messageId":"xmqq1rfg8e3j.fsf@gitster.c.googlers.com","threadId":"54854","inReplyTo":"20201223141950.GA23264@szeder.dev","subject":"Re: [PATCH try2 0/4] completion: bash: a bunch of fixes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-23T20:25:04Z","receivedAt":"2020-12-23T20:25:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor <szeder.dev@gmail.com> writes:\n\n> On Wed, Dec 23, 2020 at 07:38:12AM -0600, Felipe Contreras wrote:\n> ...\n>> I see 5 courses of action:\n>> \n>>  1. Drop the offending patch: this is wrong because the bug is still\n>>     there, we are just not checking for it.\n>>  2. Add a BASH prereq just for that test, or test_expect_unstable (we\n>>     would need to add extra code for both of those).\n>>  3. Add the fix, but not the test for the fix.\n>\n> I'm for this option 3: this patch does fix a bug for users of Bash\n> v4.0 or later, while it doesn't change the behavior with v3.2 or\n> earlier (and with zsh, if I understand correctly).  OTOH, the test\n> doesn't seem to be all that useful: while it does demonstrate the\n> issue, it checks only one of those callsites that passed the wrong\n> suffix, and, more importantly, it doesn't protect us from adding\n> another callsites with similarly wrong suffex in the future.\n\nYeah, I might have preferred, if we didn't read your \"doesn't seem\nto be all that useful\", to keep the test with prereq on bash 4, but\nI think either way is fine.\n\n> In any case, the commit message should note that the fix doesn't work\n> with all Bash versions and why.\n\nVery true.\n\nThanks, all.\n"},{"id":"412971","messageId":"5fe3deee26d38_7855a208ed@natae.notmuch","threadId":"54854","inReplyTo":"xmqq1rfg8e3j.fsf@gitster.c.googlers.com","subject":"Re: [PATCH try2 0/4] completion: bash: a bunch of fixes","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-24T00:21:02Z","receivedAt":"2020-12-24T00:22:01Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Junio C Hamano wrote:\n> SZEDER Gábor <szeder.dev@gmail.com> writes:\n> \n> > On Wed, Dec 23, 2020 at 07:38:12AM -0600, Felipe Contreras wrote:\n> > ...\n> >> I see 5 courses of action:\n> >> \n> >>  1. Drop the offending patch: this is wrong because the bug is still\n> >>     there, we are just not checking for it.\n> >>  2. Add a BASH prereq just for that test, or test_expect_unstable (we\n> >>     would need to add extra code for both of those).\n> >>  3. Add the fix, but not the test for the fix.\n> >\n> > I'm for this option 3: this patch does fix a bug for users of Bash\n> > v4.0 or later, while it doesn't change the behavior with v3.2 or\n> > earlier (and with zsh, if I understand correctly).  OTOH, the test\n> > doesn't seem to be all that useful: while it does demonstrate the\n> > issue, it checks only one of those callsites that passed the wrong\n> > suffix, and, more importantly, it doesn't protect us from adding\n> > another callsites with similarly wrong suffex in the future.\n> \n> Yeah, I might have preferred, if we didn't read your \"doesn't seem\n> to be all that useful\", to keep the test with prereq on bash 4, but\n> I think either way is fine.\n\nEven if we add a prereq on BASH4, he is right that the test wouldn't be\nall that useful, because it's checking only for one conditional branch,\nand the function has quite a few, from the top of my head there are about\n10.\n\nA more useuful test would at least add one check for each one of the\ncases. It still would be dependent on the current implementation, but\nwould be more useful.\n\nI can add that in a future patch series, once the other issues are\nresolved.\n\nCheers.\n\n-- \nFelipe Contreras"}]}