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

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

larsxschneider@gmail.com writes:

> +# 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

Unquoted references to $FILE bothers me.  Are you relying on them
getting split at IFS boundaries?  Otherwise write this (and other
similar ones) like so:

	for FILE in "$@"
	do
		do-this-to "$FILE" | ... >"$FILE.tmp" &&
		cat "$FILE.tmp" >"$FILE" &&
		rm -f "$FILE.tmp"

> +	done &&
> +	test_cmp $@

The use of "$@" here is quite pointless, as you _know_ all of them
are filenames, and you _know_ that test_cmp takes only two
filenames.  Be explicit and say

	test_cmp "$1" "$2"

or even

	test_cmp_count () {
	expect=$1 actual=$2
	for FILE in "$expect" "$actual"
	do
		...
	done &&
	test_cmp "$expect" "$actual"

> +# 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?

Thanks.