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

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

On Wed, Nov 11, 2020 at 1:27 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> 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.

> > +     "$@" && 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.

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