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

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

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