{"thread":{"id":"52870","subject":"[PATCH 1/3] t4117: check for files using `test_path_is_file`","startedAt":"2020-02-23T08:49:17Z","lastAt":"2020-02-24T20:49:29Z","messageCount":8,"participants":["Martin Ågren","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"392346","messageId":"dde0f85e5e3dd99a61b83df1b6eb572be8a3ff51.1582447606.git.martin.agren@gmail.com","threadId":"52870","inReplyTo":null,"subject":"[PATCH 1/3] t4117: check for files using `test_path_is_file`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-02-23T08:48:34Z","receivedAt":"2020-02-23T08:49:17Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"We `cat` files, but don't inspect or grab the contents in any way. These\n`cat` calls look like remnants from a debug session, so it's tempting to\nget rid of them. But they do actually verify that the files exist, which\nmight not necessarily be the case for some failure modes of `git apply\n--reject`. Let's not lose that.\n\nConvert the `cat` calls to use `test_path_is_file` instead. This is of\ncourse still a minor change since we no longer verify that the files can\nbe opened for reading, but that is not something we usually worry about.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n t/t4117-apply-reject.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t4117-apply-reject.sh b/t/t4117-apply-reject.sh\nindex f7de6f077a..0ee93fe845 100755\n--- a/t/t4117-apply-reject.sh\n+++ b/t/t4117-apply-reject.sh\n@@ -74,7 +74,7 @@ test_expect_success 'apply with --reject should fail but update the file' '\n \ttest_must_fail git apply --reject patch.1 &&\n \ttest_cmp expected file1 &&\n \n-\tcat file1.rej &&\n+\ttest_path_is_file file1.rej &&\n \ttest_path_is_missing file2.rej\n '\n \n@@ -87,7 +87,7 @@ test_expect_success 'apply with --reject should fail but update the file' '\n \ttest_path_is_missing file1 &&\n \ttest_cmp expected file2 &&\n \n-\tcat file2.rej &&\n+\ttest_path_is_file file2.rej &&\n \ttest_path_is_missing file1.rej\n \n '\n@@ -101,7 +101,7 @@ test_expect_success 'the same test with --verbose' '\n \ttest_path_is_missing file1 &&\n \ttest_cmp expected file2 &&\n \n-\tcat file2.rej &&\n+\ttest_path_is_file file2.rej &&\n \ttest_path_is_missing file1.rej\n \n '\n-- \n2.25.0\n\n"},{"id":"392347","messageId":"ef45c9cabe7fe3f31d63edb872ba946ffbd951fe.1582447606.git.martin.agren@gmail.com","threadId":"52870","inReplyTo":"dde0f85e5e3dd99a61b83df1b6eb572be8a3ff51.1582447606.git.martin.agren@gmail.com","subject":"[PATCH 2/3] t9810: drop debug `cat` call","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-02-23T08:48:35Z","receivedAt":"2020-02-23T08:49:21Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"We `cat` kwdelfile.c, but don't inspect or grab the contents in any way.\nThis looks like a remnant from a debug session. Similar to the previous\ncommit, one could argue that `cat`-ing the file verifies that it didn't\ndisappear somehow. But because the very next thing we do after `cat`-ing\nthe file is to `grep` in it, we can safely drop the call to `cat`.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n t/t9810-git-p4-rcs.sh | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/t/t9810-git-p4-rcs.sh b/t/t9810-git-p4-rcs.sh\nindex 57b533dc6f..e3836888ec 100755\n--- a/t/t9810-git-p4-rcs.sh\n+++ b/t/t9810-git-p4-rcs.sh\n@@ -294,7 +294,6 @@ test_expect_success 'cope with rcs keyword file deletion' '\n \t\techo \"\\$Revision\\$\" >kwdelfile.c &&\n \t\tp4 add -t ktext kwdelfile.c &&\n \t\tp4 submit -d \"Add file to be deleted\" &&\n-\t\tcat kwdelfile.c &&\n \t\tgrep 1 kwdelfile.c\n \t) &&\n \tgit p4 clone --dest=\"$git\" //depot &&\n-- \n2.25.0\n\n"},{"id":"392348","messageId":"179c67caec0f8123d09455cb78532419166e1b9e.1582447606.git.martin.agren@gmail.com","threadId":"52870","inReplyTo":"dde0f85e5e3dd99a61b83df1b6eb572be8a3ff51.1582447606.git.martin.agren@gmail.com","subject":"[PATCH 3/3] t: drop debug `cat` calls","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-02-23T08:48:36Z","receivedAt":"2020-02-23T08:49:26Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"We `cat` files, but don't inspect or grab the contents in any way.\nUnlike in an earlier commit, there is no reason to suspect that these\nfiles could be missing, so `cat`-ing them is just wasted effort.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n t/t0021-conversion.sh                  | 1 -\n t/t1406-submodule-ref-store.sh         | 2 +-\n t/t1450-fsck.sh                        | 1 -\n t/t2107-update-index-basic.sh          | 1 -\n t/t3007-ls-files-recurse-submodules.sh | 1 -\n t/t5150-request-pull.sh                | 2 --\n t/t5409-colorize-remote-messages.sh    | 3 +--\n t/t5510-fetch.sh                       | 1 -\n t/t8003-blame-corner-cases.sh          | 1 -\n t/t9300-fast-import.sh                 | 1 -\n t/t9800-git-p4-basic.sh                | 1 -\n 11 files changed, 2 insertions(+), 13 deletions(-)\n\ndiff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\nindex 6c6d77b51a..dc664da551 100755\n--- a/t/t0021-conversion.sh\n+++ b/t/t0021-conversion.sh\n@@ -795,7 +795,6 @@ test_expect_success PERL 'missing file in delayed checkout' '\n \n \trm -rf repo-cloned &&\n \ttest_must_fail git clone repo repo-cloned 2>git-stderr.log &&\n-\tcat git-stderr.log &&\n \tgrep \"error: .missing-delay\\.a. was not filtered properly\" git-stderr.log\n '\n \ndiff --git a/t/t1406-submodule-ref-store.sh b/t/t1406-submodule-ref-store.sh\nindex d199d872fb..36b7ef5046 100755\n--- a/t/t1406-submodule-ref-store.sh\n+++ b/t/t1406-submodule-ref-store.sh\n@@ -75,7 +75,7 @@ test_expect_success 'for_each_reflog()' '\n '\n \n test_expect_success 'for_each_reflog_ent()' '\n-\t$RUN for-each-reflog-ent HEAD >actual && cat actual &&\n+\t$RUN for-each-reflog-ent HEAD >actual &&\n \thead -n1 actual | grep first &&\n \ttail -n2 actual | head -n1 | grep master.to.new\n '\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex 02478bc4ec..d09eff503c 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -141,7 +141,6 @@ test_expect_success 'email without @ is okay' '\n \tgit update-ref refs/heads/bogus \"$new\" &&\n \ttest_when_finished \"git update-ref -d refs/heads/bogus\" &&\n \tgit fsck 2>out &&\n-\tcat out &&\n \t! grep \"commit $new\" out\n '\n \ndiff --git a/t/t2107-update-index-basic.sh b/t/t2107-update-index-basic.sh\nindex 2242cd098e..a30b7ca6bc 100755\n--- a/t/t2107-update-index-basic.sh\n+++ b/t/t2107-update-index-basic.sh\n@@ -9,7 +9,6 @@ Tests for command-line parsing and basic operation.\n \n test_expect_success 'update-index --nonsense fails' '\n \ttest_must_fail git update-index --nonsense 2>msg &&\n-\tcat msg &&\n \ttest -s msg\n '\n \ndiff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh\nindex 318b5bce7e..4a08000713 100755\n--- a/t/t3007-ls-files-recurse-submodules.sh\n+++ b/t/t3007-ls-files-recurse-submodules.sh\n@@ -130,7 +130,6 @@ test_expect_success '--recurse-submodules and pathspecs setup' '\n \n \tgit ls-files --recurse-submodules >actual &&\n \ttest_cmp expect actual &&\n-\tcat actual &&\n \tgit ls-files --recurse-submodules \"*\" >actual &&\n \ttest_cmp expect actual\n '\ndiff --git a/t/t5150-request-pull.sh b/t/t5150-request-pull.sh\nindex 1ad4ecc29a..c1811ea0f4 100755\n--- a/t/t5150-request-pull.sh\n+++ b/t/t5150-request-pull.sh\n@@ -150,7 +150,6 @@ test_expect_success 'pull request after push' '\n \t\tgit request-pull initial origin master:for-upstream >../request\n \t) &&\n \tsed -nf read-request.sed <request >digest &&\n-\tcat digest &&\n \t{\n \t\tread task &&\n \t\tread repository &&\n@@ -179,7 +178,6 @@ test_expect_success 'request asks HEAD to be pulled' '\n \t\tgit request-pull initial \"$downstream_url\" >../request\n \t) &&\n \tsed -nf read-request.sed <request >digest &&\n-\tcat digest &&\n \t{\n \t\tread task &&\n \t\tread repository &&\ndiff --git a/t/t5409-colorize-remote-messages.sh b/t/t5409-colorize-remote-messages.sh\nindex 2a8c449661..5d8f401d8e 100755\n--- a/t/t5409-colorize-remote-messages.sh\n+++ b/t/t5409-colorize-remote-messages.sh\n@@ -56,14 +56,13 @@ test_expect_success 'short line' '\n \n test_expect_success 'case-insensitive' '\n \tgit --git-dir child/.git -c color.remote=always push -f origin HEAD:refs/heads/case-insensitive 2>output &&\n-\tcat output &&\n \ttest_decode_color <output >decoded &&\n \tgrep \"<BOLD;RED>error<RESET>: error\" decoded &&\n \tgrep \"<BOLD;RED>ERROR<RESET>: also highlighted\" decoded\n '\n \n test_expect_success 'leading space' '\n-\tgit --git-dir child/.git -c color.remote=always push -f origin HEAD:refs/heads/leading-space 2>output &&        cat output &&\n+\tgit --git-dir child/.git -c color.remote=always push -f origin HEAD:refs/heads/leading-space 2>output &&\n \ttest_decode_color <output >decoded &&\n \tgrep \"  <BOLD;RED>error<RESET>: leading space\" decoded\n '\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 5f8f1c287f..4d3a55caaf 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -378,7 +378,6 @@ test_expect_success 'fetch from GIT URL with a non-applying branch.<name>.merge\n # the strange name is: a\\!'b\n test_expect_success 'quoting of a strangely named repo' '\n \ttest_must_fail git fetch \"a\\\\!'\\''b\" > result 2>&1 &&\n-\tcat result &&\n \tgrep \"fatal: '\\''a\\\\\\\\!'\\''b'\\''\" result\n '\n \ndiff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh\nindex 1c5fb1d1f8..9130b887d2 100755\n--- a/t/t8003-blame-corner-cases.sh\n+++ b/t/t8003-blame-corner-cases.sh\n@@ -173,7 +173,6 @@ test_expect_success 'blame during cherry-pick with file rename conflict' '\n \tgit show HEAD@{1}:rodent > rodent &&\n \tgit add rodent &&\n \tgit blame -f -C -C1 rodent | sed -e \"$pick_fc\" >current &&\n-\tcat current &&\n \tcat >expected <<-\\EOF &&\n \tmouse-Initial\n \tmouse-Second\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex ae9950a9c2..3e41c58a13 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -1047,7 +1047,6 @@ test_expect_success 'M: rename root to subdirectory' '\n \tEOF\n \tgit fast-import <input &&\n \tgit diff-tree -M -r M4^ M4 >actual &&\n-\tcat actual &&\n \tcompare_diff_raw expect actual\n '\n \ndiff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh\nindex 5856563068..c98c1dfc23 100755\n--- a/t/t9800-git-p4-basic.sh\n+++ b/t/t9800-git-p4-basic.sh\n@@ -202,7 +202,6 @@ test_expect_success 'exit when p4 fails to produce marshaled output' '\n \t\texport PATH &&\n \t\ttest_expect_code 1 git p4 clone --dest=\"$git\" //depot >errs 2>&1\n \t) &&\n-\tcat errs &&\n \ttest_i18ngrep ! Traceback errs\n '\n \n-- \n2.25.0\n\n"},{"id":"392365","messageId":"20200223214650.GB564691@coredump.intra.peff.net","threadId":"52870","inReplyTo":"dde0f85e5e3dd99a61b83df1b6eb572be8a3ff51.1582447606.git.martin.agren@gmail.com","subject":"Re: [PATCH 1/3] t4117: check for files using `test_path_is_file`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-02-23T21:46:50Z","receivedAt":"2020-02-23T21:46:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 23, 2020 at 09:48:34AM +0100, Martin Ågren wrote:\n\n> We `cat` files, but don't inspect or grab the contents in any way. These\n> `cat` calls look like remnants from a debug session, so it's tempting to\n> get rid of them. But they do actually verify that the files exist, which\n> might not necessarily be the case for some failure modes of `git apply\n> --reject`. Let's not lose that.\n> \n> Convert the `cat` calls to use `test_path_is_file` instead. This is of\n> course still a minor change since we no longer verify that the files can\n> be opened for reading, but that is not something we usually worry about.\n\nThis reasoning makes sense to me. Thanks for being careful in thinking\nabout possible ramifications of removing these otherwise useless-looking\ncat statements.\n\nThis is and the other two patches all look good to me.\n\n-Peff\n"},{"id":"392431","messageId":"xmqqpne3pzey.fsf@gitster-ct.c.googlers.com","threadId":"52870","inReplyTo":"ef45c9cabe7fe3f31d63edb872ba946ffbd951fe.1582447606.git.martin.agren@gmail.com","subject":"Re: [PATCH 2/3] t9810: drop debug `cat` call","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-24T19:18:13Z","receivedAt":"2020-02-24T19:18:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren <martin.agren@gmail.com> writes:\n\n> We `cat` kwdelfile.c, but don't inspect or grab the contents in any way.\n> This looks like a remnant from a debug session. Similar to the previous\n> commit, one could argue that `cat`-ing the file verifies that it didn't\n> disappear somehow. But because the very next thing we do after `cat`-ing\n> the file is to `grep` in it, we can safely drop the call to `cat`.\n>\n> Signed-off-by: Martin Ågren <martin.agren@gmail.com>\n> ---\n>  t/t9810-git-p4-rcs.sh | 1 -\n>  1 file changed, 1 deletion(-)\n\nThe change does make sense, but makes me wonder why we need to\nsingle this one out in a separate patch.  Doesn't this fall into the\nsame category of fixes done in 3/3?  Exactly the same rationale as\nabove applies to the first one in 3/3 on t0021---we can safely lose\nthe cat because the file is immediately grepped for a string, so\nmissing file will be caught as an error either way.\n\nPuzzled...\n\n> diff --git a/t/t9810-git-p4-rcs.sh b/t/t9810-git-p4-rcs.sh\n> index 57b533dc6f..e3836888ec 100755\n> --- a/t/t9810-git-p4-rcs.sh\n> +++ b/t/t9810-git-p4-rcs.sh\n> @@ -294,7 +294,6 @@ test_expect_success 'cope with rcs keyword file deletion' '\n>  \t\techo \"\\$Revision\\$\" >kwdelfile.c &&\n>  \t\tp4 add -t ktext kwdelfile.c &&\n>  \t\tp4 submit -d \"Add file to be deleted\" &&\n> -\t\tcat kwdelfile.c &&\n>  \t\tgrep 1 kwdelfile.c\n>  \t) &&\n>  \tgit p4 clone --dest=\"$git\" //depot &&\n"},{"id":"392432","messageId":"xmqqlforpyqe.fsf@gitster-ct.c.googlers.com","threadId":"52870","inReplyTo":"179c67caec0f8123d09455cb78532419166e1b9e.1582447606.git.martin.agren@gmail.com","subject":"Re: [PATCH 3/3] t: drop debug `cat` calls","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-24T19:32:57Z","receivedAt":"2020-02-24T19:33:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren <martin.agren@gmail.com> writes:\n\n> We `cat` files, but don't inspect or grab the contents in any way.\n> Unlike in an earlier commit, there is no reason to suspect that these\n> files could be missing, so `cat`-ing them is just wasted effort.\n> ...\n>  \trm -rf repo-cloned &&\n>  \ttest_must_fail git clone repo repo-cloned 2>git-stderr.log &&\n> -\tcat git-stderr.log &&\n>  \tgrep \"error: .missing-delay\\.a. was not filtered properly\" git-stderr.log\n\nLoss of cat does not change if git-stderr.log file went missing for\nwhatever reason (e.g. typo while updating the test), just like 2/3,\nso the debugging cruft can safely be removed.\n\n> -\t$RUN for-each-reflog-ent HEAD >actual && cat actual &&\n> +\t$RUN for-each-reflog-ent HEAD >actual &&\n>  \thead -n1 actual | grep first &&\n\nVery similar, as lack of 'actual' for whatever reason will manifest\nin a failure for \"grep\" from finding 'first' anyway.\n\nI sort of sympathetic to your desire to have two classes and\nseparate/distribute them into 2/3 and 3/3, but to me the line\nbetween two classes looks like it was drawn along a wrong axis.\n\nThe proposed log message for this step hints that the criteria for\ntests to be thrown into this category is that the output of 'cat' is\nnot used in any useful way, and the input of 'cat' is known to exist\n(i.e. so \"cat fails if the file is missing\" is not part of the test),\nbut that applies to the one instance singled out in 2/3 (i.e. the\nfile in question, kwdelfile.c, is created with shell redirection,\njust like \"actual\" file above is).\n\n> diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\n> index 02478bc4ec..d09eff503c 100755\n> --- a/t/t1450-fsck.sh\n> +++ b/t/t1450-fsck.sh\n> @@ -141,7 +141,6 @@ test_expect_success 'email without @ is okay' '\n>  \tgit update-ref refs/heads/bogus \"$new\" &&\n>  \ttest_when_finished \"git update-ref -d refs/heads/bogus\" &&\n>  \tgit fsck 2>out &&\n> -\tcat out &&\n>  \t! grep \"commit $new\" out\n>  '\n\nThis one on the other hand *DOES* rely on 'out' being created; we do\nnot want to take the failing 'grep' as a sign of success if it is\nbecause 'out' is missing.\n\n> diff --git a/t/t2107-update-index-basic.sh b/t/t2107-update-index-basic.sh\n> index 2242cd098e..a30b7ca6bc 100755\n> --- a/t/t2107-update-index-basic.sh\n> +++ b/t/t2107-update-index-basic.sh\n> @@ -9,7 +9,6 @@ Tests for command-line parsing and basic operation.\n>  \n>  test_expect_success 'update-index --nonsense fails' '\n>  \ttest_must_fail git update-index --nonsense 2>msg &&\n> -\tcat msg &&\n>  \ttest -s msg\n>  '\n\nThis one does not.  \"test -s msg\" on non-existent msg will fail, so\nthis is closer to category 2/3.\n\nSo, I am OK to have two patches that catch two classes, but the\ndivision between 2/3 and 3/3 in this series does not look the right\none.\n\nI am also OK to have a single patch with updated log message, saying\n\"removal of 'cat <file>' may miss a failure mode that <file> did not\nget created, which would have been caught as a test failure in the\noriginal, but the <file>s used by cats removed in this patch are\neither impossible to be missing (because a preceding step in the\ntest created it, or the &&-cascade would have failed if it failed to\ncreate the file), or followed by another step in the test that would\nfail if the file is missing (e.g. running grep on the file), so it is\nsafe to drop these cats\", or something like that.\n\n\n\n"},{"id":"392434","messageId":"CAN0heSo8RSyvTqTtDLcnBsvMJ-DLL_B-sKRZDNUZBhZ1yX=bmQ@mail.gmail.com","threadId":"52870","inReplyTo":"xmqqlforpyqe.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 3/3] t: drop debug `cat` calls","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-02-24T20:24:32Z","receivedAt":"2020-02-24T20:24:45Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Mon, 24 Feb 2020 at 20:33, Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Martin Ågren <martin.agren@gmail.com> writes:\n>\n> >       git fsck 2>out &&\n> > -     cat out &&\n> >       ! grep \"commit $new\" out\n> >  '\n>\n> This one on the other hand *DOES* rely on 'out' being created; we do\n> not want to take the failing 'grep' as a sign of success if it is\n> because 'out' is missing.\n>\n> >       test_must_fail git update-index --nonsense 2>msg &&\n> > -     cat msg &&\n> >       test -s msg\n> >  '\n>\n> This one does not.  \"test -s msg\" on non-existent msg will fail, so\n> this is closer to category 2/3.\n>\n> So, I am OK to have two patches that catch two classes, but the\n> division between 2/3 and 3/3 in this series does not look the right\n> one.\n\nHeh. You don't want to know how long I waffled on whether to split the\none hunk out to 2/3 and make the rest 3/3 vs. having a slightly larger\n2/2.\n\nFor the first patch 1/3, if we lose the cat entirely, we risk bugs in\n*git* being hidden. For the hunk in patch 2/3, I first thought it was in\nthe same category, before I realized that kwdelfile.c disappearing would\nbe a bug in *p4* as opposed to *git p4*. Since we're not in the business\nof testing/verifying other people's software, we can afford to drop that\ncall entirely. At one point, I had this in the commit message, but in\nthe end I figured one reason for the removal was enough and just kept\nthe \"we'll soon grep\" argument.\n\nI realize now that the line between 2/3 and 3/3 is blurry.\n\nFWIW, for patch 3/3 my reasoning was that for the similar concern about\nthe file not existing, we'd depend on the shell messing up the\nredirection quite badly and not creating a file at all, yet continuing\nwith the && cascade. Which seemed like a pretty crazy bug. And again,\nthat shouldn't be our worry. (I see now that there's a case in 3/3 where\na buggy test_cmp could delete \"actual\" and we'd fail to notice after\nthis commit. That probably also sorts under pathological bugs...)\n\n> I am also OK to have a single patch with updated log message, saying\n> \"removal of 'cat <file>' may miss a failure mode that <file> did not\n> get created, which would have been caught as a test failure in the\n> original, but the <file>s used by cats removed in this patch are\n> either impossible to be missing (because a preceding step in the\n> test created it, or the &&-cascade would have failed if it failed to\n> create the file), or followed by another step in the test that would\n> fail if the file is missing (e.g. running grep on the file), so it is\n> safe to drop these cats\", or something like that.\n\nLet me re-roll with 1/2 (=1/3) and 2/2 (=1/3+2/3).\n\nThanks for a review.\n\nMartin\n"},{"id":"392437","messageId":"xmqq8skrpv6z.fsf@gitster-ct.c.googlers.com","threadId":"52870","inReplyTo":"CAN0heSo8RSyvTqTtDLcnBsvMJ-DLL_B-sKRZDNUZBhZ1yX=bmQ@mail.gmail.com","subject":"Re: [PATCH 3/3] t: drop debug `cat` calls","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-24T20:49:24Z","receivedAt":"2020-02-24T20:49:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren <martin.agren@gmail.com> writes:\n\n> For the first patch 1/3, if we lose the cat entirely, we risk bugs in\n> *git* being hidden.\n\nYes, as Peff already said, the change to explicitly test if the file\nexists makes perfect sense.\n\n> For the hunk in patch 2/3, I first thought it was in the same\n> category, before I realized that kwdelfile.c disappearing would be\n> a bug in *p4* as opposed to *git p4*.  Since we're not in the\n> business of testing/verifying other people's software, we can\n> afford to drop that call entirely.\n\nIt is \">kwdelfile.c\" shell redirection that creates that file.  I\nagree with you that preparing for a bug that \"p4 add -t ktext\nkwdelfile.c\" or \"p4 submit\" removes the file is a little too much.\n"}]}