Re: [PATCH] tests: replace mingw_test_cmp with a helper in C
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 29, 2022, 16:44 UTC
- Message-ID
- <xmqqwnbv7trp.fsf@gitster.g>
- In-Reply-To
- <pull.1309.git.1659106382128.gitgitgadget@gmail.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?
Show 6 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.
Show 11 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.
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.
Show 15 quoted lines
> 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