{"thread":{"id":"58345","subject":"[PATCH 0/3] tests: fix broken &&-chains & abort loops on error","startedAt":"2022-08-22T18:26:50Z","lastAt":"2022-08-28T04:50:55Z","messageCount":10,"participants":["Eric Sunshine via GitGitGadget","Derrick Stolee","Junio C Hamano","Elijah Newren","Johannes Sixt","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"461788","messageId":"pull.1312.git.git.1661192802.gitgitgadget@gmail.com","threadId":"58345","inReplyTo":null,"subject":"[PATCH 0/3] tests: fix broken &&-chains & abort loops on error","fromName":"Eric Sunshine via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-22T18:26:39Z","receivedAt":"2022-08-22T18:26:50Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"This series fixes some broken &&-chains in tests and adds missing || return\n1 (or || exit 1) to loops to ensure they exit early upon error. It is a\nfollowup to an earlier series which fixed many more such problems.^1\n[https://lore.kernel.org/git/20211209051115.52629-1-sunshine@sunshineco.com/]\n\nEric Sunshine (3):\n t2407: fix broken &&-chains in compound statement\n t1092: fix buggy sparse \"blame\" test\n t: detect and signal failure within loop\n\n t/perf/p7527-builtin-fsmonitor.sh | 2 +-\n t/t1092-sparse-checkout-compatibility.sh | 10 +++++-----\n t/t2407-worktree-heads.sh | 4 ++--\n t/t5329-pack-objects-cruft.sh | 8 ++++----\n t/t6429-merge-sequence-rename-caching.sh | 2 +-\n 5 files changed, 13 insertions(+), 13 deletions(-)\n\n\nbase-commit: 795ea8776befc95ea2becd8020c7a284677b4161\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1312%2Fsunshineco%2Fchainmore-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1312/sunshineco/chainmore-v1\nPull-Request: https://github.com/git/git/pull/1312\n-- \ngitgitgadget\n"},{"id":"461789","messageId":"15d7520479f412d13de17c323311aba077043bf8.1661192802.git.gitgitgadget@gmail.com","threadId":"58345","inReplyTo":"pull.1312.git.git.1661192802.gitgitgadget@gmail.com","subject":"[PATCH 1/3] t2407: fix broken &&-chains in compound statement","fromName":"Eric Sunshine via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-22T18:26:40Z","receivedAt":"2022-08-22T18:26:53Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"From: Eric Sunshine \n\nThe breaks in the &&-chain in this test went unnoticed because the\n\"magic exit code 117\" &&-chain checker built into test-lib.sh only\nrecognizes broken &&-chains at the top-level; it does not work within\n`{...}` groups, `(...)` subshells, `$(...)` substitutions, or within\nbodies of compound statements, such as `if`, `for`, `while`, `case`,\netc. Furthermore, `chainlint.sed` detects broken &&-chains only in\n`(...)` subshells. Thus, the &&-chain breaks in this test fall into the\nblind spots of the &&-chain linters.\n\nSigned-off-by: Eric Sunshine \n---\n t/t2407-worktree-heads.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t2407-worktree-heads.sh b/t/t2407-worktree-heads.sh\nindex 50815acd3e8..019a40df2ca 100755\n--- a/t/t2407-worktree-heads.sh\n+++ b/t/t2407-worktree-heads.sh\n@@ -41,10 +41,10 @@ test_expect_success 'setup' '\n test_expect_success 'refuse to overwrite: checked out in worktree' '\n \tfor i in 1 2 3 4\n \tdo\n-\t\ttest_must_fail git branch -f wt-$i HEAD 2>err\n+\t\ttest_must_fail git branch -f wt-$i HEAD 2>err &&\n \t\tgrep \"cannot force update the branch\" err &&\n \n-\t\ttest_must_fail git branch -D wt-$i 2>err\n+\t\ttest_must_fail git branch -D wt-$i 2>err &&\n \t\tgrep \"Cannot delete branch\" err || return 1\n \tdone\n '\n-- \ngitgitgadget\n\n"},{"id":"461790","messageId":"7b0784056f3cc0c96e9543ae44d0f5a7b0bf85fa.1661192802.git.gitgitgadget@gmail.com","threadId":"58345","inReplyTo":"pull.1312.git.git.1661192802.gitgitgadget@gmail.com","subject":"[PATCH 2/3] t1092: fix buggy sparse \"blame\" test","fromName":"Eric Sunshine via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-22T18:26:41Z","receivedAt":"2022-08-22T18:26:55Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"From: Eric Sunshine \n\nThis test wants to verify that `git blame` errors out when asked to\nblame a file _not_ in the sparse checkout. However, the very first file\nit asks to blame _is_ present in the checkout, thus `test_must_fail git\nblame $file` gives an unexpected result (the \"blame\" succeeds). This\nproblem went unnoticed because the test invokes `test_must_fail git\nblame $file` in loop but forgets to break out of the loop early upon\nfailure, thus the failure gets swallowed.\n\nFix the test by having it not ask to blame a file present in the sparse\ncheckout, and instead only blame files not present, as intended. While\nat it, also add the missing `|| return 1` which allowed this bug to go\nunnoticed.\n\nSigned-off-by: Eric Sunshine \n---\n t/t1092-sparse-checkout-compatibility.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex a6a14c8a21f..e13368861ce 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -567,7 +567,7 @@ test_expect_success 'blame with pathspec outside sparse definition' '\n \tinit_repos &&\n \ttest_sparse_match git sparse-checkout set &&\n \n-\tfor file in a \\\n+\tfor file in \\\n \t\t\tdeep/a \\\n \t\t\tdeep/deeper1/a \\\n \t\t\tdeep/deeper1/deepest/a\n@@ -579,7 +579,7 @@ test_expect_success 'blame with pathspec outside sparse definition' '\n \t\t# We compare sparse-checkout-err and sparse-index-err in\n \t\t# `test_sparse_match`. Given we know they are the same, we\n \t\t# only check the content of sparse-index-err here.\n-\t\ttest_cmp expect sparse-index-err\n+\t\ttest_cmp expect sparse-index-err || return 1\n \tdone\n '\n \n-- \ngitgitgadget\n\n"},{"id":"461791","messageId":"31a962fd5070d68964e545fb5506d795e8845ec3.1661192802.git.gitgitgadget@gmail.com","threadId":"58345","inReplyTo":"pull.1312.git.git.1661192802.gitgitgadget@gmail.com","subject":"[PATCH 3/3] t: detect and signal failure within loop","fromName":"Eric Sunshine via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-22T18:26:42Z","receivedAt":"2022-08-22T18:26:58Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"From: Eric Sunshine \n\nFailures within `for` and `while` loops can go unnoticed if not detected\nand signaled manually since the loop itself does not abort when a\ncontained command fails, nor will a failure necessarily be detected when\nthe loop finishes since the loop returns the exit code of the last\ncommand it ran on the final iteration, which may not be the command\nwhich failed. Therefore, detect and signal failures manually within\nloops using the idiom `|| return 1` (or `|| exit 1` within subshells).\n\nSigned-off-by: Eric Sunshine \n---\n t/perf/p7527-builtin-fsmonitor.sh | 2 +-\n t/t1092-sparse-checkout-compatibility.sh | 6 +++---\n t/t5329-pack-objects-cruft.sh | 8 ++++----\n t/t6429-merge-sequence-rename-caching.sh | 2 +-\n 4 files changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/t/perf/p7527-builtin-fsmonitor.sh b/t/perf/p7527-builtin-fsmonitor.sh\nindex 9338b9ea008..c3f9a4caa4c 100755\n--- a/t/perf/p7527-builtin-fsmonitor.sh\n+++ b/t/perf/p7527-builtin-fsmonitor.sh\n@@ -249,7 +249,7 @@ test_expect_success \"Cleanup temp and matrix branches\" \"\n \tdo\n \t\tfor fsm_val in $fsm_values\n \t\tdo\n-\t\t\tcleanup $uc_val $fsm_val\n+\t\t\tcleanup $uc_val $fsm_val || return 1\n \t\tdone\n \tdone\n \"\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex e13368861ce..0302e36fd66 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -556,7 +556,7 @@ test_expect_success 'blame with pathspec inside sparse definition' '\n \t\t\tdeep/deeper1/a \\\n \t\t\tdeep/deeper1/deepest/a\n \tdo\n-\t\ttest_all_match git blame $file\n+\t\ttest_all_match git blame $file || return 1\n \tdone\n '\n \n@@ -1571,7 +1571,7 @@ test_expect_success 'sparse index is not expanded: blame' '\n \t\t\tdeep/deeper1/a \\\n \t\t\tdeep/deeper1/deepest/a\n \tdo\n-\t\tensure_not_expanded blame $file\n+\t\tensure_not_expanded blame $file || return 1\n \tdone\n '\n \n@@ -1907,7 +1907,7 @@ test_expect_success 'rm pathspec outside sparse definition' '\n \t\ttest_sparse_match test_must_fail git rm $file &&\n \t\ttest_sparse_match test_must_fail git rm --cached $file &&\n \t\ttest_sparse_match git rm --sparse $file &&\n-\t\ttest_sparse_match git status --porcelain=v2\n+\t\ttest_sparse_match git status --porcelain=v2 || return 1\n \tdone &&\n \n \tcat >folder1-full <<-EOF &&\ndiff --git a/t/t5329-pack-objects-cruft.sh b/t/t5329-pack-objects-cruft.sh\nindex 8968f7a08d8..6049e2c1d78 100755\n--- a/t/t5329-pack-objects-cruft.sh\n+++ b/t/t5329-pack-objects-cruft.sh\n@@ -29,7 +29,7 @@ basic_cruft_pack_tests () {\n \t\t\t\twhile read oid\n \t\t\t\tdo\n \t\t\t\t\tpath=\"$objdir/$(test_oid_to_path \"$oid\")\" &&\n-\t\t\t\t\tprintf \"%s %d\\n\" \"$oid\" \"$(test-tool chmtime --get \"$path\")\"\n+\t\t\t\t\tprintf \"%s %d\\n\" \"$oid\" \"$(test-tool chmtime --get \"$path\")\" || exit 1\n \t\t\t\tdone |\n \t\t\t\tsort -k1\n \t\t\t) >expect &&\n@@ -232,7 +232,7 @@ test_expect_success 'cruft tags rescue tagged objects' '\n \t\twhile read oid\n \t\tdo\n \t\t\ttest-tool chmtime -1000 \\\n-\t\t\t\t\"$objdir/$(test_oid_to_path $oid)\"\n+\t\t\t\t\"$objdir/$(test_oid_to_path $oid)\" || exit 1\n \t\tdone unrelated/$i\n+\t\t\t>unrelated/$i || exit 1\n \t\tdone &&\n \t\ttest_seq 2 10 >numbers &&\n \t\ttest_seq 12 20 >values &&\n-- \ngitgitgadget\n"},{"id":"461807","messageId":"39b64f22-702a-80f0-af5d-50bb2dcdddfc@github.com","threadId":"58345","inReplyTo":"7b0784056f3cc0c96e9543ae44d0f5a7b0bf85fa.1661192802.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/3] t1092: fix buggy sparse \"blame\" test","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-08-22T20:09:54Z","receivedAt":"2022-08-22T20:10:01Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/22/2022 2:26 PM, Eric Sunshine via GitGitGadget wrote:\n> From: Eric Sunshine \n> \n> This test wants to verify that `git blame` errors out when asked to\n> blame a file _not_ in the sparse checkout. However, the very first file\n> it asks to blame _is_ present in the checkout, thus `test_must_fail git\n> blame $file` gives an unexpected result (the \"blame\" succeeds). This\n> problem went unnoticed because the test invokes `test_must_fail git\n> blame $file` in loop but forgets to break out of the loop early upon\n> failure, thus the failure gets swallowed.\n> \n> Fix the test by having it not ask to blame a file present in the sparse\n> checkout, and instead only blame files not present, as intended. While\n> at it, also add the missing `|| return 1` which allowed this bug to go\n> unnoticed.\n\nThank you for catching this!\n\n-Stolee\n"},{"id":"461809","messageId":"xmqqwnb0av09.fsf@gitster.g","threadId":"58345","inReplyTo":"31a962fd5070d68964e545fb5506d795e8845ec3.1661192802.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/3] t: detect and signal failure within loop","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-22T20:22:30Z","receivedAt":"2022-08-22T20:22:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Eric Sunshine via GitGitGadget\" writes:\n\n> diff --git a/t/t5329-pack-objects-cruft.sh b/t/t5329-pack-objects-cruft.sh\n> index 8968f7a08d8..6049e2c1d78 100755\n> --- a/t/t5329-pack-objects-cruft.sh\n> +++ b/t/t5329-pack-objects-cruft.sh\n> @@ -29,7 +29,7 @@ basic_cruft_pack_tests () {\n> \t\t\t\twhile read oid\n> \t\t\t\tdo\n> \t\t\t\t\tpath=\"$objdir/$(test_oid_to_path \"$oid\")\" &&\n> -\t\t\t\t\tprintf \"%s %d\\n\" \"$oid\" \"$(test-tool chmtime --get \"$path\")\"\n> +\t\t\t\t\tprintf \"%s %d\\n\" \"$oid\" \"$(test-tool chmtime --get \"$path\")\" || exit 1\n> \t\t\t\tdone |\n> \t\t\t\tsort -k1\n> \t\t\t) >expect &&\n\nWith the loop being on the upstream of a pipe, does the added \"exit\n1\" have any effect?\n\nEverything else in these three patches looked very sensible, but\nthis one I found questionable.\n\nThanks.\n"},{"id":"461810","messageId":"xmqqfshoataq.fsf@gitster.g","threadId":"58345","inReplyTo":"xmqqwnb0av09.fsf@gitster.g","subject":"Re: [PATCH 3/3] t: detect and signal failure within loop","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-22T20:59:25Z","receivedAt":"2022-08-22T20:59:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> \"Eric Sunshine via GitGitGadget\" writes:\n>\n>> diff --git a/t/t5329-pack-objects-cruft.sh b/t/t5329-pack-objects-cruft.sh\n>> index 8968f7a08d8..6049e2c1d78 100755\n>> --- a/t/t5329-pack-objects-cruft.sh\n>> +++ b/t/t5329-pack-objects-cruft.sh\n>> @@ -29,7 +29,7 @@ basic_cruft_pack_tests () {\n>> \t\t\t\twhile read oid\n>> \t\t\t\tdo\n>> \t\t\t\t\tpath=\"$objdir/$(test_oid_to_path \"$oid\")\" &&\n>> -\t\t\t\t\tprintf \"%s %d\\n\" \"$oid\" \"$(test-tool chmtime --get \"$path\")\"\n>> +\t\t\t\t\tprintf \"%s %d\\n\" \"$oid\" \"$(test-tool chmtime --get \"$path\")\" || exit 1\n>> \t\t\t\tdone |\n>> \t\t\t\tsort -k1\n>> \t\t\t) >expect &&\n>\n> With the loop being on the upstream of a pipe, does the added \"exit\n> 1\" have any effect?\n\nAnd the answer is \"no\". Without use of rhetorical question:\n\n The loop is on the upstream side of a pipe, so \"exit 1\" will be\n lost. \"sort -k1\" will get a shortened output, unless the\n failure happens at the last iteration, so it is likely that the\n test may fail, but relying on the \"expect\" (what is supposed to\n have the _right_ answer) file not being right to get our\n breakage noticed does not sound right.\n\n> Everything else in these three patches looked very sensible, but\n> this one I found questionable.\n\nAs to the questionable one, we could probably do something like the\nattached patch if we really wanted to. We can guarantee that this\n\"expect\" will never match any \"actual\", which is output from\npack-mtimes test tool command. Whatever \"tricky/ugly\" approach we\nchoose to take, I think this one deserves to be done in a single\npatch on its own with an explanation.\n\n----- >8 --------- >8 --------- >8 --------- >8 ----\nt5329: notice a failure within a loop\n\nWe try to write \"|| return 1\" at the end of a sequence of &&-chained\ncommand in a loop of our tests, so that a failure of any step during\nthe earlier iteration of the loop can properly be caught.\n\nThere is one loop in this test script that is used to compute the\nexpected result, that will be later compared with an actual output\nproduced by the \"test-tool pack-mtimes\" command. This particular\nloop, however, is placed on the upstream side of a pipe, whose\nnon-zero exit code does not get noticed.\n\nEmit a line that will never be produced by the \"test-tool pack-mtimes\"\nto cause the later comparison to fail. As we use test_cmp to compare\nthis \"expected output\" file with the \"actual output\", the \"error\nmessage\" we are emitting into the expected output stream will stand\nout and shown to the tester.\n\nSigned-off-by: Junio C Hamano \n---\n t/t5329-pack-objects-cruft.sh | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git c/t/t5329-pack-objects-cruft.sh w/t/t5329-pack-objects-cruft.sh\nindex 6049e2c1d7..43d752acc7 100755\n--- c/t/t5329-pack-objects-cruft.sh\n+++ w/t/t5329-pack-objects-cruft.sh\n@@ -29,7 +29,8 @@ basic_cruft_pack_tests () {\n \t\t\t\twhile read oid\n \t\t\t\tdo\n \t\t\t\t\tpath=\"$objdir/$(test_oid_to_path \"$oid\")\" &&\n-\t\t\t\t\tprintf \"%s %d\\n\" \"$oid\" \"$(test-tool chmtime --get \"$path\")\"\n+\t\t\t\t\tprintf \"%s %d\\n\" \"$oid\" \"$(test-tool chmtime --get \"$path\")\" ||\n+\t\t\t\t\techo \"object list generation failed for $obj\"\n \t\t\t\tdone |\n \t\t\t\tsort -k1\n \t\t\t) >expect &&\n\n"},{"id":"461830","messageId":"CABPp-BH-QzH-5MmvBwqncxr2VQQPfAk0oEYus2HMgdmpX3ppUg@mail.gmail.com","threadId":"58345","inReplyTo":"31a962fd5070d68964e545fb5506d795e8845ec3.1661192802.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/3] t: detect and signal failure within loop","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-08-23T03:05:05Z","receivedAt":"2022-08-23T03:08:10Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Aug 22, 2022 at 11:26 AM Eric Sunshine via GitGitGadget\n wrote:\n>\n> From: Eric Sunshine \n>\n> Failures within `for` and `while` loops can go unnoticed if not detected\n> and signaled manually since the loop itself does not abort when a\n> contained command fails, nor will a failure necessarily be detected when\n> the loop finishes since the loop returns the exit code of the last\n> command it ran on the final iteration, which may not be the command\n> which failed. Therefore, detect and signal failures manually within\n> loops using the idiom `|| return 1` (or `|| exit 1` within subshells).\n>\n> Signed-off-by: Eric Sunshine \n[...]\n> diff --git a/t/t6429-merge-sequence-rename-caching.sh b/t/t6429-merge-sequence-rename-caching.sh\n> index e1ce9199164..650b3cd14ff 100755\n> --- a/t/t6429-merge-sequence-rename-caching.sh\n> +++ b/t/t6429-merge-sequence-rename-caching.sh\n> @@ -725,7 +725,7 @@ test_expect_success 'avoid assuming we detected renames' '\n> mkdir unrelated &&\n> for i in $(test_seq 1 10)\n> do\n> - >unrelated/$i\n> + >unrelated/$i || exit 1\n> done &&\n> test_seq 2 10 >numbers &&\n> test_seq 12 20 >values &&\n> --\n> gitgitgadget\n\nThat's not something I'm likely ever going to remember to think of as\ncapable of failing and needing this special care. Is this a\npreliminary series before you send chainlint improvements that finds\nthis kind of thing for us? Or did you notice this some other way?\n\nChange is fine, of course, I'm just curious how it was found (and how\nI can avoid adding more of these that you'll need to later fix up).\n"},{"id":"461833","messageId":"103fa5ac-d67c-82a7-11b2-0ffee7570349@kdbg.org","threadId":"58345","inReplyTo":"xmqqfshoataq.fsf@gitster.g","subject":"Re: [PATCH 3/3] t: detect and signal failure within loop","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2022-08-23T06:30:03Z","receivedAt":"2022-08-23T06:30:18Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 22.08.22 um 22:59 schrieb Junio C Hamano:\n> t5329: notice a failure within a loop\n> \n> We try to write \"|| return 1\" at the end of a sequence of &&-chained\n> command in a loop of our tests, so that a failure of any step during\n> the earlier iteration of the loop can properly be caught.\n> \n> There is one loop in this test script that is used to compute the\n> expected result, that will be later compared with an actual output\n> produced by the \"test-tool pack-mtimes\" command. This particular\n> loop, however, is placed on the upstream side of a pipe, whose\n> non-zero exit code does not get noticed.\n> \n> Emit a line that will never be produced by the \"test-tool pack-mtimes\"\n> to cause the later comparison to fail. As we use test_cmp to compare\n> this \"expected output\" file with the \"actual output\", the \"error\n> message\" we are emitting into the expected output stream will stand\n> out and shown to the tester.\n> \n> Signed-off-by: Junio C Hamano \n> ---\n> t/t5329-pack-objects-cruft.sh | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n> \n> diff --git c/t/t5329-pack-objects-cruft.sh w/t/t5329-pack-objects-cruft.sh\n> index 6049e2c1d7..43d752acc7 100755\n> --- c/t/t5329-pack-objects-cruft.sh\n> +++ w/t/t5329-pack-objects-cruft.sh\n> @@ -29,7 +29,8 @@ basic_cruft_pack_tests () {\n> \t\t\t\twhile read oid\n> \t\t\t\tdo\n> \t\t\t\t\tpath=\"$objdir/$(test_oid_to_path \"$oid\")\" &&\n> -\t\t\t\t\tprintf \"%s %d\\n\" \"$oid\" \"$(test-tool chmtime --get \"$path\")\"\n> +\t\t\t\t\tprintf \"%s %d\\n\" \"$oid\" \"$(test-tool chmtime --get \"$path\")\" ||\n> +\t\t\t\t\techo \"object list generation failed for $obj\"\n\nThis looks like the right thing to do. But write $oid, not $obj.\n\n> \t\t\t\tdone |\n> \t\t\t\tsort -k1\n> \t\t\t) >expect &&\n> \n> \n\n-- Hannes\n"},{"id":"462048","messageId":"CAPig+cTGJeNJCYT2gNP8AJoohptqOD_hr2nyy1vv=HUL2u-eXA@mail.gmail.com","threadId":"58345","inReplyTo":"CABPp-BH-QzH-5MmvBwqncxr2VQQPfAk0oEYus2HMgdmpX3ppUg@mail.gmail.com","subject":"Re: [PATCH 3/3] t: detect and signal failure within loop","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-08-28T04:50:39Z","receivedAt":"2022-08-28T04:50:55Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Aug 22, 2022 at 11:05 PM Elijah Newren wrote:\n> On Mon, Aug 22, 2022 at 11:26 AM Eric Sunshine via GitGitGadget\n> wrote:\n> > Failures within `for` and `while` loops can go unnoticed if not detected\n> > and signaled manually since the loop itself does not abort when a\n> > contained command fails, nor will a failure necessarily be detected when\n> > the loop finishes since the loop returns the exit code of the last\n> > command it ran on the final iteration, which may not be the command\n> > which failed. Therefore, detect and signal failures manually within\n> > loops using the idiom `|| return 1` (or `|| exit 1` within subshells).\n> >\n> > diff --git a/t/t6429-merge-sequence-rename-caching.sh b/t/t6429-merge-sequence-rename-caching.sh\n> > for i in $(test_seq 1 10)\n> > do\n> > - >unrelated/$i\n> > + >unrelated/$i || exit 1\n> > done &&\n>\n> That's not something I'm likely ever going to remember to think of as\n> capable of failing and needing this special care. Is this a\n> preliminary series before you send chainlint improvements that finds\n> this kind of thing for us? Or did you notice this some other way?\n\nThis could fail due to lack of space on the filesystem or \"inode\"\nexhaustion (indeed, I've seen out-of-space failures when I've\nunderallocated the ramdisk on which I run tests). But there are some\ncases of added `|| return` or `|| exit` in the original series[1]\nwhich are just churn because the code inside the loop wouldn't /\ncouldn't / shouldn't fail. I wasn't happy about adding those simply to\npacify a not-smart-enough linter, but I eventually convinced myself\nthat the small amount of inconvenience of those pointless cases was\ngreatly outweighed by the vast number of cases in which adding `||\nreturn` or `|| exit` was the correct thing to do since those cases\ncould genuinely have allowed errors in Git or in the tests themselves\nto go unnoticed.\n\nYes, this is another preliminary series before sending the new\n`chainlint` series, and it was the new linter which found these cases.\n\n[1]: https://lore.kernel.org/git/20211209051115.52629-1-sunshine@sunshineco.com/\n\n> Change is fine, of course, I'm just curious how it was found (and how\n> I can avoid adding more of these that you'll need to later fix up).\n\nAlthough the implementation of the new linter has been complete for\nover a year, I finally found time to work on polishing the patch\nseries itself. I had hoped to get it submitted this past week, but\ntime constraints prevented it. So, the answer is that the new linter\n(once I submit it and once Junio accepts it -- if he does) should help\nyou avoid introducing more such cases.\n"}]}