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

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Dec 24, 2020, 00:21 UTC
Message-ID: <5fe3deee26d38_7855a208ed@natae.notmuch>
In-Reply-To: <xmqq1rfg8e3j.fsf@gitster.c.googlers.com>

Junio C Hamano wrote:

Show 23 quoted lines

> 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