From: Junio C Hamano <gitster@pobox.com>
To: "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Johannes Schindelin <johannes.schindelin@gmx.de>
Subject: Re: [PATCH] tests: replace mingw_test_cmp with a helper in C
Date: Fri, 29 Jul 2022 09:44:58 -0700 [thread overview]
Message-ID: <xmqqwnbv7trp.fsf@gitster.g> (raw)
In-Reply-To: <pull.1309.git.1659106382128.gitgitgadget@gmail.com> (Johannes Schindelin via GitGitGadget's message of "Fri, 29 Jul 2022 14:53:01 +0000")
"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?
> + 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
next prev parent reply other threads:[~2022-07-29 16:45 UTC|newest]
Thread overview: 58+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-07-29 14:53 [PATCH] tests: replace mingw_test_cmp with a helper in C Johannes Schindelin via GitGitGadget
2022-07-29 14:54 ` Johannes Schindelin
2022-07-29 16:44 ` Junio C Hamano [this message]
2022-09-06 13:10 ` Johannes Schindelin
2022-09-07 12:09 ` René Scharfe
2022-09-07 16:25 ` Junio C Hamano
2022-09-07 21:45 ` Re* " Junio C Hamano
2022-09-07 22:39 ` René Scharfe
2022-09-08 0:03 ` Junio C Hamano
2022-09-08 8:59 ` René Scharfe
2022-09-08 15:26 ` Ævar Arnfjörð Bjarmason
2022-09-08 20:54 ` Johannes Schindelin
2022-09-08 21:09 ` Junio C Hamano
2022-09-06 13:10 ` [PATCH v2 0/2] " Johannes Schindelin via GitGitGadget
2022-09-06 13:10 ` [PATCH v2 1/2] t0021: use Windows-friendly `pwd` Johannes Schindelin via GitGitGadget
2022-09-06 13:10 ` [PATCH v2 2/2] tests: replace mingw_test_cmp with a helper in C Johannes Schindelin via GitGitGadget
2022-09-07 11:57 ` Ævar Arnfjörð Bjarmason
2022-09-07 12:24 ` Ævar Arnfjörð Bjarmason
2022-09-07 19:45 ` Junio C Hamano
2022-09-07 9:04 ` [PATCH v2 0/2] " Johannes Schindelin
2022-11-12 22:07 ` [PATCH v3 " Johannes Schindelin via GitGitGadget
2022-11-12 22:07 ` [PATCH v3 1/2] t0021: use Windows-friendly `pwd` Johannes Schindelin via GitGitGadget
2022-11-12 22:07 ` [PATCH v3 2/2] tests(mingw): avoid very slow `mingw_test_cmp` Johannes Schindelin via GitGitGadget
2022-11-13 4:51 ` Taylor Blau
2022-11-14 13:34 ` Johannes Schindelin
2022-11-18 23:15 ` Junio C Hamano
2022-11-19 2:53 ` Taylor Blau
2022-11-19 12:03 ` Ævar Arnfjörð Bjarmason
2022-11-19 8:18 ` Johannes Sixt
2022-11-19 17:50 ` René Scharfe
2022-11-20 9:29 ` Torsten Bögershausen
2022-11-21 17:49 ` Johannes Sixt
2022-11-21 3:13 ` Junio C Hamano
2022-11-14 9:53 ` Phillip Wood
2022-11-14 13:47 ` Johannes Schindelin
2022-11-14 11:55 ` Ævar Arnfjörð Bjarmason
2022-11-14 14:02 ` Johannes Schindelin
2022-11-14 15:23 ` Ævar Arnfjörð Bjarmason
2022-11-18 23:19 ` Junio C Hamano
2022-11-19 2:56 ` Taylor Blau
2022-11-19 11:54 ` Ævar Arnfjörð Bjarmason
2022-11-21 3:17 ` Junio C Hamano
2022-11-14 14:06 ` [PATCH v4 0/2] tests(mingw): avoid super-slow mingw_test_cmp Johannes Schindelin via GitGitGadget
2022-11-14 14:06 ` [PATCH v4 1/2] t0021: use Windows-friendly `pwd` Johannes Schindelin via GitGitGadget
2022-11-14 14:06 ` [PATCH v4 2/2] tests(mingw): avoid very slow `mingw_test_cmp` Johannes Schindelin via GitGitGadget
2022-11-14 22:40 ` Taylor Blau
2022-11-18 13:32 ` Johannes Schindelin
2022-11-18 18:14 ` Taylor Blau
2022-11-20 23:36 ` Johannes Schindelin
2022-11-21 0:07 ` Taylor Blau
2022-12-06 15:07 ` [PATCH v5 0/2] tests(mingw): avoid super-slow mingw_test_cmp Johannes Schindelin via GitGitGadget
2022-12-06 15:07 ` [PATCH v5 1/2] t0021: use Windows-friendly `pwd` Johannes Schindelin via GitGitGadget
2022-12-06 15:07 ` [PATCH v5 2/2] tests(mingw): avoid very slow `mingw_test_cmp` Johannes Schindelin via GitGitGadget
2022-12-06 18:55 ` Ævar Arnfjörð Bjarmason
2022-12-06 21:52 ` Johannes Sixt
2022-12-06 21:54 ` René Scharfe
2022-12-07 4:33 ` Junio C Hamano
2022-12-07 1:31 ` Taylor Blau
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqwnbv7trp.fsf@gitster.g \
--to=gitster@pobox.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=johannes.schindelin@gmx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).