Re: [PATCH v6 7/7] grep: simplify config parsing and option parsing

Ævar Arnfjörð Bjarmason  [off-list ref] writes:

I very much like the lossage of redundant fixed and pcre2 members.

As I kept telling you, we still need a separate bit to keep track of
the last value of grep.extendedRegexp, but the primary mechanism to
determine what pattern type to use should be a single enum that is
pattern_type.  When we see "fixed", "pcre", "-G", etc. from
grep.patternType config or from command line, we can stuff their
enum values in pattern_type member of this struct, and when we see
"default", we need to leave "default" in pattern_type member until
we see the last definition of grep.extendedRegexp, at which time
we can turn it into either "basic" or "extended".

So having only two members is absolutely the right thing to do.

But this part convinces me that whatever this patch does, it will
not possible be capable of doing the right thing.  You cannot
implement "we have to remember that the last grep.patternType we saw
was DEFAULT and in that case we cannot decide the real pattern type
until we see the last definition of grep.extendedRegexp, which may
be well after we saw the last grep.patternType definition" without a
value in this enum to express that the last value we saw was DEFAULT.

 enum grep_pattern_type {
-	GREP_PATTERN_TYPE_UNSPECIFIED = 0,
-	GREP_PATTERN_TYPE_BRE,
+	GREP_PATTERN_TYPE_BRE = 0,
 	GREP_PATTERN_TYPE_ERE,
 	GREP_PATTERN_TYPE_FIXED,
 	GREP_PATTERN_TYPE_PCRE

In other words, this lossage is likely wrong.  Let's keep reading
and see how well the config reader in this patch does. 

The above does not look correct at all.

What happens when the configuration parser sees these configuration
variables in this sequence:

 - grep.patternType set to say "pcre" (or anything not "default").
 - grep.extendedRegexp set to "true".
 - grep.patternType set to "default".

After these three variable definitions with the usual "last one
wins" (for each variable independently), the last value for the
grep.patternType variable is "default", and the last value for
the grep.extendedRegexp variable is "true".  The user wants to use
the ERE patterns.

The way the above code would work on this three variable definition
sequence, as far as I read it, would however not give us the desired
behaviour.  First we drop extended_regexp_option member to -1 while
setting PCRE to attern_type_option member, and then grep.extendedRegexp
is totally ignored, and then we see patterntype set to default and
notice extended_regexp_option is *NOT* 1 (because you ignored it and
left it to -1), and end up using BRE, no?

I agree 100% with the direction that .fixed and .pcre2 members that
were added over time to the struct are redundant and it is a very
good idea to get rid of them.  But we need to keep track of two
configuration variables separately to allow them the "last one wins"
semantics independently, and for that, you cannot lose the "default"
value from the enum.  It is impossible not to store the fact that
"default" was the last value so far we saw for grep.patternType
because you do not know, at the point of seeing "default", what the
final value for grep.extendedRegexp will be.  If you want to correctly
implement the interaction between two variables without regression,
that is.

Junio C Hamano [off-list ref] writes:

The above does not look correct at all.

What happens when the configuration parser sees these configuration
variables in this sequence:

 - grep.patternType set to say "pcre" (or anything not "default").
 - grep.extendedRegexp set to "true".
 - grep.patternType set to "default".

After these three variable definitions with the usual "last one
wins" (for each variable independently), the last value for the
grep.patternType variable is "default", and the last value for
the grep.extendedRegexp variable is "true".  The user wants to use
the ERE patterns.

By the way, the example I gave you for the previous round, and
similarly the one in the message I am responding to were all written
to help you realize that it is simply a broken approach if we do not
keep "default" as default and instead resolve it to either "basic"
or "extended" too early.  The goal of these examples was *NOT* to
tell you "this single thing is broken with the code in this round so
let's fix it".

It seems I am not succeeding in conveying that point, and specially
I smell that in the change between v5 and v6.

So let me try to be a bit more explicit.  Let's not do another round
of "I think this is a moral equivalent of what you want, even though
it is not done the way you suggested." I think we wasted a reroll or
three with that attitude in changes leading to v6 already, after I
gave my review to v5, and I think the v5 review essentially was a
repeat of my review for v3's 3/7, so if I conveyed the point clearly
enough back then, perhaps we didn't have to waste your time on v4
and v5, either.  Sorry about that.

So, here is what this step of the series SHOULD do:

 * Use two members to keep track of the final configuration value we
   saw for grep.patternTYpe and grep.extendedRegexp independently.
   The existing .fixed and .pcre2 fields are superfluous.  But no
   more "ah, we see patternType so let's ignore extendedRegexp"
   games.

 * When parsing the command line options -G, -E, etc., update the
   .patternType member with the value found.  We do not want to and
   need to touch .extendedRegexp member, whose SOLE purpose should
   be to keep track of "what the last value we saw for
   grep.extendedRegexp configuration variable".

 * Do ALL THE ABOVE while keeping "default" in the .patternType
   member as "default" as-is given by the user; do not turn it into
   "basic" or "extended" in config callback at all.

 * At some point of your choice between the time we finished parsing
   both configuration variables and command line options and the
   time we compile the pattern string to regexp objects of various
   types, look at the .patternType member and resolve it into
   basic/extended IFF it is set to default, using .extendedRegexp
   member (for this to work correctly, it is important not to let
   -E/-G command like options to touch .extendedRegexp member---it
   should be used ONLY to keep track of "what the last value we saw
   for grep.extendedRegexp configuration variable").

 * After the above step is done, .extendedRegexp member is no longer
   needed and we can compile the pattern using only the value in
   .patternType member.

The penultimate bullet point gives us a wiggle room to lose the
"commit" thing and delay it until the very last moment, the function
that decides to call which regexp engine's regcomp.  The important
thing is that we cannot lose the value "default" from .patternType
field or lose the last value given to .extendedRegexp field too
early, namely, before we have read all the configurtion streams and
know the last value for these two variables.

Thanks.  Hopefully I was clear enough this time.

----- >8 ---- ----- >8 ---- ----- >8 ---- ----- >8 ---- ----- >8 -----
Subject: [PATCH] fixup! grep tests: add missing "grep.patternType" config tests

---
 t/t7810-grep.sh | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 664f884e12..2e2829ee55 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh

@@ -471,6 +471,16 @@ do
 		test_cmp expected actual
 	'
 
+	test_expect_success "grep $L with grep.extendedRegexp and grep.patternType are both last-one-wins independently" '
+		echo "${HC}ab:abc" >expected &&
+		git \
+			-c grep.patternType=fixed \
+			-c grep.extendedRegexp=true \
+			-c grep.patternType=default \
+			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 \

-- 
2.34.1-568-g69e9fd72b5