Re: [PATCH v10 13/14] convert: add filter.<driver>.process option

[Lars Schneider <larsxschneider@gmail.com>]

> On 10 Oct 2016, at 21:58, Junio C Hamano <gitster@pobox.com> wrote:
> 
> larsxschneider@gmail.com writes:
> 
> [...]
>> +# Count unique lines except clean invocations in two files and compare
>> +# them. Clean invocations are not counted because their number can vary.
>> +# c.f. http://public-inbox.org/git/xmqqshv18i8i.fsf@gitster.mtv.corp.google.com/
>> +test_cmp_count_except_clean () {
>> +	for FILE in $@
>> +	do
>> +		sort $FILE | uniq -c | sed "s/^[ ]*//" |
>> +			sed "s/^\([0-9]\) IN: clean/x IN: clean/" >$FILE.tmp
>> +		cat $FILE.tmp >$FILE
>> +	done &&
>> +	test_cmp $@
>> +}
> 
> Why do you even _care_ about the number of invocations?  While I
> told you why "clean" could be called multiple times under racy Git
> as an example, that was not meant to be an exhaustive example.  I
> wouldn't be surprised if we needed to run smudge twice, for example,
> in some weirdly racy cases in the future.
> 
> Can we just have the correctness (i.e. "we expect that the working
> tree file gets this as the result of checking it out, and we made
> sure that is the case") test without getting into such an
> implementation detail?

My goal is to check that clean/smudge is invoked at least once. I could
just run `uniq` to achieve that but then all other filter commands could
happen multiple times and the test would not detect that.

I also prefer to check the filter commands to ensure the filter is 
working as expected (e.g. no multiple start ups etc) in addition to 
checking the working tree.

Would the patch below work for you? If yes, then please squash it into
"convert: add filter.<driver>.process option".

Thank you,
Lars



diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index 9f892c0..714f706 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh
@@ -31,38 +31,33 @@ filter_git () {
 	rm -f git-stderr.log
 }
 
-# Count unique lines in two files and compare them.
-test_cmp_count () {
-	for FILE in $@
-	do
-		sort $FILE | uniq -c | sed "s/^[ ]*//" >$FILE.tmp
-		cat $FILE.tmp >$FILE
-	done &&
-	test_cmp $@
-}
-
-# Count unique lines except clean invocations in two files and compare
-# them. Clean invocations are not counted because their number can vary.
+# Compare two files and ensure that `clean` and `smudge` respectively are
+# called at least once if specified in the `expect` file. The actual
+# invocation count is not relevant because their number can vary.
 # c.f. http://public-inbox.org/git/xmqqshv18i8i.fsf@gitster.mtv.corp.google.com/
-test_cmp_count_except_clean () {
-	for FILE in $@
+test_cmp_count () {
+	expect=$1 actual=$2
+	for FILE in "$expect" "$actual"
 	do
-		sort $FILE | uniq -c | sed "s/^[ ]*//" |
-			sed "s/^\([0-9]\) IN: clean/x IN: clean/" >$FILE.tmp
-		cat $FILE.tmp >$FILE
+		sort "$FILE" | uniq -c | sed "s/^[ ]*//" |
+			sed "s/^\([0-9]\) IN: clean/x IN: clean/" |
+			sed "s/^\([0-9]\) IN: smudge/x IN: smudge/" >"$FILE.tmp" &&
+		cat "$FILE.tmp" >"$FILE"
 	done &&
-	test_cmp $@
+	test_cmp "$expect" "$actual"
 }
 
-# Compare two files but exclude clean invocations because they can vary.
+# Compare two files but exclude all `clean` invocations because Git can
+# call `clean` zero or more times.
 # c.f. http://public-inbox.org/git/xmqqshv18i8i.fsf@gitster.mtv.corp.google.com/
 test_cmp_exclude_clean () {
-	for FILE in $@
+	expect=$1 actual=$2
+	for FILE in "$expect" "$actual"
 	do
-		grep -v "IN: clean" $FILE >$FILE.tmp
-		cat $FILE.tmp >$FILE
+		grep -v "IN: clean" "$FILE" >"$FILE.tmp" &&
+		cat "$FILE.tmp" >"$FILE"
 	done &&
-	test_cmp $@
+	test_cmp "$expect" "$actual"
 }
 
 # Check that the contents of two files are equal and that their rot13 version
@@ -395,7 +390,7 @@ test_expect_success PERL 'required process filter should filter data' '
 			IN: clean testsubdir/test3 '\''sq'\'',\$x.r $S3 [OK] -- OUT: $S3 . [OK]
 			STOP
 		EOF
-		test_cmp_count_except_clean expected.log rot13-filter.log &&
+		test_cmp_count expected.log rot13-filter.log &&
 
 		rm -f test2.r "testsubdir/test3 '\''sq'\'',\$x.r" &&