Re: [PATCH v5 01/10] t/t4209-log-pickaxe: Naming typo: -G takes a regex
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 13, 2025, 04:06 UTC
- Message-ID
- <xmqqbjv6qyiv.fsf@gitster.g>
- In-Reply-To
- <20250212032657.1807939-2-illia.bobyr@gmail.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.
Show 15 quoted lines
> 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.