Re: [PATCH v2 06/26] test: completion: add run_func() helper

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Nov 11, 2020, 11:43 UTC
Message-ID: <CAMP44s0XA6QjOZTJaC5CK9Rp9ySfoH9_rJu-AoEGgprstzprfw@mail.gmail.com>
In-Reply-To: <xmqq1rh05p6f.fsf@gitster.c.googlers.com>

On Wed, Nov 11, 2020 at 1:27 AM Junio C Hamano <gitster@pobox.com> wrote:

Show 11 quoted lines

>
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
> > Pretty straightforward: runs functions.
>
> Hmph, sorry but this is not straight-forward at least to me.  Yes,
> the helper runs whatever is given on the command line, but then it
> does "print_comp", too.  And the proposed log message is not
> entirely clear on the most important thing: why?
>
> What is this "helper" meant to help?  Reduce repetition?

Well, I thought the "helper" part of the title made it obvious: the helper function does help in not having to type the same code over and over. But there's in fact multiple benefits.

1. It makes the code more consistent. Everything in the test script
either calls a test_ function, or a run_ function, except the code
that is testing the functions directly.

2. It reduces code (obvious in a helper function), as the same
__gitcomp* && print_comp is executed over and over, with zero
variation.

3. It makes the code more maintainable (I also thought this was
obvious); if we want to add something we don't have to do it dozens of
times, we just do it on the helper function.

Is this enough of a "why"?

It is in fact number 3 the one I'm after, and a line that shouldn't be part of this patch was smuggled in, so perhaps that's why future patches don't obviate the need for this one.

But even with no other reason for it, the patch stands on its own.

> > +run_func ()
> > +{
> > +     local -a COMPREPLY &&

This is the line that was smuggled in. It should be part of a separate patch, since this is behavior change.

Show 19 quoted lines

> > +     "$@" && print_comp
> > +}
> > +
> >  # Test high-level completion
> >  # Arguments are:
> >  # 1: typed text so far (cur)
> > @@ -452,8 +458,7 @@ test_expect_success '__gitcomp_direct - puts everything into COMPREPLY as-is' '
> >       EOF
> >       (
> >               cur=should_be_ignored &&
> > -             __gitcomp_direct "$(cat expected)" &&
> > -             print_comp
> > +             run_func __gitcomp_direct "$(cat expected)"
>
> This is an no-op rewrite, as we used to do the gitcomp-direct
> followed by print-comp, which is exactly what the helper does.  So
> the helper does reduce repetition, which by itself would be a good
> thing but is there other benefit we are trying to seek (there does
> not have to be any)?

It's not exactly a no-op, since I cleared COMPREPLY. That should be done in a separate patch so it's truly a no-op.

It is the clearing of COMPREPLY that I'm eventually interested in. First; that's how the testing framework is supposed to work: test #1 should not interfere with test #2, but second; once the gitcomp functions are changed to append code instead of clearing COMPREPLY by themselves and then appending, this prevents the tests from failing.

Show 7 quoted lines

> >  test_expect_success '__gitcomp - doesnt fail because of invalid variable name' '
> > -     __gitcomp "$invalid_variable_name"
> > +     run_func __gitcomp "$invalid_variable_name"
>
> This one changes the behaviour in that it starts running print_comp
> which we didn't run.  It may be a good thing and improvement, but
> then we'd better advertise it in the proposed log message.

But nothing is done with the output; the behavior doesn't change. The test still passes or fails irrespective of what print_comp does.

  test_expect_success 'test 1' 'true'
  test_expect_success 'test 2' ': echo foobar'
  test_expect_success 'test 3' 'echo foobar > /dev/null'

These three tests may do different things, but their behavior is the same, that is to say: with the same input they generate the same output.

Do you want me to add: "In two places we generate an output that didn't exist before, but nothing ever reads it." ?

Cheers.

-- 
Felipe Contreras