Re: [PATCH v5 01/10] t/t4209-log-pickaxe: Naming typo: -G takes a regex

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

Illia Bobyr <illia.bobyr@gmail.com> writes:

> Subject: Re: [PATCH v5 01/10] t/t4209-log-pickaxe: Naming typo: -G takes a regex

"Naming" -> "naming".  It is even more preferable if you can avoid
introducing the second colon.  E.g.

    Subject: t/t4209: call the value given to -G <regex>, not <string>

This applies to the titles of all the patches in the series, I suspect?

> Not effect on the test logic, but as "-G" argument is a regex it is more
> accurate to use "regex" as a dummy argument value rather than "string".
> In all the other case when "-G" is passed a dummy value it is spelled as
> "regex" rather than as "string".

I guess the -G tests are copied-and-pasted from existing tests for
the -S option when the -G option was introduced much later.  This
makes me wonder if we try to see what happens when a malformed
regular expression is fed to the -G option (I didn't check---if we
have no test for it, we might want to add one).

> ---

Missing sign-off.

Having said all that, I'd prefer to see the changes in one topic
focused only to give --patch-grep and --patch-modifies synonyms
to the existing option and do nothing else.  Certainly a change
like this is a distraction we do not have to discuss at the same
time.

If you can group all these "preliminary clean-up" changes together
into a separate series, without including any change to add the
longhand to -S/-G, that is also a viable alternative approach.  Once
such a series graduates to 'master', then you'd do the longhand on
top.

What we do not want is to see changes that are not directly
necessary to add the longhand intermixed in the same series.

>  t/t4209-log-pickaxe.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh
> index a675ac..ed70c 100755
> --- a/t/t4209-log-pickaxe.sh
> +++ b/t/t4209-log-pickaxe.sh
> @@ -89,7 +89,7 @@ test_expect_success 'usage: --no-pickaxe-regex' '
>  	test_expect_code 128 git log -Sstring --no-pickaxe-regex 2>actual &&
>  	test_cmp expect actual &&
>  
> -	test_expect_code 128 git log -Gstring --no-pickaxe-regex 2>err &&
> +	test_expect_code 128 git log -Gregex --no-pickaxe-regex 2>err &&
>  	test_cmp expect actual
>  '

Thanks.