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

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Dec 23, 2020, 14:19 UTC
Message-ID: <20201223141950.GA23264@szeder.dev>
In-Reply-To: <5fe3484465fac_198be208bf@natae.notmuch>

On Wed, Dec 23, 2020 at 07:38:12AM -0600, Felipe Contreras wrote:

Show 23 quoted lines

> 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.

Show 9 quoted lines

> 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.

Show 7 quoted lines

> 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.