[PATCH try2 0/4] completion: bash: a bunch of fixes

[PATCH try2 0/4] completion: bash: a bunch of fixes

[Felipe Contreras]

This is just the bug fixes from the previous series.

These should be pretty obvious and straightforward.

Felipe Contreras (4):
  completion: bash: fix prefix detection in branch.*
  completion: bash: add correct suffix in variables
  completion: bash: fix for suboptions with value
  completion: bash: fix for multiple dash commands

 contrib/completion/git-completion.bash | 14 +++++++-------
 t/t9902-completion.sh                  | 22 ++++++++++++++++++++++
 2 files changed, 29 insertions(+), 7 deletions(-)

-- 
2.30.0.rc0

[PATCH try2 1/4] completion: bash: fix prefix detection in branch.*

[Felipe Contreras]

Otherwise we are completely ignoring the --cur argument.

The issue can be tested with:

  git clone --config=branch.<tab>

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/completion/git-completion.bash | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 7c81e4ba49..b866b68b3c 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2615,8 +2615,8 @@ __git_complete_config_variable_name ()
 		return
 		;;
 	branch.*)
-		local pfx="${cur%.*}."
-		cur_="${cur#*.}"
+		local pfx="${cur_%.*}."
+		cur_="${cur_#*.}"
 		__gitcomp_direct "$(__git_heads "$pfx" "$cur_" ".")"
 		__gitcomp_nl_append $'autoSetupMerge\nautoSetupRebase\n' "$pfx" "$cur_" "$sfx"
 		return
-- 
2.30.0.rc0

[PATCH try2 2/4] completion: bash: add correct suffix in variables

[Felipe Contreras]

__gitcomp automatically adds a suffix, but __gitcomp_nl and others
don't, we need to specify a space by default.

Can be tested with:

  git config branch.autoSetupMe<tab>

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/completion/git-completion.bash | 6 +++---
 t/t9902-completion.sh                  | 7 +++++++
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index b866b68b3c..bbdb46d87e 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2618,7 +2618,7 @@ __git_complete_config_variable_name ()
 		local pfx="${cur_%.*}."
 		cur_="${cur_#*.}"
 		__gitcomp_direct "$(__git_heads "$pfx" "$cur_" ".")"
-		__gitcomp_nl_append $'autoSetupMerge\nautoSetupRebase\n' "$pfx" "$cur_" "$sfx"
+		__gitcomp_nl_append $'autoSetupMerge\nautoSetupRebase\n' "$pfx" "$cur_" "${sfx- }"
 		return
 		;;
 	guitool.*.*)
@@ -2652,7 +2652,7 @@ __git_complete_config_variable_name ()
 		local pfx="${cur_%.*}."
 		cur_="${cur_#*.}"
 		__git_compute_all_commands
-		__gitcomp_nl "$__git_all_commands" "$pfx" "$cur_" "$sfx"
+		__gitcomp_nl "$__git_all_commands" "$pfx" "$cur_" "${sfx- }"
 		return
 		;;
 	remote.*.*)
@@ -2668,7 +2668,7 @@ __git_complete_config_variable_name ()
 		local pfx="${cur_%.*}."
 		cur_="${cur_#*.}"
 		__gitcomp_nl "$(__git_remotes)" "$pfx" "$cur_" "."
-		__gitcomp_nl_append "pushDefault" "$pfx" "$cur_" "$sfx"
+		__gitcomp_nl_append "pushDefault" "$pfx" "$cur_" "${sfx- }"
 		return
 		;;
 	url.*.*)
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 2be9190425..4a3d3d1597 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -2293,6 +2293,13 @@ test_expect_success 'git config - value' '
 	EOF
 '
 
+test_expect_success 'git config - direct completions' '
+	test_completion "git config branch.autoSetup" <<-\EOF
+	branch.autoSetupMerge Z
+	branch.autoSetupRebase Z
+	EOF
+'
+
 test_expect_success 'git -c - section' '
 	test_completion "git -c br" <<-\EOF
 	branch.Z
-- 
2.30.0.rc0

[PATCH try2 3/4] completion: bash: fix for suboptions with value

[Felipe Contreras]

We need to ignore options that don't start with -- as well.

Depending on the value of COMP_WORDBREAKS the last word could be
duplicated otherwise.

Can be tested with:

  git merge -X diff-algorithm=<tab>

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/completion/git-completion.bash |  2 +-
 t/t9902-completion.sh                  | 15 +++++++++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index bbdb46d87e..5b2dff150d 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -347,7 +347,7 @@ __gitcomp ()
 	local cur_="${3-$cur}"
 
 	case "$cur_" in
-	--*=)
+	*=)
 		;;
 	--no-*)
 		local c i=0 IFS=$' \t\n'
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 4a3d3d1597..4deda01153 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -537,6 +537,15 @@ test_expect_success '__gitcomp - expand/narrow all negative options' '
 	EOF
 '
 
+test_expect_success '__gitcomp - equal skip' '
+	test_gitcomp "--option=" "--option=" <<-\EOF &&
+
+	EOF
+	test_gitcomp "option=" "option=" <<-\EOF
+
+	EOF
+'
+
 test_expect_success '__gitcomp - doesnt fail because of invalid variable name' '
 	__gitcomp "$invalid_variable_name"
 '
@@ -2342,6 +2351,12 @@ test_expect_success 'git clone --config= - value' '
 	EOF
 '
 
+test_expect_success 'options with value' '
+	test_completion "git merge -X diff-algorithm=" <<-\EOF
+
+	EOF
+'
+
 test_expect_success 'sourcing the completion script clears cached commands' '
 	__git_compute_all_commands &&
 	verbose test -n "$__git_all_commands" &&
-- 
2.30.0.rc0

[PATCH try2 4/4] completion: bash: fix for multiple dash commands

[Felipe Contreras]

Otherwise commands like 'for-each-ref' are not completed.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/completion/git-completion.bash | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 5b2dff150d..7e893abf43 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -412,7 +412,7 @@ __gitcomp_builtin ()
 	local incl="${2-}"
 	local excl="${3-}"
 
-	local var=__gitcomp_builtin_"${cmd/-/_}"
+	local var=__gitcomp_builtin_"${cmd//-/_}"
 	local options
 	eval "options=\${$var-}"
 
-- 
2.30.0.rc0

Re: [PATCH try2 0/4] completion: bash: a bunch of fixes

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> This is just the bug fixes from the previous series.
>
> These should be pretty obvious and straightforward.
>
> Felipe Contreras (4):
>   completion: bash: fix prefix detection in branch.*
>   completion: bash: add correct suffix in variables
>   completion: bash: fix for suboptions with value
>   completion: bash: fix for multiple dash commands

It seems that this tickles some platform specific glitches in the
tests (the detailed CI report can only be seen when logged in, it
seems):

    https://github.com/git/git/runs/1597682180#step:5:35614

The expected output lines have a trailing SP each, while the actual
output lines do not, but this failure happens only on macos job and
not others.

Re: [PATCH try2 0/4] completion: bash: a bunch of fixes

[Felipe Contreras]

Junio C Hamano wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > This is just the bug fixes from the previous series.
> >
> > These should be pretty obvious and straightforward.
> >
> > Felipe Contreras (4):
> >   completion: bash: fix prefix detection in branch.*
> >   completion: bash: add correct suffix in variables
> >   completion: bash: fix for suboptions with value
> >   completion: bash: fix for multiple dash commands
> 
> It seems that this tickles some platform specific glitches in the
> tests (the detailed CI report can only be seen when logged in, it
> seems):
> 
>     https://github.com/git/git/runs/1597682180#step:5:35614

I found that output very hard to parse, but I think I understand the
issue. It's interesting.

Apaprently macOS uses zsh by default, and in zsh, this:

  local sfx
  echo "'${sfx- }'"

Prints an empty string.

That's because in zsh "local sfx" is effectively "local sfx=''" which in
my opinion is a bug.

I did catch this issue for zsh some time ago.

I contacted the zsh developers a while ago, and that triggered a huge
discussion, since they don't consider it a bug, but they are working on
tentative changes. That's another story though.

My previous series with 26 patches didn't trigger that issue because I have
fixes on top of of __gitcomp so suffixes work correctly, and the code
can eventually be changed to:

  local sfx=" "

That makes the completion work correctly for both bash and zsh.

I see 5 courses of action:

 1. Drop the offending patch: this is wrong because the bug is still
    there, we are just not checking for it.
 2. Add a BASH prereq just for that test, or test_expect_unstable (we
    would need to add extra code for both of those).
 3. Add the fix, but not the test for the fix.
 4. Backport my __gitcomp to make the code work for both shells.
 5. Update the patch series to include all the changes up to the point
    that is fixed.

For me obviously 1 and 5 require the least work, but in 1 the bug is
still there, and in 5 the patches might get stuck more time than
necessary.

Either way I think the real problem is that not enough eyes are looking
at these patches, so it's unclear if and when they will be merged.

The issues are real though, and they are present in the current code.

Cheers.

-- 
Felipe Contreras

Re: [PATCH try2 0/4] completion: bash: a bunch of fixes

[SZEDER Gábor]

On Wed, Dec 23, 2020 at 07:38:12AM -0600, Felipe Contreras wrote:
> Junio C Hamano wrote:
> > Felipe Contreras <felipe.contreras@gmail.com> writes:
> > 
> > > This is just the bug fixes from the previous series.
> > >
> > > These should be pretty obvious and straightforward.
> > >
> > > Felipe Contreras (4):
> > >   completion: bash: fix prefix detection in branch.*
> > >   completion: bash: add correct suffix in variables
> > >   completion: bash: fix for suboptions with value
> > >   completion: bash: fix for multiple dash commands
> > 
> > It seems that this tickles some platform specific glitches in the
> > tests (the detailed CI report can only be seen when logged in, it
> > seems):
> > 
> >     https://github.com/git/git/runs/1597682180#step:5:35614
> 
> I found that output very hard to parse, but I think I understand the
> issue. It's interesting.
> 
> Apaprently macOS uses zsh by default,

The completion and prompt tests are suppsed to be run by Bash, no
matter what the default shell, or are skipped, or the setup is broken
(a non-Bash shell sets $BASH, or 'exec bash' runs zsh).

Our CI jobs use the default Bash version, which in the macOS jobs
is v3.2.

> and in zsh, this:
> 
>   local sfx
>   echo "'${sfx- }'"
> 
> Prints an empty string.
> 
> That's because in zsh "local sfx" is effectively "local sfx=''" which in
> my opinion is a bug.

Bash versions up to 4.0-alpha suffered from this bug as well; I believe
the relevant changelog entry for 4.0-beta is this:

  e.  Fixed a bug that caused local variables to be created with the empty
      string for a value rather than no value.

So the default Bash version on macOS still has this bug, thus
__gitcomp_nl_append() is invoked with an empty string sfx parameter
instead of a space, causing the test failure.  I can reproduce the
test failure on Linux using Bash v3.2 (and v3.1 and v3.0), and it
passes with v4.0 and later versions.

 
> I see 5 courses of action:
> 
>  1. Drop the offending patch: this is wrong because the bug is still
>     there, we are just not checking for it.
>  2. Add a BASH prereq just for that test, or test_expect_unstable (we
>     would need to add extra code for both of those).
>  3. Add the fix, but not the test for the fix.

I'm for this option 3: this patch does fix a bug for users of Bash
v4.0 or later, while it doesn't change the behavior with v3.2 or
earlier (and with zsh, if I understand correctly).  OTOH, the test
doesn't seem to be all that useful: while it does demonstrate the
issue, it checks only one of those callsites that passed the wrong
suffix, and, more importantly, it doesn't protect us from adding
another callsites with similarly wrong suffex in the future.

In any case, the commit message should note that the fix doesn't work
with all Bash versions and why.

Re: [PATCH try2 0/4] completion: bash: a bunch of fixes

[Felipe Contreras]

SZEDER Gábor wrote:
> On Wed, Dec 23, 2020 at 07:38:12AM -0600, Felipe Contreras wrote:

> > and in zsh, this:
> > 
> >   local sfx
> >   echo "'${sfx- }'"
> > 
> > Prints an empty string.
> > 
> > That's because in zsh "local sfx" is effectively "local sfx=''" which in
> > my opinion is a bug.
> 
> Bash versions up to 4.0-alpha suffered from this bug as well; I believe
> the relevant changelog entry for 4.0-beta is this:
> 
>   e.  Fixed a bug that caused local variables to be created with the empty
>       string for a value rather than no value.
> 
> So the default Bash version on macOS still has this bug, thus
> __gitcomp_nl_append() is invoked with an empty string sfx parameter
> instead of a space, causing the test failure.  I can reproduce the
> test failure on Linux using Bash v3.2 (and v3.1 and v3.0), and it
> passes with v4.0 and later versions.

Ahhh, interesting.

So I'm not the only one who considers it a bug.

> > I see 5 courses of action:
> > 
> >  1. Drop the offending patch: this is wrong because the bug is still
> >     there, we are just not checking for it.
> >  2. Add a BASH prereq just for that test, or test_expect_unstable (we
> >     would need to add extra code for both of those).
> >  3. Add the fix, but not the test for the fix.
> 
> I'm for this option 3: this patch does fix a bug for users of Bash
> v4.0 or later, while it doesn't change the behavior with v3.2 or
> earlier (and with zsh, if I understand correctly).  OTOH, the test
> doesn't seem to be all that useful: while it does demonstrate the
> issue, it checks only one of those callsites that passed the wrong
> suffix, and, more importantly, it doesn't protect us from adding
> another callsites with similarly wrong suffex in the future.

I'm fine with that option.

> In any case, the commit message should note that the fix doesn't work
> with all Bash versions and why.

Yes. I will send a re-roll (unless somebody objects beforehand).

-- 
Felipe Contreras

Re: [PATCH try2 0/4] completion: bash: a bunch of fixes

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

> On Wed, Dec 23, 2020 at 07:38:12AM -0600, Felipe Contreras wrote:
> ...
>> I see 5 courses of action:
>> 
>>  1. Drop the offending patch: this is wrong because the bug is still
>>     there, we are just not checking for it.
>>  2. Add a BASH prereq just for that test, or test_expect_unstable (we
>>     would need to add extra code for both of those).
>>  3. Add the fix, but not the test for the fix.
>
> I'm for this option 3: this patch does fix a bug for users of Bash
> v4.0 or later, while it doesn't change the behavior with v3.2 or
> earlier (and with zsh, if I understand correctly).  OTOH, the test
> doesn't seem to be all that useful: while it does demonstrate the
> issue, it checks only one of those callsites that passed the wrong
> suffix, and, more importantly, it doesn't protect us from adding
> another callsites with similarly wrong suffex in the future.

Yeah, I might have preferred, if we didn't read your "doesn't seem
to be all that useful", to keep the test with prereq on bash 4, but
I think either way is fine.

> In any case, the commit message should note that the fix doesn't work
> with all Bash versions and why.

Very true.

Thanks, all.

Re: [PATCH try2 0/4] completion: bash: a bunch of fixes

[Felipe Contreras]

Junio C Hamano wrote:
> SZEDER Gábor <szeder.dev@gmail.com> writes:
> 
> > On Wed, Dec 23, 2020 at 07:38:12AM -0600, Felipe Contreras wrote:
> > ...
> >> I see 5 courses of action:
> >> 
> >>  1. Drop the offending patch: this is wrong because the bug is still
> >>     there, we are just not checking for it.
> >>  2. Add a BASH prereq just for that test, or test_expect_unstable (we
> >>     would need to add extra code for both of those).
> >>  3. Add the fix, but not the test for the fix.
> >
> > I'm for this option 3: this patch does fix a bug for users of Bash
> > v4.0 or later, while it doesn't change the behavior with v3.2 or
> > earlier (and with zsh, if I understand correctly).  OTOH, the test
> > doesn't seem to be all that useful: while it does demonstrate the
> > issue, it checks only one of those callsites that passed the wrong
> > suffix, and, more importantly, it doesn't protect us from adding
> > another callsites with similarly wrong suffex in the future.
> 
> Yeah, I might have preferred, if we didn't read your "doesn't seem
> to be all that useful", to keep the test with prereq on bash 4, but
> I think either way is fine.

Even if we add a prereq on BASH4, he is right that the test wouldn't be
all that useful, because it's checking only for one conditional branch,
and the function has quite a few, from the top of my head there are about
10.

A more useuful test would at least add one check for each one of the
cases. It still would be dependent on the current implementation, but
would be more useful.

I can add that in a future patch series, once the other issues are
resolved.

Cheers.

-- 
Felipe Contreras