{"thread":{"id":"58833","subject":"[PATCH] t3920: don't ignore errors of more than one command with `|| true`","startedAt":"2022-11-21T17:58:20Z","lastAt":"2022-12-05T10:43:47Z","messageCount":25,"participants":["Johannes Sixt","René Scharfe","Junio C Hamano","Philippe Blain","Ævar Arnfjörð Bjarmason","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"467699","messageId":"febcfb0a-c410-fb71-cff9-92acfcb269e2@kdbg.org","threadId":"58833","inReplyTo":null,"subject":"[PATCH] t3920: don't ignore errors of more than one command with `|| true`","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2022-11-21T17:58:13Z","receivedAt":"2022-11-21T17:58:20Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"It is customary to write `A || true` to ignore a potential error exit of\ncommand A. But when we have a sequence `A && B && C || true && D`, then\na failure of any of A, B, or C skips to D right away. This is not\nintended here. Turn the command whose failure is to be ignored into a\ncompound command to ensure it is the only one that is allowed to fail.\n\nSigned-off-by: Johannes Sixt \n---\n t/t3920-crlf-messages.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t3920-crlf-messages.sh b/t/t3920-crlf-messages.sh\nindex 4c661d4d54..a58522c163 100755\n--- a/t/t3920-crlf-messages.sh\n+++ b/t/t3920-crlf-messages.sh\n@@ -12,7 +12,7 @@ create_crlf_ref () {\n \tcat >.crlf-orig-$branch.txt &&\n \tcat .crlf-orig-$branch.txt | append_cr >.crlf-message-$branch.txt &&\n \tgrep 'Subject' .crlf-orig-$branch.txt | tr '\\n' ' ' | sed 's/[ ]*$//' | tr -d '\\n' >.crlf-subject-$branch.txt &&\n-\tgrep 'Body' .crlf-message-$branch.txt >.crlf-body-$branch.txt || true &&\n+\t{ grep 'Body' .crlf-message-$branch.txt >.crlf-body-$branch.txt || true; } &&\n \tLIB_CRLF_BRANCHES=\"${LIB_CRLF_BRANCHES} ${branch}\" &&\n \ttest_tick &&\n \thash=$(git commit-tree HEAD^{tree} -p HEAD -F .crlf-message-${branch}.txt) &&\n-- \n2.36.0.137.geb37740430\n"},{"id":"467721","messageId":"eed47577-1e47-2fbb-6344-fd4ae324f6b6@web.de","threadId":"58833","inReplyTo":"febcfb0a-c410-fb71-cff9-92acfcb269e2@kdbg.org","subject":"Re: [PATCH] t3920: don't ignore errors of more than one command with `|| true`","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-11-21T22:56:37Z","receivedAt":"2022-11-21T22:56:45Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 21.11.22 um 18:58 schrieb Johannes Sixt:\n> It is customary to write `A || true` to ignore a potential error exit of\n> command A. But when we have a sequence `A && B && C || true && D`, then\n> a failure of any of A, B, or C skips to D right away. This is not\n> intended here. Turn the command whose failure is to be ignored into a\n> compound command to ensure it is the only one that is allowed to fail.\n\nGood catch!\n\n> Signed-off-by: Johannes Sixt \n> ---\n> t/t3920-crlf-messages.sh | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/t/t3920-crlf-messages.sh b/t/t3920-crlf-messages.sh\n> index 4c661d4d54..a58522c163 100755\n> --- a/t/t3920-crlf-messages.sh\n> +++ b/t/t3920-crlf-messages.sh\n> @@ -12,7 +12,7 @@ create_crlf_ref () {\n> \tcat >.crlf-orig-$branch.txt &&\n> \tcat .crlf-orig-$branch.txt | append_cr >.crlf-message-$branch.txt &&\n\nUseless use of cat.\n\n> \tgrep 'Subject' .crlf-orig-$branch.txt | tr '\\n' ' ' | sed 's/[ ]*$//' | tr -d '\\n' >.crlf-subject-$branch.txt &&\n\nMy knee-jerk reaction to long lines like this is to pull out awk:\n\n\tawk '/Subject/ {printf \"%s\", sep $0; sep = \" \"}' .crlf-orig-$branch.txt >.crlf-subject-$branch.txt &&\n\nThis is not a faithful conversion because the original trims all\nspaces from the end of the subject for some reason. That would be:\n\n\tawk '/Subject/ {s = s $0 \" \"} END {sub(/ *$/, \"\", s); printf \"%s\", s}' .crlf-orig-$branch.txt >.crlf-subject-$branch.txt &&\n\n> -\tgrep 'Body' .crlf-message-$branch.txt >.crlf-body-$branch.txt || true &&\n> +\t{ grep 'Body' .crlf-message-$branch.txt >.crlf-body-$branch.txt || true; } &&\n\nOK, back on topic: Adding CRs explicitly instead of relying on grep to\npass them through (which it doesn't on MinGW) also ignores the return\nvalue of grep as a side effect of the pipe:\n\n\tgrep 'Body' .crlf-orig-$branch.txt | append_cr >.crlf-body-$branch.txt &&\n\n> \tLIB_CRLF_BRANCHES=\"${LIB_CRLF_BRANCHES} ${branch}\" &&\n> \ttest_tick &&\n> \thash=$(git commit-tree HEAD^{tree} -p HEAD -F .crlf-message-${branch}.txt) &&\n"},{"id":"467735","messageId":"xmqqilj7u7wn.fsf@gitster.g","threadId":"58833","inReplyTo":"eed47577-1e47-2fbb-6344-fd4ae324f6b6@web.de","subject":"Re: [PATCH] t3920: don't ignore errors of more than one command with `|| true`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-11-22T00:53:12Z","receivedAt":"2022-11-22T00:53:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n>> @@ -12,7 +12,7 @@ create_crlf_ref () {\n>> \tcat >.crlf-orig-$branch.txt &&\n>> \tcat .crlf-orig-$branch.txt | append_cr >.crlf-message-$branch.txt &&\n>\n> Useless use of cat.\n\nVery good eyes.\n\n> OK, back on topic: Adding CRs explicitly instead of relying on grep to\n> pass them through (which it doesn't on MinGW) also ignores the return\n> value of grep as a side effect of the pipe:\n>\n> \tgrep 'Body' .crlf-orig-$branch.txt | append_cr >.crlf-body-$branch.txt &&\n\nVery nice.\n"},{"id":"467793","messageId":"a8cf5ba2-0593-b8df-a795-c50511fcb38a@gmail.com","threadId":"58833","inReplyTo":"febcfb0a-c410-fb71-cff9-92acfcb269e2@kdbg.org","subject":"Re: [PATCH] t3920: don't ignore errors of more than one command with `|| true`","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2022-11-22T18:28:13Z","receivedAt":"2022-11-22T18:28:30Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Johannes,\n\nLe 2022-11-21 à 12:58, Johannes Sixt a écrit :\n> It is customary to write `A || true` to ignore a potential error exit of\n> command A. But when we have a sequence `A && B && C || true && D`, then\n> a failure of any of A, B, or C skips to D right away. This is not\n> intended here. Turn the command whose failure is to be ignored into a\n> compound command to ensure it is the only one that is allowed to fail.\n> \n> Signed-off-by: Johannes Sixt \n> ---\n> t/t3920-crlf-messages.sh | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/t/t3920-crlf-messages.sh b/t/t3920-crlf-messages.sh\n> index 4c661d4d54..a58522c163 100755\n> --- a/t/t3920-crlf-messages.sh\n> +++ b/t/t3920-crlf-messages.sh\n> @@ -12,7 +12,7 @@ create_crlf_ref () {\n> \tcat >.crlf-orig-$branch.txt &&\n> \tcat .crlf-orig-$branch.txt | append_cr >.crlf-message-$branch.txt &&\n> \tgrep 'Subject' .crlf-orig-$branch.txt | tr '\\n' ' ' | sed 's/[ ]*$//' | tr -d '\\n' >.crlf-subject-$branch.txt &&\n> -\tgrep 'Body' .crlf-message-$branch.txt >.crlf-body-$branch.txt || true &&\n> +\t{ grep 'Body' .crlf-message-$branch.txt >.crlf-body-$branch.txt || true; } &&\n> \tLIB_CRLF_BRANCHES=\"${LIB_CRLF_BRANCHES} ${branch}\" &&\n> \ttest_tick &&\n> \thash=$(git commit-tree HEAD^{tree} -p HEAD -F .crlf-message-${branch}.txt) &&\n> \n\nThank you for making that function more robust.\n\nPhilippe.\n"},{"id":"467818","messageId":"221122.86mt8iaamz.gmgdl@evledraar.gmail.com","threadId":"58833","inReplyTo":"febcfb0a-c410-fb71-cff9-92acfcb269e2@kdbg.org","subject":"Re: [PATCH] t3920: don't ignore errors of more than one command with `|| true`","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-22T22:24:36Z","receivedAt":"2022-11-22T22:26:55Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Nov 21 2022, Johannes Sixt wrote:\n\n> It is customary to write `A || true` to ignore a potential error exit of\n> command A. But when we have a sequence `A && B && C || true && D`, then\n> a failure of any of A, B, or C skips to D right away. This is not\n> intended here. Turn the command whose failure is to be ignored into a\n> compound command to ensure it is the only one that is allowed to fail.\n\n> Signed-off-by: Johannes Sixt \n> ---\n> t/t3920-crlf-messages.sh | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/t/t3920-crlf-messages.sh b/t/t3920-crlf-messages.sh\n> index 4c661d4d54..a58522c163 100755\n> --- a/t/t3920-crlf-messages.sh\n> +++ b/t/t3920-crlf-messages.sh\n> @@ -12,7 +12,7 @@ create_crlf_ref () {\n> \tcat >.crlf-orig-$branch.txt &&\n> \tcat .crlf-orig-$branch.txt | append_cr >.crlf-message-$branch.txt &&\n> \tgrep 'Subject' .crlf-orig-$branch.txt | tr '\\n' ' ' | sed 's/[ ]*$//' | tr -d '\\n' >.crlf-subject-$branch.txt &&\n> -\tgrep 'Body' .crlf-message-$branch.txt >.crlf-body-$branch.txt || true &&\n> +\t{ grep 'Body' .crlf-message-$branch.txt >.crlf-body-$branch.txt || true; } &&\n> \tLIB_CRLF_BRANCHES=\"${LIB_CRLF_BRANCHES} ${branch}\" &&\n> \ttest_tick &&\n> \thash=$(git commit-tree HEAD^{tree} -p HEAD -F .crlf-message-${branch}.txt) &&\n\nAny reason not to make this:\n\n\t- grep 'Body' .crlf-message-$branch.txt >.crlf-body-$branch.txt || true &&\n\t+ sed -ne '/Body/p' <.crlf-message-$branch.txt >.crlf-body-$branch.txt &&\n\n?\n\nWe usually use that for \"grep when we don't care about the exit\ncode\". But maybe some CRLF concerns in this code don't allow it (I only\ntested this on *nix).\n"},{"id":"467820","messageId":"271dd9e6-3575-a46f-4c92-fa202aaf95d7@kdbg.org","threadId":"58833","inReplyTo":"221122.86mt8iaamz.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] t3920: don't ignore errors of more than one command with `|| true`","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2022-11-22T22:37:53Z","receivedAt":"2022-11-22T22:38:01Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 22.11.22 um 23:24 schrieb Ævar Arnfjörð Bjarmason:\n> On Mon, Nov 21 2022, Johannes Sixt wrote:\n>> -\tgrep 'Body' .crlf-message-$branch.txt >.crlf-body-$branch.txt || true &&\n>> +\t{ grep 'Body' .crlf-message-$branch.txt >.crlf-body-$branch.txt || true; } &&\n> \n> Any reason not to make this:\n> \n> \t- grep 'Body' .crlf-message-$branch.txt >.crlf-body-$branch.txt || true &&\n> \t+ sed -ne '/Body/p' <.crlf-message-$branch.txt >.crlf-body-$branch.txt &&\n> \n> ?\n\nYes: I have tested my version, but not this one.\n\n-- Hannes\n\n"},{"id":"467823","messageId":"221123.86ilj6a8ux.gmgdl@evledraar.gmail.com","threadId":"58833","inReplyTo":"271dd9e6-3575-a46f-4c92-fa202aaf95d7@kdbg.org","subject":"Re: [PATCH] t3920: don't ignore errors of more than one command with `|| true`","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-22T22:57:20Z","receivedAt":"2022-11-22T23:05:21Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Nov 22 2022, Johannes Sixt wrote:\n\n> Am 22.11.22 um 23:24 schrieb Ævar Arnfjörð Bjarmason:\n>> On Mon, Nov 21 2022, Johannes Sixt wrote:\n>>> -\tgrep 'Body' .crlf-message-$branch.txt >.crlf-body-$branch.txt || true &&\n>>> +\t{ grep 'Body' .crlf-message-$branch.txt >.crlf-body-$branch.txt || true; } &&\n>> \n>> Any reason not to make this:\n>> \n>> \t- grep 'Body' .crlf-message-$branch.txt >.crlf-body-$branch.txt || true &&\n>> \t+ sed -ne '/Body/p' <.crlf-message-$branch.txt >.crlf-body-$branch.txt &&\n>> \n>> ?\n>\n> Yes: I have tested my version, but not this one.\n\nI don't find that too surprising, as unless there's a discussion of \"I\ntried xyz, but it didn't work, so...\" a submitted patch is unlikely to\nhave tried various alternatives for such a trivial fix, but just gone\nwith whatever the author tried first.\n\nBut in case it wasn't clear, the implied suggestion is that unless there\nis a good reason to introduce a pattern of:\n\n\t { in >out || true; } &&\n\nThat we should use another suitable command with the simpler use of:\n\n\t out &&\n\nIf the result is equivalent, as subsequent maintainers won't need to\npuzzle over the seemingly odd pattern.\n\nWe have that \"sed -n -e\" in somewhat wide use:\n\n\t$ git grep 'sed (-n -e|-ne).*/p.*&&' | wc -l\n\t54\n\nThe only existing occurance of this \"grep but ignore the exit code\" I\ncould find was:\n\n\tt/t9001-send-email.sh: { grep '^X-Mailer:' out || :; } >mailer &&\n\nFor which we can also:\n\n\t- { grep '^X-Mailer:' out || :; } >mailer &&\n\t+ sed -ne '/^X-Mailer:/p' mailer &&\n\nAnd which I'd think would be great to have in a re-roll, i.e. \"here's\nthese two cases where a grep-but-dont-care-about-the-exit-code could be\nreplaced by sed -ne\".\n\nBut if re-testing this is tedious etc. I don't mind if it goes in as-is,\nbut then I'd think we could safely emulate the t9001-send-email.sh\npattern of using \":\" instead of \"true\"; we don't need to invoke another\nprocess just to ignore the exit code.\n\n\n\n"},{"id":"467839","messageId":"xmqq1qpupk0k.fsf@gitster.g","threadId":"58833","inReplyTo":"221123.86ilj6a8ux.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] t3920: don't ignore errors of more than one command with `|| true`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-11-23T00:55:07Z","receivedAt":"2022-11-23T00:55:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> We have that \"sed -n -e\" in somewhat wide use:\n>\n> \t$ git grep 'sed (-n -e|-ne).*/p.*&&' | wc -l\n> \t54\n>\n> The only existing occurance of this \"grep but ignore the exit code\" I\n> could find was:\n>\n> \tt/t9001-send-email.sh: { grep '^X-Mailer:' out || :; } >mailer &&\n>\n> For which we can also:\n>\n> \t- { grep '^X-Mailer:' out || :; } >mailer &&\n> \t+ sed -ne '/^X-Mailer:/p' mailer &&\n>\n> And which I'd think would be great to have in a re-roll, i.e. \"here's\n> these two cases where a grep-but-dont-care-about-the-exit-code could be\n> replaced by sed -ne\".\n>\n> But if re-testing this is tedious etc. I don't mind if it goes in as-is,\n> but then I'd think we could safely emulate the t9001-send-email.sh\n> pattern of using \":\" instead of \"true\"; we don't need to invoke another\n> process just to ignore the exit code.\n\nLet's leave all of the above (mostly good ideas) for \"after the dust\nsettles\" clean-up.\n\nIf sed is much slower than grep (for such a small string use case,\nstart-up cost of a process would dwarf everybody else), \"grep || :\"\nmight be justifiable, but other than that I do not think of a good\nreason why we might favor \"grep || :\" offhand over \"sed -ne //p\".\n\n\n\n"},{"id":"468446","messageId":"cbe88abc-c1fb-cb50-6057-47ff27f7a12d@web.de","threadId":"58833","inReplyTo":"febcfb0a-c410-fb71-cff9-92acfcb269e2@kdbg.org","subject":"[PATCH 2/1] t3920: support CR-eating grep","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-12-02T16:51:13Z","receivedAt":"2022-12-02T16:51:25Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"grep(1) converts CRLF line endings to CR on current MinGW:\n\n $ uname -sr\n MINGW64_NT-10.0-22621 3.3.6-341.x86_64\n\n $ printf 'a\\r\\n' | hexdump.exe -C\n 00000000 61 0d 0a |a..|\n 00000003\n\n $ printf 'a\\r\\n' | grep . | hexdump.exe -C\n 00000000 61 0a |a.|\n 00000002\n\nCreate the intended test file by grepping the original file with LF\nline endings and adding CRs explicitly.\n\nThe missing CRs went unnoticed because test_cmp on MinGW ignores line\nendings since 4d715ac05c (Windows: a test_cmp that is agnostic to random\nLF <> CRLF conversions, 2013-10-26). Fix this test anyway to avoid\ndepending on that special test_cmp behavior, especially since this is\nthe only test that needs it.\n\nPiping the output of grep(1) through append_cr has the side-effect of\nignoring its return value. That means we no longer need the explicit\n\"|| true\" to support commit messages without a body.\n\nSigned-off-by: René Scharfe \n---\n t/t3920-crlf-messages.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t3920-crlf-messages.sh b/t/t3920-crlf-messages.sh\nindex a58522c163..67fd2345af 100755\n--- a/t/t3920-crlf-messages.sh\n+++ b/t/t3920-crlf-messages.sh\n@@ -12,7 +12,7 @@ create_crlf_ref () {\n \tcat >.crlf-orig-$branch.txt &&\n \tcat .crlf-orig-$branch.txt | append_cr >.crlf-message-$branch.txt &&\n \tgrep 'Subject' .crlf-orig-$branch.txt | tr '\\n' ' ' | sed 's/[ ]*$//' | tr -d '\\n' >.crlf-subject-$branch.txt &&\n-\t{ grep 'Body' .crlf-message-$branch.txt >.crlf-body-$branch.txt || true; } &&\n+\tgrep 'Body' .crlf-orig-$branch.txt | append_cr >.crlf-body-$branch.txt &&\n \tLIB_CRLF_BRANCHES=\"${LIB_CRLF_BRANCHES} ${branch}\" &&\n \ttest_tick &&\n \thash=$(git commit-tree HEAD^{tree} -p HEAD -F .crlf-message-${branch}.txt) &&\n--\n2.38.1.windows.1\n"},{"id":"468447","messageId":"9dd8764d-bda9-6602-9ce5-af14de32e315@web.de","threadId":"58833","inReplyTo":"febcfb0a-c410-fb71-cff9-92acfcb269e2@kdbg.org","subject":"[PATCH 3/1] t3920: simplify redirection of loop output","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-12-02T16:51:18Z","receivedAt":"2022-12-02T16:51:30Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Redirect the output of for loops once instead of deleting the receiving\nfile and appending the output of individual commands. This reduces\nrepetition and speeds up the test. Here are the numbers on my Windows\nmachine before:\n\n $ (cd t && hyperfine.exe -w3 \"sh.exe t3920-crlf-messages.sh\")\n Benchmark 1: sh.exe t3920-crlf-messages.sh\n Time (mean ± σ): 5.923 s ± 0.037 s [User: 0.000 s, System: 0.004 s]\n Range (min … max): 5.889 s … 5.997 s 10 runs\n\n... and with this patch:\n\n Benchmark 1: sh.exe t3920-crlf-messages.sh\n Time (mean ± σ): 5.705 s ± 0.047 s [User: 0.000 s, System: 0.001 s]\n Range (min … max): 5.632 s … 5.772 s 10 runs\n\nSigned-off-by: René Scharfe \n---\n t/t3920-crlf-messages.sh | 16 +++++++---------\n 1 file changed, 7 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t3920-crlf-messages.sh b/t/t3920-crlf-messages.sh\nindex 67fd2345af..4fc9fa9cad 100755\n--- a/t/t3920-crlf-messages.sh\n+++ b/t/t3920-crlf-messages.sh\n@@ -68,12 +68,11 @@ test_crlf_subject_body_and_contents() {\n \t\tset ${atoms} && atom=$1 && shift && atoms=\"$*\" &&\n \t\tset ${files} && file=$1 && shift && files=\"$*\" &&\n \t\ttest_expect_success \"${command}: --format='%${atom}' works with messages using CRLF\" \"\n-\t\t\trm -f expect &&\n \t\t\tfor ref in ${LIB_CRLF_BRANCHES}\n \t\t\tdo\n-\t\t\t\tcat .crlf-${file}-\\\"\\${ref}\\\".txt >>expect &&\n-\t\t\t\tprintf \\\"\\n\\\" >>expect || return 1\n-\t\t\tdone &&\n+\t\t\t\tcat .crlf-${file}-\\\"\\${ref}\\\".txt &&\n+\t\t\t\tprintf \\\"\\n\\\" || return 1\n+\t\t\tdone >expect &&\n \t\t\tgit $command_and_args --format=\\\"%${atom}\\\" >actual &&\n \t\t\ttest_cmp expect actual\n \t\t\"\n@@ -87,13 +86,12 @@ test_expect_success 'Setup refs with commit and tag messages using CRLF' '\n '\n\n test_expect_success 'branch: --verbose works with messages using CRLF' '\n-\trm -f expect &&\n \tfor branch in $LIB_CRLF_BRANCHES\n \tdo\n-\t\tprintf \" \" >>expect &&\n-\t\tcat .crlf-subject-${branch}.txt >>expect &&\n-\t\tprintf \"\\n\" >>expect || return 1\n-\tdone &&\n+\t\tprintf \" \" &&\n+\t\tcat .crlf-subject-${branch}.txt &&\n+\t\tprintf \"\\n\" t || return 1\n+\tdone >expect &&\n \tgit branch -v >tmp &&\n \t# Remove first two columns, and the line for the currently checked out branch\n \tcurrent=$(git branch --show-current) &&\n--\n2.38.1.windows.1\n"},{"id":"468448","messageId":"203cb627-2423-8a35-d280-9f9ffc66e072@web.de","threadId":"58833","inReplyTo":"febcfb0a-c410-fb71-cff9-92acfcb269e2@kdbg.org","subject":"[PATCH 4/1] t3920: replace two cats with a tee","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-12-02T16:51:22Z","receivedAt":"2022-12-02T16:51:33Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Use tee(1) to replace two calls of cat(1) for writing files with\ndifferent line endings. That's shorter and spawns less processes.\n\nIt has a small, but measurable performance impact on my Windows machine.\nHere are the numbers before:\n\n $ (cd t && hyperfine.exe -w3 \"sh.exe t3920-crlf-messages.sh\")\n Benchmark 1: sh.exe t3920-crlf-messages.sh\n Time (mean ± σ): 5.705 s ± 0.047 s [User: 0.000 s, System: 0.001 s]\n Range (min … max): 5.632 s … 5.772 s 10 runs\n\n... and with this patch:\n\n $ (cd t && hyperfine.exe -w3 \"sh.exe t3920-crlf-messages.sh\")\n Benchmark 1: sh.exe t3920-crlf-messages.sh\n Time (mean ± σ): 5.616 s ± 0.021 s [User: 0.001 s, System: 0.002 s]\n Range (min … max): 5.577 s … 5.644 s 10 runs\n\nSigned-off-by: René Scharfe \n---\n t/t3920-crlf-messages.sh | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/t/t3920-crlf-messages.sh b/t/t3920-crlf-messages.sh\nindex 4fc9fa9cad..1f64ce565f 100755\n--- a/t/t3920-crlf-messages.sh\n+++ b/t/t3920-crlf-messages.sh\n@@ -9,8 +9,7 @@ LIB_CRLF_BRANCHES=\"\"\n\n create_crlf_ref () {\n \tbranch=\"$1\" &&\n-\tcat >.crlf-orig-$branch.txt &&\n-\tcat .crlf-orig-$branch.txt | append_cr >.crlf-message-$branch.txt &&\n+\ttee .crlf-orig-$branch.txt | append_cr >.crlf-message-$branch.txt &&\n \tgrep 'Subject' .crlf-orig-$branch.txt | tr '\\n' ' ' | sed 's/[ ]*$//' | tr -d '\\n' >.crlf-subject-$branch.txt &&\n \tgrep 'Body' .crlf-orig-$branch.txt | append_cr >.crlf-body-$branch.txt &&\n \tLIB_CRLF_BRANCHES=\"${LIB_CRLF_BRANCHES} ${branch}\" &&\n--\n2.38.1.windows.1\n"},{"id":"468473","messageId":"cd92755f-de16-0df9-075e-2bd5ab0dd4fb@gmail.com","threadId":"58833","inReplyTo":"cbe88abc-c1fb-cb50-6057-47ff27f7a12d@web.de","subject":"Re: [PATCH 2/1] t3920: support CR-eating grep","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2022-12-02T23:14:03Z","receivedAt":"2022-12-02T23:14:09Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi René,\n\nLe 2022-12-02 à 11:51, René Scharfe a écrit :\n> grep(1) converts CRLF line endings to CR on current MinGW:\n> \n> $ uname -sr\n> MINGW64_NT-10.0-22621 3.3.6-341.x86_64\n\nI don't really know the conventions in this regards, but for me \n\"MinGW\" refers to the port of GCC to Windows. Here it would be more \ncorrect to refer to \"the Git for Windows flavor of MSYS2\" or something\nlike this, in my (maybe uninformed) opinion.\n\n> \n> $ printf 'a\\r\\n' | hexdump.exe -C\n> 00000000 61 0d 0a |a..|\n> 00000003\n> \n> $ printf 'a\\r\\n' | grep . | hexdump.exe -C\n> 00000000 61 0a |a.|\n> 00000002\n> \n> Create the intended test file by grepping the original file with LF\n> line endings and adding CRs explicitly.\n> \n> The missing CRs went unnoticed because test_cmp on MinGW ignores line\n> endings since 4d715ac05c (Windows: a test_cmp that is agnostic to random\n> LF <> CRLF conversions, 2013-10-26). \n\nReading that commit's messages, if I understand correctly it's more MSYS2's Bash\nthat is the culprit, rather than its grep, no?\n\n> Fix this test anyway to avoid\n> depending on that special test_cmp behavior, especially since this is\n> the only test that needs it.\n\nDo you mean the only test in that file, or in the whole Git test suite (which would\nmean after this series we could completely remove the Windows-specific 'test_cmp' ) ?\n\n> \n> Piping the output of grep(1) through append_cr has the side-effect of\n> ignoring its return value. That means we no longer need the explicit\n> \"|| true\" to support commit messages without a body.\n> \n> Signed-off-by: René Scharfe \n> ---\n> t/t3920-crlf-messages.sh | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/t/t3920-crlf-messages.sh b/t/t3920-crlf-messages.sh\n> index a58522c163..67fd2345af 100755\n> --- a/t/t3920-crlf-messages.sh\n> +++ b/t/t3920-crlf-messages.sh\n> @@ -12,7 +12,7 @@ create_crlf_ref () {\n> \tcat >.crlf-orig-$branch.txt &&\n> \tcat .crlf-orig-$branch.txt | append_cr >.crlf-message-$branch.txt &&\n> \tgrep 'Subject' .crlf-orig-$branch.txt | tr '\\n' ' ' | sed 's/[ ]*$//' | tr -d '\\n' >.crlf-subject-$branch.txt &&\n> -\t{ grep 'Body' .crlf-message-$branch.txt >.crlf-body-$branch.txt || true; } &&\n> +\tgrep 'Body' .crlf-orig-$branch.txt | append_cr >.crlf-body-$branch.txt &&\n> \tLIB_CRLF_BRANCHES=\"${LIB_CRLF_BRANCHES} ${branch}\" &&\n> \ttest_tick &&\n> \thash=$(git commit-tree HEAD^{tree} -p HEAD -F .crlf-message-${branch}.txt) &&\n> --\n> 2.38.1.windows.1\n> \n\napart from the minor things in the commit message, the 3 patches look good to me :) \nHere is my Acked-by: for all 3 :\n\nAcked-by: Philippe Blain \n\nThanks for detailed messages as always, it definitely went over my radar at the time\nthat both 'grep' and 'test_cmp' acted differently on Windows.\n\nCheers,\nPhilippe.\np.s. I wonder what happened for format-patch to generate these 2/1, 3/1 and 4/1 patch prefixes :P\n"},{"id":"468474","messageId":"CAPig+cQBsr30Sr6djCN5_982-nrrhduTcMK2PMJLTXHvsdmYAQ@mail.gmail.com","threadId":"58833","inReplyTo":"cbe88abc-c1fb-cb50-6057-47ff27f7a12d@web.de","subject":"Re: [PATCH 2/1] t3920: support CR-eating grep","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-12-02T23:32:07Z","receivedAt":"2022-12-02T23:32:23Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Dec 2, 2022 at 11:51 AM René Scharfe wrote:\n> grep(1) converts CRLF line endings to CR on current MinGW:\n\nDid you mean s/to CR/to LF/ ?\n"},{"id":"468480","messageId":"CAPig+cQack=pJ04fwEiq81x6+2AAG4ni0Gd4GQgr4FS=PERb_Q@mail.gmail.com","threadId":"58833","inReplyTo":"203cb627-2423-8a35-d280-9f9ffc66e072@web.de","subject":"Re: [PATCH 4/1] t3920: replace two cats with a tee","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-12-03T05:09:25Z","receivedAt":"2022-12-03T05:09:42Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Dec 2, 2022 at 11:51 AM René Scharfe wrote:\n> Use tee(1) to replace two calls of cat(1) for writing files with\n> different line endings. That's shorter and spawns less processes.\n> [...]\n> Signed-off-by: René Scharfe \n> ---\n> diff --git a/t/t3920-crlf-messages.sh b/t/t3920-crlf-messages.sh\n> @@ -9,8 +9,7 @@ LIB_CRLF_BRANCHES=\"\"\n> create_crlf_ref () {\n> - cat >.crlf-orig-$branch.txt &&\n> - cat .crlf-orig-$branch.txt | append_cr >.crlf-message-$branch.txt &&\n> + tee .crlf-orig-$branch.txt | append_cr >.crlf-message-$branch.txt &&\n\nThis feels slightly magical and more difficult to reason about than\nusing simple redirection to eliminate the second `cat`. Wouldn't this\nwork just as well?\n\n cat >.crlf-orig-$branch.txt &&\n append_cr <.crlf-orig-$branch.txt >.crlf-message-$branch.txt &&\n\n(Plus, this avoids introducing `tee` into the test suite, more or\nless. The few existing instances are all from the same test author and\ndon't seem particularly legitimate -- they appear to be aids the\nauthor used while developing the test to be able to watch its output\nas it ran.)\n"},{"id":"468485","messageId":"ae2862dd-78c2-238f-46df-4a25643046ae@web.de","threadId":"58833","inReplyTo":"cd92755f-de16-0df9-075e-2bd5ab0dd4fb@gmail.com","subject":"Re: [PATCH 2/1] t3920: support CR-eating grep","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-12-03T07:09:23Z","receivedAt":"2022-12-03T07:09:33Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 03.12.22 um 00:14 schrieb Philippe Blain:\n> Hi René,\n>\n> Le 2022-12-02 à 11:51, René Scharfe a écrit :\n>> grep(1) converts CRLF line endings to CR on current MinGW:\n>>\n>> $ uname -sr\n>> MINGW64_NT-10.0-22621 3.3.6-341.x86_64\n>\n> I don't really know the conventions in this regards, but for me\n> \"MinGW\" refers to the port of GCC to Windows. Here it would be more\n> correct to refer to \"the Git for Windows flavor of MSYS2\" or something\n> like this, in my (maybe uninformed) opinion.\n\nThe thing I downloaded and installed is the \"Git for Windows SDK\", so I\ncould use that name instead.\n\n>\n>>\n>> $ printf 'a\\r\\n' | hexdump.exe -C\n>> 00000000 61 0d 0a |a..|\n>> 00000003\n>>\n>> $ printf 'a\\r\\n' | grep . | hexdump.exe -C\n>> 00000000 61 0a |a.|\n>> 00000002\n>>\n>> Create the intended test file by grepping the original file with LF\n>> line endings and adding CRs explicitly.\n>>\n>> The missing CRs went unnoticed because test_cmp on MinGW ignores line\n>> endings since 4d715ac05c (Windows: a test_cmp that is agnostic to random\n>> LF <> CRLF conversions, 2013-10-26).\n>\n> Reading that commit's messages, if I understand correctly it's more MSYS2's Bash\n> that is the culprit, rather than its grep, no?\n\n4d715ac05c blames bash for converting LF to CRLF. Above I blame grep\nfor converting CRLF to LF. The experiment to confirm the latter leaves\nthe possibility that bash somehow inserts a CR eater between grep and\nhexdump. If it does then it is quite selective; e.g. cat passes CRs on\nunscathed:\n\n $ printf 'a\\r\\nb\\n' | grep . | hexdump.exe -C\n 00000000 61 0a 62 0a |a.b.|\n 00000004\n\n $ printf 'a\\r\\nb\\n' | cat | hexdump.exe -C\n 00000000 61 0d 0a 62 0a |a..b.|\n 00000005\n\n>> Fix this test anyway to avoid\n>> depending on that special test_cmp behavior, especially since this is\n>> the only test that needs it.\n>\n> Do you mean the only test in that file, or in the whole Git test suite (which would\n> mean after this series we could completely remove the Windows-specific 'test_cmp' ) ?\n\nBoth.\n\n>>\n>> Piping the output of grep(1) through append_cr has the side-effect of\n>> ignoring its return value. That means we no longer need the explicit\n>> \"|| true\" to support commit messages without a body.\n>>\n>> Signed-off-by: René Scharfe \n>> ---\n>> t/t3920-crlf-messages.sh | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/t/t3920-crlf-messages.sh b/t/t3920-crlf-messages.sh\n>> index a58522c163..67fd2345af 100755\n>> --- a/t/t3920-crlf-messages.sh\n>> +++ b/t/t3920-crlf-messages.sh\n>> @@ -12,7 +12,7 @@ create_crlf_ref () {\n>> \tcat >.crlf-orig-$branch.txt &&\n>> \tcat .crlf-orig-$branch.txt | append_cr >.crlf-message-$branch.txt &&\n>> \tgrep 'Subject' .crlf-orig-$branch.txt | tr '\\n' ' ' | sed 's/[ ]*$//' | tr -d '\\n' >.crlf-subject-$branch.txt &&\n>> -\t{ grep 'Body' .crlf-message-$branch.txt >.crlf-body-$branch.txt || true; } &&\n>> +\tgrep 'Body' .crlf-orig-$branch.txt | append_cr >.crlf-body-$branch.txt &&\n>> \tLIB_CRLF_BRANCHES=\"${LIB_CRLF_BRANCHES} ${branch}\" &&\n>> \ttest_tick &&\n>> \thash=$(git commit-tree HEAD^{tree} -p HEAD -F .crlf-message-${branch}.txt) &&\n>> --\n>> 2.38.1.windows.1\n>>\n>\n> apart from the minor things in the commit message, the 3 patches look good to me :)\n> Here is my Acked-by: for all 3 :\n>\n> Acked-by: Philippe Blain \n>\n> Thanks for detailed messages as always, it definitely went over my radar at the time\n> that both 'grep' and 'test_cmp' acted differently on Windows.\n>\n> Cheers,\n> Philippe.\n> p.s. I wonder what happened for format-patch to generate these 2/1, 3/1 and 4/1 patch prefixes :P\n\nHand-edited..\n\nRené\n"},{"id":"468486","messageId":"f20815dc-7d0a-552d-4f7e-8ff21f942d31@web.de","threadId":"58833","inReplyTo":"CAPig+cQBsr30Sr6djCN5_982-nrrhduTcMK2PMJLTXHvsdmYAQ@mail.gmail.com","subject":"Re: [PATCH 2/1] t3920: support CR-eating grep","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-12-03T07:12:26Z","receivedAt":"2022-12-03T07:12:40Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 03.12.22 um 00:32 schrieb Eric Sunshine:\n> On Fri, Dec 2, 2022 at 11:51 AM René Scharfe wrote:\n>> grep(1) converts CRLF line endings to CR on current MinGW:\n>\n> Did you mean s/to CR/to LF/ ?\n\nYes.\n\nRené\n"},{"id":"468488","messageId":"4611a23a-e7e9-6039-5d54-c2f8cadc2f73@web.de","threadId":"58833","inReplyTo":"CAPig+cQack=pJ04fwEiq81x6+2AAG4ni0Gd4GQgr4FS=PERb_Q@mail.gmail.com","subject":"Re: [PATCH 4/1] t3920: replace two cats with a tee","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-12-03T08:43:18Z","receivedAt":"2022-12-03T08:43:33Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 03.12.22 um 06:09 schrieb Eric Sunshine:\n> On Fri, Dec 2, 2022 at 11:51 AM René Scharfe wrote:\n>> Use tee(1) to replace two calls of cat(1) for writing files with\n>> different line endings. That's shorter and spawns less processes.\n>> [...]\n>> Signed-off-by: René Scharfe \n>> ---\n>> diff --git a/t/t3920-crlf-messages.sh b/t/t3920-crlf-messages.sh\n>> @@ -9,8 +9,7 @@ LIB_CRLF_BRANCHES=\"\"\n>> create_crlf_ref () {\n>> - cat >.crlf-orig-$branch.txt &&\n>> - cat .crlf-orig-$branch.txt | append_cr >.crlf-message-$branch.txt &&\n>> + tee .crlf-orig-$branch.txt | append_cr >.crlf-message-$branch.txt &&\n>\n> This feels slightly magical and more difficult to reason about than\n> using simple redirection to eliminate the second `cat`. Wouldn't this\n> work just as well?\n>\n> cat >.crlf-orig-$branch.txt &&\n> append_cr <.crlf-orig-$branch.txt >.crlf-message-$branch.txt &&\n\nIt would work, of course, but this is the exact use case for tee(1). No\nrepetition, no extra redirection symbols, just an nicely fitting piece\nof pipework. Don't fear the tee! ;-)\n\n(I'm delighted to learn from https://en.wikipedia.org/wiki/Tee_(command)\nthat PowerShell has a tee command as well.)\n\n> (Plus, this avoids introducing `tee` into the test suite, more or\n> less. The few existing instances are all from the same test author and\n> don't seem particularly legitimate -- they appear to be aids the\n> author used while developing the test to be able to watch its output\n> as it ran.)\n\nI agree that the tee calls in t1001 and t5523 are unnecessary.\n\nRené\n"},{"id":"468490","messageId":"221203.86lenoeipb.gmgdl@evledraar.gmail.com","threadId":"58833","inReplyTo":"4611a23a-e7e9-6039-5d54-c2f8cadc2f73@web.de","subject":"Re: [PATCH 4/1] t3920: replace two cats with a tee","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-03T12:53:13Z","receivedAt":"2022-12-03T13:10:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Dec 03 2022, René Scharfe wrote:\n\n> Am 03.12.22 um 06:09 schrieb Eric Sunshine:\n>> On Fri, Dec 2, 2022 at 11:51 AM René Scharfe wrote:\n>>> Use tee(1) to replace two calls of cat(1) for writing files with\n>>> different line endings. That's shorter and spawns less processes.\n>>> [...]\n>>> Signed-off-by: René Scharfe \n>>> ---\n>>> diff --git a/t/t3920-crlf-messages.sh b/t/t3920-crlf-messages.sh\n>>> @@ -9,8 +9,7 @@ LIB_CRLF_BRANCHES=\"\"\n>>> create_crlf_ref () {\n>>> - cat >.crlf-orig-$branch.txt &&\n>>> - cat .crlf-orig-$branch.txt | append_cr >.crlf-message-$branch.txt &&\n>>> + tee .crlf-orig-$branch.txt | append_cr >.crlf-message-$branch.txt &&\n>>\n>> This feels slightly magical and more difficult to reason about than\n>> using simple redirection to eliminate the second `cat`. Wouldn't this\n>> work just as well?\n>>\n>> cat >.crlf-orig-$branch.txt &&\n>> append_cr <.crlf-orig-$branch.txt >.crlf-message-$branch.txt &&\n>\n> It would work, of course, but this is the exact use case for tee(1). No\n> repetition, no extra redirection symbols, just an nicely fitting piece\n> of pipework. Don't fear the tee! ;-)\n>\n> (I'm delighted to learn from https://en.wikipedia.org/wiki/Tee_(command)\n> that PowerShell has a tee command as well.)\n\nI don't really care, but I must say I agree with Eric here. Not having\nsurprising patterns in the test suite has a value of its own.\n\nIn this case I wonder if you want to optimize this whether we couldn't\ndo much better with \"test_commit_bulk\", maybe by teaching it a small set\nof new tricks.\n\nI.e. if I do:\n\n\tgit fast-export --all\n\nAt the end of the setup test it seems we just end up with refs with\nnames that correspond to their contents, and with double newlines in\nthem or whatever. This is a lot of \"grep\", \"sed\", \"tr\" etc. just to end\nup with that.\n\nSo maybe we can create them as a patch, possibly with some slight \"sed\"\nmunging on the input stream, just just teach it to accept a \"ref prefix\"\nand \"commit message contents\". That could just be an argument that you\n\"$(printf \"...\")\", so we don't even need a sub-process....\n\nAlso this:\n\n perl -wE 'say for 1..1024*100' | tee /tmp/x | perl -nE 'print \"in: $_\"; exit 1 if $_ == 512'; tail -n 1 /tmp/x\n\nIsn't deterministic. Now, in this case I doubt it matters, but it's nice\nto have intermediate files in the test suite be determanistic, i.e. to\nalways have the full content be in the file at the top after the \"top\".\n\nWith a \"tee\" you need to worry about the \"append_cr\" function it's being\npiped in stopping the stdin.\n\nI don't think it matters in this case, but in general as a pattern: I do\nfear the \"tee\" a bit :)\n"},{"id":"468493","messageId":"491ad25c-1cf3-98dd-f7aa-e8d1f24c8cd0@web.de","threadId":"58833","inReplyTo":"221203.86lenoeipb.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 4/1] t3920: replace two cats with a tee","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-12-03T17:22:40Z","receivedAt":"2022-12-03T17:22:53Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 03.12.22 um 13:53 schrieb Ævar Arnfjörð Bjarmason:\n>\n> On Sat, Dec 03 2022, René Scharfe wrote:\n>\n>> Am 03.12.22 um 06:09 schrieb Eric Sunshine:\n>>> On Fri, Dec 2, 2022 at 11:51 AM René Scharfe wrote:\n>>>> Use tee(1) to replace two calls of cat(1) for writing files with\n>>>> different line endings. That's shorter and spawns less processes.\n>>>> [...]\n>>>> Signed-off-by: René Scharfe \n>>>> ---\n>>>> diff --git a/t/t3920-crlf-messages.sh b/t/t3920-crlf-messages.sh\n>>>> @@ -9,8 +9,7 @@ LIB_CRLF_BRANCHES=\"\"\n>>>> create_crlf_ref () {\n>>>> - cat >.crlf-orig-$branch.txt &&\n>>>> - cat .crlf-orig-$branch.txt | append_cr >.crlf-message-$branch.txt &&\n>>>> + tee .crlf-orig-$branch.txt | append_cr >.crlf-message-$branch.txt &&\n>>>\n>>> This feels slightly magical and more difficult to reason about than\n>>> using simple redirection to eliminate the second `cat`. Wouldn't this\n>>> work just as well?\n>>>\n>>> cat >.crlf-orig-$branch.txt &&\n>>> append_cr <.crlf-orig-$branch.txt >.crlf-message-$branch.txt &&\n>>\n>> It would work, of course, but this is the exact use case for tee(1). No\n>> repetition, no extra redirection symbols, just an nicely fitting piece\n>> of pipework. Don't fear the tee! ;-)\n>>\n>> (I'm delighted to learn from https://en.wikipedia.org/wiki/Tee_(command)\n>> that PowerShell has a tee command as well.)\n>\n> I don't really care, but I must say I agree with Eric here. Not having\n> surprising patterns in the test suite has a value of its own.\n\nThat's a good general guideline, but I wouldn't have expected a pipe\nwith three holes to startle anyone. *shrug*\n\n> In this case I wonder if you want to optimize this whether we couldn't\n> do much better with \"test_commit_bulk\", maybe by teaching it a small set\n> of new tricks.\n>\n> I.e. if I do:\n>\n> \tgit fast-export --all\n>\n> At the end of the setup test it seems we just end up with refs with\n> names that correspond to their contents, and with double newlines in\n> them or whatever. This is a lot of \"grep\", \"sed\", \"tr\" etc. just to end\n> up with that.\n>\n> So maybe we can create them as a patch, possibly with some slight \"sed\"\n> munging on the input stream, just just teach it to accept a \"ref prefix\"\n> and \"commit message contents\". That could just be an argument that you\n> \"$(printf \"...\")\", so we don't even need a sub-process....\n\nThe files are used later for verification, so their contents can't just\nbe passed on via parameters.\n\nHad a similar idea and spent too much time on creating the four files in\na single awk invocation. The code was too verbose and yet hard to read\nfor my taste.\n\n> Also this:\n>\n> perl -wE 'say for 1..1024*100' | tee /tmp/x | perl -nE 'print \"in: $_\"; exit 1 if $_ == 512'; tail -n 1 /tmp/x\n>\n> Isn't deterministic. Now, in this case I doubt it matters, but it's nice\n> to have intermediate files in the test suite be determanistic, i.e. to\n> always have the full content be in the file at the top after the \"top\".\n\nWhoa, such a one-liner is a good argument for banishing Perl.\n\nSo to rephrase it in a way that I can understand, you say that something\nlike this:\n\n\t$ cd /tmp; seq 100000 | tee x | head -1 >/dev/null; wc -l x\n\n... will probably report less than 100000 lines because the downpipe\ncommand ends the whole thing early.\n\n> With a \"tee\" you need to worry about the \"append_cr\" function it's being\n> piped in stopping the stdin.\n>\n> I don't think it matters in this case, but in general as a pattern: I do\n> fear the \"tee\" a bit :)\n\nRight, append_cr reads until EOF.\n\nRené\n\n"},{"id":"468496","messageId":"221204.86cz8zecam.gmgdl@evledraar.gmail.com","threadId":"58833","inReplyTo":"491ad25c-1cf3-98dd-f7aa-e8d1f24c8cd0@web.de","subject":"Re: [PATCH 4/1] t3920: replace two cats with a tee","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-04T09:34:39Z","receivedAt":"2022-12-04T09:41:41Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Dec 03 2022, René Scharfe wrote:\n\n> Am 03.12.22 um 13:53 schrieb Ævar Arnfjörð Bjarmason:\n>>\n>> On Sat, Dec 03 2022, René Scharfe wrote:\n>>\n>>> Am 03.12.22 um 06:09 schrieb Eric Sunshine:\n>>>> On Fri, Dec 2, 2022 at 11:51 AM René Scharfe wrote:\n>>>>> Use tee(1) to replace two calls of cat(1) for writing files with\n>>>>> different line endings. That's shorter and spawns less processes.\n>>>>> [...]\n>>>>> Signed-off-by: René Scharfe \n>>>>> ---\n>>>>> diff --git a/t/t3920-crlf-messages.sh b/t/t3920-crlf-messages.sh\n>>>>> @@ -9,8 +9,7 @@ LIB_CRLF_BRANCHES=\"\"\n>>>>> create_crlf_ref () {\n>>>>> - cat >.crlf-orig-$branch.txt &&\n>>>>> - cat .crlf-orig-$branch.txt | append_cr >.crlf-message-$branch.txt &&\n>>>>> + tee .crlf-orig-$branch.txt | append_cr >.crlf-message-$branch.txt &&\n>>>>\n>>>> This feels slightly magical and more difficult to reason about than\n>>>> using simple redirection to eliminate the second `cat`. Wouldn't this\n>>>> work just as well?\n>>>>\n>>>> cat >.crlf-orig-$branch.txt &&\n>>>> append_cr <.crlf-orig-$branch.txt >.crlf-message-$branch.txt &&\n>>>\n>>> It would work, of course, but this is the exact use case for tee(1). No\n>>> repetition, no extra redirection symbols, just an nicely fitting piece\n>>> of pipework. Don't fear the tee! ;-)\n>>>\n>>> (I'm delighted to learn from https://en.wikipedia.org/wiki/Tee_(command)\n>>> that PowerShell has a tee command as well.)\n>>\n>> I don't really care, but I must say I agree with Eric here. Not having\n>> surprising patterns in the test suite has a value of its own.\n>\n> That's a good general guideline, but I wouldn't have expected a pipe\n> with three holes to startle anyone. *shrug*\n\nIt's more that you're used to seeing one thing, the \"cat >in\" at the\nstart of a function is a common pattern.\n\nThen it takes some time to stop and grok an a new pattern. If I was\nhacking on a function like that I'd probably stop to try to understand\n\"why\", even though I understood the \"what\".\n\nI'd then find it was to try to optimize things on Windows a bit... :)\n\nI'm not saying it's not worth it in this case, just pointing out that\nboring \"standard\" patterns have a value of their own in us collectively\nunderstanding them, which has a value of its own. Whether optimizing a\ntest case outweighs that is another matter (sometimes it would).\n\n>> In this case I wonder if you want to optimize this whether we couldn't\n>> do much better with \"test_commit_bulk\", maybe by teaching it a small set\n>> of new tricks.\n>>\n>> I.e. if I do:\n>>\n>> \tgit fast-export --all\n>>\n>> At the end of the setup test it seems we just end up with refs with\n>> names that correspond to their contents, and with double newlines in\n>> them or whatever. This is a lot of \"grep\", \"sed\", \"tr\" etc. just to end\n>> up with that.\n>>\n>> So maybe we can create them as a patch, possibly with some slight \"sed\"\n>> munging on the input stream, just just teach it to accept a \"ref prefix\"\n>> and \"commit message contents\". That could just be an argument that you\n>> \"$(printf \"...\")\", so we don't even need a sub-process....\n>\n> The files are used later for verification, so their contents can't just\n> be passed on via parameters.\n>\n> Had a similar idea and spent too much time on creating the four files in\n> a single awk invocation. The code was too verbose and yet hard to read\n> for my taste.\n\nHah, I didn't try. Just a suggestion in case it made sense :)\n\n>> Also this:\n>>\n>> perl -wE 'say for 1..1024*100' | tee /tmp/x | perl -nE 'print \"in: $_\"; exit 1 if $_ == 512'; tail -n 1 /tmp/x\n>>\n>> Isn't deterministic. Now, in this case I doubt it matters, but it's nice\n>> to have intermediate files in the test suite be determanistic, i.e. to\n>> always have the full content be in the file at the top after the \"top\".\n>\n> Whoa, such a one-liner is a good argument for banishing Perl.\n>\n> So to rephrase it in a way that I can understand, you say that something\n> like this:\n>\n> \t$ cd /tmp; seq 100000 | tee x | head -1 >/dev/null; wc -l x\n>\n> ... will probably report less than 100000 lines because the downpipe\n> command ends the whole thing early.\n\nYes, the \"perl\" line was just a quick demo hack.\n\nBut the point is that the initial perl process on the LHS will be killed\nwith a SIGPIPE as the \"perl\" on the RHS stops and a SIGPIPE is\npropagated up the chain.\n\nI don't think it matters in this case, but just pointing out that it\n*is* an edge case this sort of pattern introduces.\n\nI've sometimes resorted to recursively diffing the trash directories of\ntwo test runs to see if they're the same. E.g. I've caught cases where\nthe stderr of programs unexpectedly changes, but we had no test coverage\nfor it.\n\nI think it's good to avoid patterns in general that make test runs\nnondeterministic.\n\nIn this case it's only nondeterministic on failure, so it's probably\nfine.\n"},{"id":"468500","messageId":"CAPig+cQq6Jy9y8vNCFjW+J-GDqTsgbbGJW_SZpNsrb5pkLFDPw@mail.gmail.com","threadId":"58833","inReplyTo":"221204.86cz8zecam.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 4/1] t3920: replace two cats with a tee","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-12-04T16:39:17Z","receivedAt":"2022-12-04T16:39:33Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Dec 4, 2022 at 4:41 AM Ævar Arnfjörð Bjarmason wrote:\n> On Sat, Dec 03 2022, René Scharfe wrote:\n> > Am 03.12.22 um 13:53 schrieb Ævar Arnfjörð Bjarmason:\n> >> On Sat, Dec 03 2022, René Scharfe wrote:\n> >>> Am 03.12.22 um 06:09 schrieb Eric Sunshine:\n> >>>> On Fri, Dec 2, 2022 at 11:51 AM René Scharfe wrote:\n> >>>>> - cat >.crlf-orig-$branch.txt &&\n> >>>>> - cat .crlf-orig-$branch.txt | append_cr >.crlf-message-$branch.txt &&\n> >>>>> + tee .crlf-orig-$branch.txt | append_cr >.crlf-message-$branch.txt &&\n> >>>>\n> >>>> This feels slightly magical and more difficult to reason about than\n> >>>> using simple redirection to eliminate the second `cat`. Wouldn't this\n> >>>> work just as well?\n> >>>>\n> >>>> cat >.crlf-orig-$branch.txt &&\n> >>>> append_cr <.crlf-orig-$branch.txt >.crlf-message-$branch.txt &&\n> >>>\n> >>> It would work, of course, but this is the exact use case for tee(1). No\n> >>> repetition, no extra redirection symbols, just an nicely fitting piece\n> >>> of pipework. Don't fear the tee! ;-)\n> >>\n> >> I don't really care, but I must say I agree with Eric here. Not having\n> >> surprising patterns in the test suite has a value of its own.\n> >\n> > That's a good general guideline, but I wouldn't have expected a pipe\n> > with three holes to startle anyone. *shrug*\n>\n> It's more that you're used to seeing one thing, the \"cat >in\" at the\n> start of a function is a common pattern.\n>\n> Then it takes some time to stop and grok an a new pattern. If I was\n> hacking on a function like that I'd probably stop to try to understand\n> \"why\", even though I understood the \"what\".\n>\n> I'm not saying it's not worth it in this case, just pointing out that\n> boring \"standard\" patterns have a value of their own in us collectively\n> understanding them, which has a value of its own. Whether optimizing a\n> test case outweighs that is another matter (sometimes it would).\n\nPerhaps my experience is atypical, but in decades of using Unix, my\nuse of `tee` can (probably) be counted on a single finger, so the\npatch, as implemented, did have higher cognitive load for me than a\npatch using simple redirection would have had. Anyhow, I mentioned the\nredirection approach, not to ask for a change, but only in case you\nhad overlooked the (to me) simpler approach. I didn't expect it to\nspark so much discussion (though I do agree with everything Ævar has\nsaid about following established patterns).\n\nThat said, I'm still rather unclear on the purpose of this patch. In a\nsense, it feels like mere churn for 1/100 of a second gain (assuming\nI'm reading the `hyperfine` output correctly).\n"},{"id":"468506","messageId":"xmqqo7sisllv.fsf@gitster.g","threadId":"58833","inReplyTo":"cbe88abc-c1fb-cb50-6057-47ff27f7a12d@web.de","subject":"Re: [PATCH 2/1] t3920: support CR-eating grep","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-05T01:08:44Z","receivedAt":"2022-12-05T01:08:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> \tgrep 'Subject' .crlf-orig-$branch.txt | tr '\\n' ' ' | sed 's/[ ]*$//' | tr -d '\\n' >.crlf-subject-$branch.txt &&\n> -\t{ grep 'Body' .crlf-message-$branch.txt >.crlf-body-$branch.txt || true; } &&\n> +\tgrep 'Body' .crlf-orig-$branch.txt | append_cr >.crlf-body-$branch.txt &&\n\nDoesn't append_cr unconditionally adds CR at the end? Do we need to\ntouch this test again when \"grep\" gets fixed on the platform?\n"},{"id":"468508","messageId":"569714f6-a913-5f7f-855a-b303adbde3ee@web.de","threadId":"58833","inReplyTo":"xmqqo7sisllv.fsf@gitster.g","subject":"Re: [PATCH 2/1] t3920: support CR-eating grep","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-12-05T08:28:45Z","receivedAt":"2022-12-05T08:28:58Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 05.12.22 um 02:08 schrieb Junio C Hamano:\n> René Scharfe writes:\n>\n>> \tgrep 'Subject' .crlf-orig-$branch.txt | tr '\\n' ' ' | sed 's/[ ]*$//' | tr -d '\\n' >.crlf-subject-$branch.txt &&\n>> -\t{ grep 'Body' .crlf-message-$branch.txt >.crlf-body-$branch.txt || true; } &&\n>> +\tgrep 'Body' .crlf-orig-$branch.txt | append_cr >.crlf-body-$branch.txt &&\n>\n> Doesn't append_cr unconditionally adds CR at the end?\n\nIt does.\n\n> Do we need to\n> touch this test again when \"grep\" gets fixed on the platform?\n\nDepends on the meaning of \"fixed\". If it stops removing CRs then this\nline is unaffected -- .crlf-orig-$branch.txt contains no CRs. If it\nstarts adding CRs then we'd have a problem with all grep invocations,\nwhich was addressed by 4d715ac05c (Windows: a test_cmp that is agnostic\nto random LF <> CRLF conversions, 2013-10-26).\n\nRené\n"},{"id":"468511","messageId":"xmqqwn76p55v.fsf@gitster.g","threadId":"58833","inReplyTo":"569714f6-a913-5f7f-855a-b303adbde3ee@web.de","subject":"Re: [PATCH 2/1] t3920: support CR-eating grep","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-05T09:32:12Z","receivedAt":"2022-12-05T09:32:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Depends on the meaning of \"fixed\". If it stops removing CRs then this\n> line is unaffected -- .crlf-orig-$branch.txt contains no CRs.\n\nOK, that \"fix\" was what I was worried about and if there is no\nproblem with the input we use, that is good ;-)\n\n> If it\n> starts adding CRs then we'd have a problem with all grep invocations,\n> which was addressed by 4d715ac05c (Windows: a test_cmp that is agnostic\n> to random LF <> CRLF conversions, 2013-10-26).\n\nYeah, but I thought that the unspoken motivation behind recent\nchanges are so that we do not have to rely on \"the differences\nbetween CRLF and LF do not matter\" version of test_cmp?\n\nThanks.\n"},{"id":"468513","messageId":"26ed832d-fb97-c0ef-3848-f58ea574a23f@web.de","threadId":"58833","inReplyTo":"xmqqwn76p55v.fsf@gitster.g","subject":"Re: [PATCH 2/1] t3920: support CR-eating grep","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-12-05T10:43:28Z","receivedAt":"2022-12-05T10:43:47Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 05.12.22 um 10:32 schrieb Junio C Hamano:\n> René Scharfe writes:\n>\n>> Depends on the meaning of \"fixed\". If it stops removing CRs then this\n>> line is unaffected -- .crlf-orig-$branch.txt contains no CRs.\n>\n> OK, that \"fix\" was what I was worried about and if there is no\n> problem with the input we use, that is good ;-)\n>\n>> If it\n>> starts adding CRs then we'd have a problem with all grep invocations,\n>> which was addressed by 4d715ac05c (Windows: a test_cmp that is agnostic\n>> to random LF <> CRLF conversions, 2013-10-26).\n>\n> Yeah, but I thought that the unspoken motivation behind recent\n> changes are so that we do not have to rely on \"the differences\n> between CRLF and LF do not matter\" version of test_cmp?\n\nThe patch currently enables the removal of mingw_test_cmp; its commit\nmessage mentions it in passing (\"[...] especially since this is the only\ntest that needs it.\"). If grep (or bash) would be \"fixed\" to add CRs\nthen we'd have to deal with that damage e.g. by keeping mingw_test_cmp.\n\nRené\n"}]}