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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Sep 6, 2022, 13:10 UTC
Message-ID: <354qp59q-r4r3-1971-5o09-71q224911orp@tzk.qr>
In-Reply-To: <xmqqwnbv7trp.fsf@gitster.g>

Hi Junio,

On Fri, 29 Jul 2022, Junio C Hamano wrote:

Show 8 quoted lines

> "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?

Yes!

Show 13 quoted lines

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

No, you're right, I've added a guard that prevents `test-tool cmp - -` from failing in obscure ways.

Show 22 quoted lines

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

Right. I've added a clause that says that we cannot show the diff because `stdin` has been consumed already.

Show 21 quoted lines

> 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.

Fair point. The Unix tool `cmp` does not care about line endings at all, so when you come from a Unix background you will expect the same to be true for `test-tool cmp`.

On the other hand, you will expect the same to be true for `test_cmp`, too, which is not the case, and the root cause of why I had to come up with 32ed3314c10 (t5351: avoid using `test_cmp` for binary data, 2022-07-29).

Having said that, I agree that the test tool name should reflect better what the subcommand does.

I do dislike the proposed name `mingwcmp`. Not only because it is misleading, as the purpose is not to compare MINGW-specific files but instead the purpose is to compare text files (and, in fact, the tool works just fine on Linux and macOS, too). But also because it would contribute to just how much of a second-class citizen the MINGW-based build is in Git land: From choosing to implement large parts, including the entire test suite as well as the performance benchmarks, in POSIX scripts (which plays to Windows' weaknesses in a big way) to massively favoring spawned processes over multi-threading (which plays to Linux' strengths and to Windows' weaknesses), to a still-inherent assumption that the underlying filesystem is case-sensitive (think: branch names), to an implicit agreement in the core Git community that patch contributions need not take care of working well on Windows (but that that's the job "of Windows folk" instead). This is kind of at odds with the fact that we must assume that half of Git's users are Windows-based (we can only assume, based on surveys, because we successfully avoid any kind of even opt-in telemetry that would give us hard data). I definitely want to stay away from making that second-citizenry even worse.

So I am going with the name `test-tool text-cmp` instead.

Thank you for your review, Dscho

Show 19 quoted lines

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