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

["SZEDER Gábor" <szeder.dev@gmail.com>]

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.