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

> On Tue, Sep 06 2022, Johannes Schindelin via GitGitGadget wrote:
>
>> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>> [...]
>> +++ b/t/helper/test-text-cmp.c
>> @@ -0,0 +1,78 @@
>> +#include "test-tool.h"
>> +#include "git-compat-util.h"
>> +#include "strbuf.h"
>> +#include "gettext.h"
>
> Superflous header? Compiles without gettext.h for me (and we shouldn't
> use i18n in test helpers).
>
>> [...]
>> +int cmd__text_cmp(int argc, const char **argv)
>> +{
>> +	FILE *f0, *f1;
>> +	struct strbuf b0 = STRBUF_INIT, b1 = STRBUF_INIT;
>> +
>> +	if (argc != 3)
>> +		die("Require exactly 2 arguments, got %d", argc);
>
> Here you conflate the argc v.s. arguments minus the "text-cmp",
> resulting in:
>
> 	helper/test-tool text-cmp 2
>         fatal: Require exactly 2 arguments, got 2
>
> An argc-- argv++ at the beginning seems like the easiest way out of
> this. Also s/Require/require/ per CodingGuidelines.
>
>> +	if (!strcmp(argv[1], "-") && !strcmp(argv[2], "-"))
>> +		die("only one parameter can refer to `stdin` but not both");
>> +
>> +	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]);
>> +	}
>
> Faithfully emulating the old version. I do wonder if we couldn't simply
> adjust the handful of tests that actually make use of the "-" diff(1)
> feature. AFAICT there's around 10 of those at most, and they all seem
> like cases where it would be easy to change:
>
> 	(echo foo) | test_cmp - actual
>
> Or whatever, to:
>
> 	echo foo >expected &&
> 	test_cmp expected actual
>
> ...
>
>> +			if (!strcmp(argv[1], "-") || !strcmp(argv[2], "-"))
>> +				warning("cannot show diff because `stdin` was already consumed");
>
> ...
>
> Which means we wouldn't need to punt on this.
>
>> +			else if (!run_diff(argv[1], argv[2]))
>> +				die("Huh? 'diff --no-index %s %s' succeeded",
>> +				    argv[1], argv[2]);
>
> I tried manually testing this with:
>
> 	GIT_TRACE=1 GIT_TEST_CMP="/home/avar/g/git/git diff --no-index --" ./t0021-conversion.sh  -vixd
>
> v.s.:
>
> 	GIT_TRACE=1 GIT_TEST_CMP="$PWD/helper/test-tool text-cmp" ./t0021-conversion.sh  -vixd
>
> Your version doesn't get confused by the same, but AFAICT this is by
> fragile accident.
>
> I.e. you run your own equivalent of "cmp", so because the files are the
> same in that case we don't run the "diff --no-index".
>
> But the "diff --no-index" in that t0021*.sh case *would* report
> differences, even though the files are byte-for-byte identical.
>
> So the "cmp"-a-like here isn't just an optimization to avoid forking the
> "git diff" process, it's an entirely different comparison method in
> cases where we have a "filter".
>
> It just so happens that our test suite doesn't currently combine them in
> a way that causes a current failure.