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

[Junio C Hamano <gitster@pobox.com>]

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?

> +run_func ()
> +{
> +	local -a COMPREPLY &&
> +	"$@" && 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)?

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

>  '
>  
>  read -r -d "" refs <<-\EOF
> @@ -586,7 +591,7 @@ test_expect_success '__gitcomp_nl - no suffix' '
>  '
>  
>  test_expect_success '__gitcomp_nl - doesnt fail because of invalid variable name' '
> -	__gitcomp_nl "$invalid_variable_name"
> +	run_func __gitcomp_nl "$invalid_variable_name"

Likewise.

>  '


Thanks.