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

[Felipe Contreras <felipe.contreras@gmail.com>]

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