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

["René Scharfe" <l.s.r@web.de>]

Am 06.09.22 um 15:10 schrieb Johannes Schindelin:
> Hi Junio,
>
> On Fri, 29 Jul 2022, Junio C Hamano wrote:
>
>> "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!
>
>>> +	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.

"git diff --no-index - -" also doesn't complain, by the way.

> No, you're right, I've added a guard that prevents `test-tool cmp - -`
> from failing in obscure ways.
>
>>> +	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.
>
>> 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.

Why not use "git diff --no-index --ignore-cr-at-eol"?  Do you even need
to wrap it?

René