"Johannes Schindelin via GitGitGadget" <gitgitgadget@xxxxxxxxx>
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
