Re: [PATCH v11 00/10] grep: simplify & delete "init" & "config" code

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

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

>   * A new 3/10 and 4/10 hopefully address the comments about the test
>     code. I ended up just adding a helper to reduce the existing and
>     new verbosity of the tests, which should make it easier to reason
>     about them.

Here is an excerpt plus comments on "git diff @{1}" after queuing
the new series.  I found only a few minor "Huh?", none of them a
huge deal.

Will queue; thanks.

+test_pattern_type () {
+	H=$1 &&
+	HC=$2 &&
+	L=$3 &&
+	type=$4 &&
+	shift 4 &&
+
+	expected_str= &&
+	case "$type" in
+	BRE)
+		expected_str="${HC}ab:a+bc"
+		;;
+	ERE)
+		expected_str="${HC}ab:abc"
+		;;
+	FIX)
+		expected_str="${HC}ab:a+b*c"
+		;;
+	*)
+		BUG "unknown pattern type '$type'"
+		;;
+	esac &&

Excellent.  I always had to think twice when commenting on earlier
rounds of the patches which expected strings corresponded to what
pattern type.  Now we have a clearly defined table.

+	config_str="$@" &&

This forces each element of $@ to lose its identity, and makes it a
single string separated by a whitespace, so it is less misleading to
write

	config_str="$*"

instead, but it is not a huge deal.

+	test_expect_success "grep $L with '$config_str' interpreted as $type" '
+		echo $expected_str >expected &&
+		git $config_str grep "a+b*c" $H ab >actual &&
+		test_cmp expected actual
+	'

We must leave $config_str unquoted (because we do want the string
split at $IFS), but not quoting "$expected_str" looks a bit yucky
(because we have no intention to let $IFS to split it).

+}
+

+	test_pattern_type "$H" "$HC" "$L" BRE -c grep.extendedRegexp=false
+	test_pattern_type "$H" "$HC" "$L" ERE -c grep.extendedRegexp=true
+	test_pattern_type "$H" "$HC" "$L" BRE -c grep.patternType=basic
+	test_pattern_type "$H" "$HC" "$L" ERE -c grep.patternType=extended
+	test_pattern_type "$H" "$HC" "$L" FIX -c grep.patternType=fixed
 
This part demonstrates the beauty of the new helper very well ;-)

s/FIX/FIXED/ would be more grammatically correct.  It would break
alignment above and I suspect that was why the patch chose to say
"FIX" instead, but I am not sure if the alignment here is so
valuable.

+	test_pattern_type "$H" "$HC" "$L" ERE \
+		-c grep.patternType=default \
+		-c grep.extendedRegexp=true
+	test_pattern_type "$H" "$HC" "$L" ERE \
+		-c grep.extendedRegexp=true \
+		-c grep.patternType=default
+	test_pattern_type "$H" "$HC" "$L" ERE \
+		-c grep.patternType=extended \
+		-c grep.extendedRegexp=false
+	test_pattern_type "$H" "$HC" "$L" BRE \
+		-c grep.patternType=basic \
+		-c grep.extendedRegexp=true
+	test_pattern_type "$H" "$HC" "$L" ERE \
+		-c grep.extendedRegexp=false \
+		-c grep.patternType=extended
+	test_pattern_type "$H" "$HC" "$L" BRE \
+		-c grep.extendedRegexp=true \
+		-c grep.patternType=basic

OK.  A bit redundant, knowing the implementation that parses the two
variables independently just like any other two variables, but these
are correct, which counts even more ;-).

+	# grep.extendedRegexp is last-one-wins
+	test_pattern_type "$H" "$HC" "$L" BRE \
+		-c grep.extendedRegexp=true \
+		-c grep.extendedRegexp=false
+
+	# grep.patternType=basic pays no attention to grep.extendedRegexp
+	test_pattern_type "$H" "$HC" "$L" BRE \
+		-c grep.extendedRegexp=true \
+		-c grep.patternType=basic \
+		-c grep.extendedRegexp=false
+
+	# grep.patternType=extended pays no attention to grep.extendedRegexp
+	test_pattern_type "$H" "$HC" "$L" ERE \
+		-c grep.extendedRegexp=true \
+		-c grep.patternType=extended \
+		-c grep.extendedRegexp=false

All correct.

+	# grep.extendedRegexp is used with a last-one-wins grep.patternType=default
+	test_pattern_type "$H" "$HC" "$L" ERE \
+		-c grep.patternType=fixed \
+		-c grep.extendedRegexp=true \
+		-c grep.patternType=default

Nice.

+	# grep.extendedRegexp is used with earlier grep.patternType=default
+	test_pattern_type "$H" "$HC" "$L" ERE \
+		-c grep.extendedRegexp=false \
+		-c grep.patternType=default \
+		-c grep.extendedRegexp=true

OK.  I would have expected "the last" instead of "earlier".  Because
these two variables are independently "the last one wins", just like
any two variables that are "the last one wins", the relative order
of their appearance does not matter.

+	# grep.extendedRegexp is used with a last-one-loses grep.patternType=default
+	test_pattern_type "$H" "$HC" "$L" ERE \
+		-c grep.extendedRegexp=false \
+		-c grep.extendedRegexp=true \
+		-c grep.patternType=default

I am not sure what last-one-loses mean here.  Both variables are
independently last-one-wins, so at the end of the parsing,
grep.extendedRegexp has 'true' (because it is the last value seen
for the variable) while grep.patternType has 'default' (again, it is
the last value seen for the variable).

+	# grep.extendedRegexp and grep.patternType are both last-one-wins independently
+	test_pattern_type "$H" "$HC" "$L" BRE \
+		-c grep.patternType=default \
+		-c grep.extendedRegexp=true \
+		-c grep.patternType=basic

The title of this one gets the gist of the mistakes in the code in
earlier rounds.

+	# grep.patternType=extended and grep.patternType=default
+	test_pattern_type "$H" "$HC" "$L" BRE \
+		-c grep.patternType=extended \
+		-c grep.patternType=default
+
+	# grep.patternType=[extended -> default -> fixed] (BRE)" '
+	test_pattern_type "$H" "$HC" "$L" FIX \
+		-c grep.patternType=extended \
+		-c grep.patternType=default \
+		-c grep.patternType=fixed
 
OK.

 	test_expect_success "grep --count $L" '
 		echo ${HC}ab:3 >expected &&