Re* [PATCH v5 3/7] grep tests: add missing "grep.patternType" config test

Commit Message

Junio C Hamano Dec. 25, 2021, 12:06 a.m. UTC

Junio C Hamano <[e-mail]> writes:

> The grep.patternType configuration variable has the "last one wins"
> semantics just all the usual configuration variable, but the meaning
> of the variable when it is set to "default" depends on the value set
> to grep.extendedRegexp variable.
>
> If you rewrite with the above understanding, what you wrote will
> become a lot more concise.
>
>     Extend the grep tests to assert that grep.patternType is the
>     usual "last one wins" variable, and specifically, setting it to
>     "default" has the same meaning as setting it to "basic" when
>     grep.extendedRegexp is not set (or set to false).

Also, it is probably a good idea to strees that grep.extendedRegexp
is also "last one wins", so as I wrote in a separate message, Git
finds the last one for grep.extendedRegexp and grep.patternType
independently and combines these last values to come up with the
pattern type it uses.

I'll tentatively queue the following patch between your 3/7 and 4/7,
but it probably is a good idea to squash it into 3/7, as it belongs
to the same theme: clarify how these two variables are meant to
interact with each other.

----- >8 --------- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----
Subject: [PATCH] t7810: make sure grep.extendedRegexp is also last-one-wins

Signed-off-by: Junio C Hamano <[e-mail]>
---
 t/t7810-grep.sh | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Junio C Hamano Dec. 25, 2021, 1:04 a.m. UTC | #1

Junio C Hamano <[e-mail]> writes:

> I'll tentatively queue the following patch between your 3/7 and 4/7,
> but it probably is a good idea to squash it into 3/7, as it belongs
> to the same theme: clarify how these two variables are meant to
> interact with each other.

Just as I suspected earlier, up to the [PATCH 6/7] of the series
passes this test and this test reveals the breakage in [PATCH 7/7].

In the review of that step, I said "I am not sure how any change
that says we do not need the "commit" phase can be correct." but
come to think of it, it was a bit too strong.  It is possible to
implement correct semantics without the "commit" phase, as long as
we do not try to decide between basic and extended too hastily when
we see grep.patternType=default, before we are sure that we will not
see any new definition of grep.extendedRegexp [*].  In the extreme,
we could keep it as 'default' until the time just before we compile
the regexp and consult the final value of grep.extendedRegexp to
decide between the two, and such an implementation would still give
correct results without having to have a "commit" phase.


[Footnote]

* The story is the same for the `--patternType=default` command line
  option, but by the time we read the command line option, we should
  be already done with the configuration, and there is no command
  line option to override the grep.extendedRegexp configuration
  variable, so we do not have to worry about that case.  When we
  read --patternType=default from the command line, we can safely
  use the value we know about grep.extendedRegexp and decide to use
  either basic or extended.

  But that is not true for grep.patternType configuration varialble,
  as the additional test I am responding to shows.

> ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----
> Subject: [PATCH] t7810: make sure grep.extendedRegexp is also last-one-wins
>
> Signed-off-by: Junio C Hamano <[e-mail]>
> ---
>  t/t7810-grep.sh | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
> index 113902c3bd..2c17704e01 100755
> --- a/t/t7810-grep.sh
> +++ b/t/t7810-grep.sh
> @@ -451,6 +451,16 @@ do
>  		test_cmp expected actual
>  	'
>  
> +	test_expect_success "grep $L with grep.extendedRegexp is last-one-wins" '
> +		echo "${HC}ab:a+bc" >expected &&
> +		git \
> +			-c grep.extendedRegexp=true \
> +			-c grep.patternType=default \
> +			-c grep.extendedRegexp=false \
> +			grep "a+b*c" $H ab >actual &&
> +		test_cmp expected actual
> +	'
> +
>  	test_expect_success "grep $L with grep.patternType=extended and grep.patternType=default" '
>  		echo "${HC}ab:a+bc" >expected &&
>  		git \

Patch

diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 113902c3bd..2c17704e01 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -451,6 +451,16 @@  do
 		test_cmp expected actual
 	'
 
+	test_expect_success "grep $L with grep.extendedRegexp is last-one-wins" '
+		echo "${HC}ab:a+bc" >expected &&
+		git \
+			-c grep.extendedRegexp=true \
+			-c grep.patternType=default \
+			-c grep.extendedRegexp=false \
+			grep "a+b*c" $H ab >actual &&
+		test_cmp expected actual
+	'
+
 	test_expect_success "grep $L with grep.patternType=extended and grep.patternType=default" '
 		echo "${HC}ab:a+bc" >expected &&
 		git \