Re: [PATCH] tests: replace mingw_test_cmp with a helper in C

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

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> +	const char *argv[] = {
> +		"diff", "--no-index", NULL, NULL, NULL
> +	};

Don't we want to have "--" before the two paths?

> +	if (!(f0 = !strcmp(argv[1], "-") ? stdin : fopen(argv[1], "r")))
> +		return error_errno("could not open '%s'", argv[1]);
> +	if (!(f1 = !strcmp(argv[2], "-") ? stdin : fopen(argv[2], "r"))) {
> +		fclose(f0);
> +		return error_errno("could not open '%s'", argv[2]);
> +	}

It is tricky that you need to take "-" and treat it as the standard
input stream in either argv[1] or argv[2] (but not both).  If would
be a different story in an end-user facing program, but because this
is a test helper, feeding wrong input is developer's fault, and I do
not mind lack of attention to detail of error checking to make sure
we avoid comparing alternating lines of the standard input.

> +	for (;;) {
> +		int r0 = strbuf_getline(&b0, f0);
> +		int r1 = strbuf_getline(&b1, f1);
> +
> +		if (r0 == EOF) {
> +			fclose(f0);
> +			fclose(f1);
> +			strbuf_release(&b0);
> +			strbuf_release(&b1);
> +			if (r1 == EOF)
> +				return 0;

If both hit the EOF at the same time, we know they are the same, OK.

> +cmp_failed:
> +			if (!run_diff(argv[1], argv[2]))

If one of argv[] was "-", then this wouldn't work correctly, as the
other file is read from the beginning but the "-" side have consumed
the initial part of the input and we cannot unseek it.  This bug
needs to be fixed only if we expect a useful and reliable output
from the helper.

But otherwise the idea is sound.  We compare them line by line,
using strbuf_getline() to ignore differences in CRLF and LF that
originates at 4d715ac0 (Windows: a test_cmp that is agnostic to
random LF <> CRLF conversions, 2013-10-26).  Only when we find the
input different, we use "git diff --no-index" to make the difference
(and unfortunately more, as it does not ignore CRLF <> LF
differences) visible.

> +				die("Huh? 'diff --no-index %s %s' succeeded",
> +				    argv[1], argv[2]);

Nice attention to (possibly irrelevant) detail here.  I would have
ignored the return value and reported "they are different" at this
point, though.  The line-by-line comparison we did was the
authoritative one, and "git diff --no-index" is merely used for
human readable output.

In any case, "test-tool mingwcmp" would be a better name that
highlights the spirit of 4d715ac0 to ignore CRLF <> LF issues.  IOW,
it does a lot more than "cmp" replacement, and we shouldn't mislead
users/developers into thinking it is a plain "cmp" replacement.

Thanks.

> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 7726d1da88a..220c259e796 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -1546,7 +1546,7 @@ case $uname_s in
>  	test_set_prereq SED_STRIPS_CR
>  	test_set_prereq GREP_STRIPS_CR
>  	test_set_prereq WINDOWS
> -	GIT_TEST_CMP=mingw_test_cmp
> +	GIT_TEST_CMP="test-tool cmp"
>  	;;
>  *CYGWIN*)
>  	test_set_prereq POSIXPERM
>
> base-commit: 23b219f8e3f2adfb0441e135f0a880e6124f766c