[PATCH 0/3] tests: fix broken &&-chains & abort loops on error

[PATCH 0/3] tests: fix broken &&-chains & abort loops on error

[Eric Sunshine via GitGitGadget]

This series fixes some broken &&-chains in tests and adds missing || return
1 (or || exit 1) to loops to ensure they exit early upon error. It is a
followup to an earlier series which fixed many more such problems.^1
[https://lore.kernel.org/git/20211209051115.52629-1-sunshine@sunshineco.com/]

Eric Sunshine (3):
  t2407: fix broken &&-chains in compound statement
  t1092: fix buggy sparse "blame" test
  t: detect and signal failure within loop

 t/perf/p7527-builtin-fsmonitor.sh        |  2 +-
 t/t1092-sparse-checkout-compatibility.sh | 10 +++++-----
 t/t2407-worktree-heads.sh                |  4 ++--
 t/t5329-pack-objects-cruft.sh            |  8 ++++----
 t/t6429-merge-sequence-rename-caching.sh |  2 +-
 5 files changed, 13 insertions(+), 13 deletions(-)


base-commit: 795ea8776befc95ea2becd8020c7a284677b4161
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1312%2Fsunshineco%2Fchainmore-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1312/sunshineco/chainmore-v1
Pull-Request: https://github.com/git/git/pull/1312
-- 
gitgitgadget

[PATCH 1/3] t2407: fix broken &&-chains in compound statement

[Eric Sunshine via GitGitGadget]

From: Eric Sunshine <sunshine@sunshineco.com>

The breaks in the &&-chain in this test went unnoticed because the
"magic exit code 117" &&-chain checker built into test-lib.sh only
recognizes broken &&-chains at the top-level; it does not work within
`{...}` groups, `(...)` subshells, `$(...)` substitutions, or within
bodies of compound statements, such as `if`, `for`, `while`, `case`,
etc. Furthermore, `chainlint.sed` detects broken &&-chains only in
`(...)` subshells. Thus, the &&-chain breaks in this test fall into the
blind spots of the &&-chain linters.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/t2407-worktree-heads.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t2407-worktree-heads.sh b/t/t2407-worktree-heads.sh
index 50815acd3e8..019a40df2ca 100755
--- a/t/t2407-worktree-heads.sh
+++ b/t/t2407-worktree-heads.sh
@@ -41,10 +41,10 @@ test_expect_success 'setup' '
 test_expect_success 'refuse to overwrite: checked out in worktree' '
 	for i in 1 2 3 4
 	do
-		test_must_fail git branch -f wt-$i HEAD 2>err
+		test_must_fail git branch -f wt-$i HEAD 2>err &&
 		grep "cannot force update the branch" err &&
 
-		test_must_fail git branch -D wt-$i 2>err
+		test_must_fail git branch -D wt-$i 2>err &&
 		grep "Cannot delete branch" err || return 1
 	done
 '
-- 
gitgitgadget

[PATCH 2/3] t1092: fix buggy sparse "blame" test

[Eric Sunshine via GitGitGadget]

From: Eric Sunshine <sunshine@sunshineco.com>

This test wants to verify that `git blame` errors out when asked to
blame a file _not_ in the sparse checkout. However, the very first file
it asks to blame _is_ present in the checkout, thus `test_must_fail git
blame $file` gives an unexpected result (the "blame" succeeds). This
problem went unnoticed because the test invokes `test_must_fail git
blame $file` in loop but forgets to break out of the loop early upon
failure, thus the failure gets swallowed.

Fix the test by having it not ask to blame a file present in the sparse
checkout, and instead only blame files not present, as intended. While
at it, also add the missing `|| return 1` which allowed this bug to go
unnoticed.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/t1092-sparse-checkout-compatibility.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index a6a14c8a21f..e13368861ce 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -567,7 +567,7 @@ test_expect_success 'blame with pathspec outside sparse definition' '
 	init_repos &&
 	test_sparse_match git sparse-checkout set &&
 
-	for file in a \
+	for file in \
 			deep/a \
 			deep/deeper1/a \
 			deep/deeper1/deepest/a
@@ -579,7 +579,7 @@ test_expect_success 'blame with pathspec outside sparse definition' '
 		# We compare sparse-checkout-err and sparse-index-err in
 		# `test_sparse_match`. Given we know they are the same, we
 		# only check the content of sparse-index-err here.
-		test_cmp expect sparse-index-err
+		test_cmp expect sparse-index-err || return 1
 	done
 '
 
-- 
gitgitgadget

Re: [PATCH 2/3] t1092: fix buggy sparse "blame" test

[Derrick Stolee]

On 8/22/2022 2:26 PM, Eric Sunshine via GitGitGadget wrote:
> From: Eric Sunshine <sunshine@sunshineco.com>
> 
> This test wants to verify that `git blame` errors out when asked to
> blame a file _not_ in the sparse checkout. However, the very first file
> it asks to blame _is_ present in the checkout, thus `test_must_fail git
> blame $file` gives an unexpected result (the "blame" succeeds). This
> problem went unnoticed because the test invokes `test_must_fail git
> blame $file` in loop but forgets to break out of the loop early upon
> failure, thus the failure gets swallowed.
> 
> Fix the test by having it not ask to blame a file present in the sparse
> checkout, and instead only blame files not present, as intended. While
> at it, also add the missing `|| return 1` which allowed this bug to go
> unnoticed.

Thank you for catching this!

-Stolee

[PATCH 3/3] t: detect and signal failure within loop

[Eric Sunshine via GitGitGadget]

From: Eric Sunshine <sunshine@sunshineco.com>

Failures within `for` and `while` loops can go unnoticed if not detected
and signaled manually since the loop itself does not abort when a
contained command fails, nor will a failure necessarily be detected when
the loop finishes since the loop returns the exit code of the last
command it ran on the final iteration, which may not be the command
which failed. Therefore, detect and signal failures manually within
loops using the idiom `|| return 1` (or `|| exit 1` within subshells).

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/perf/p7527-builtin-fsmonitor.sh        | 2 +-
 t/t1092-sparse-checkout-compatibility.sh | 6 +++---
 t/t5329-pack-objects-cruft.sh            | 8 ++++----
 t/t6429-merge-sequence-rename-caching.sh | 2 +-
 4 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/t/perf/p7527-builtin-fsmonitor.sh b/t/perf/p7527-builtin-fsmonitor.sh
index 9338b9ea008..c3f9a4caa4c 100755
--- a/t/perf/p7527-builtin-fsmonitor.sh
+++ b/t/perf/p7527-builtin-fsmonitor.sh
@@ -249,7 +249,7 @@ test_expect_success "Cleanup temp and matrix branches" "
 	do
 		for fsm_val in $fsm_values
 		do
-			cleanup $uc_val $fsm_val
+			cleanup $uc_val $fsm_val || return 1
 		done
 	done
 "
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index e13368861ce..0302e36fd66 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -556,7 +556,7 @@ test_expect_success 'blame with pathspec inside sparse definition' '
 			deep/deeper1/a \
 			deep/deeper1/deepest/a
 	do
-		test_all_match git blame $file
+		test_all_match git blame $file || return 1
 	done
 '
 
@@ -1571,7 +1571,7 @@ test_expect_success 'sparse index is not expanded: blame' '
 			deep/deeper1/a \
 			deep/deeper1/deepest/a
 	do
-		ensure_not_expanded blame $file
+		ensure_not_expanded blame $file || return 1
 	done
 '
 
@@ -1907,7 +1907,7 @@ test_expect_success 'rm pathspec outside sparse definition' '
 		test_sparse_match test_must_fail git rm $file &&
 		test_sparse_match test_must_fail git rm --cached $file &&
 		test_sparse_match git rm --sparse $file &&
-		test_sparse_match git status --porcelain=v2
+		test_sparse_match git status --porcelain=v2 || return 1
 	done &&
 
 	cat >folder1-full <<-EOF &&
diff --git a/t/t5329-pack-objects-cruft.sh b/t/t5329-pack-objects-cruft.sh
index 8968f7a08d8..6049e2c1d78 100755
--- a/t/t5329-pack-objects-cruft.sh
+++ b/t/t5329-pack-objects-cruft.sh
@@ -29,7 +29,7 @@ basic_cruft_pack_tests () {
 				while read oid
 				do
 					path="$objdir/$(test_oid_to_path "$oid")" &&
-					printf "%s %d\n" "$oid" "$(test-tool chmtime --get "$path")"
+					printf "%s %d\n" "$oid" "$(test-tool chmtime --get "$path")" || exit 1
 				done |
 				sort -k1
 			) >expect &&
@@ -232,7 +232,7 @@ test_expect_success 'cruft tags rescue tagged objects' '
 		while read oid
 		do
 			test-tool chmtime -1000 \
-				"$objdir/$(test_oid_to_path $oid)"
+				"$objdir/$(test_oid_to_path $oid)" || exit 1
 		done <objects &&
 
 		test-tool chmtime -500 \
@@ -272,7 +272,7 @@ test_expect_success 'cruft commits rescue parents, trees' '
 		while read object
 		do
 			test-tool chmtime -1000 \
-				"$objdir/$(test_oid_to_path $object)"
+				"$objdir/$(test_oid_to_path $object)" || exit 1
 		done <objects &&
 		test-tool chmtime +500 "$objdir/$(test_oid_to_path \
 			$(git rev-parse HEAD))" &&
@@ -345,7 +345,7 @@ test_expect_success 'expired objects are pruned' '
 		while read object
 		do
 			test-tool chmtime -1000 \
-				"$objdir/$(test_oid_to_path $object)"
+				"$objdir/$(test_oid_to_path $object)" || exit 1
 		done <objects &&
 
 		keep="$(basename "$(ls $packdir/pack-*.pack)")" &&
diff --git a/t/t6429-merge-sequence-rename-caching.sh b/t/t6429-merge-sequence-rename-caching.sh
index e1ce9199164..650b3cd14ff 100755
--- a/t/t6429-merge-sequence-rename-caching.sh
+++ b/t/t6429-merge-sequence-rename-caching.sh
@@ -725,7 +725,7 @@ test_expect_success 'avoid assuming we detected renames' '
 		mkdir unrelated &&
 		for i in $(test_seq 1 10)
 		do
-			>unrelated/$i
+			>unrelated/$i || exit 1
 		done &&
 		test_seq  2 10 >numbers &&
 		test_seq 12 20 >values &&
-- 
gitgitgadget

Re: [PATCH 3/3] t: detect and signal failure within loop

[Junio C Hamano]

"Eric Sunshine via GitGitGadget" <gitgitgadget@gmail.com> writes:

> diff --git a/t/t5329-pack-objects-cruft.sh b/t/t5329-pack-objects-cruft.sh
> index 8968f7a08d8..6049e2c1d78 100755
> --- a/t/t5329-pack-objects-cruft.sh
> +++ b/t/t5329-pack-objects-cruft.sh
> @@ -29,7 +29,7 @@ basic_cruft_pack_tests () {
>  				while read oid
>  				do
>  					path="$objdir/$(test_oid_to_path "$oid")" &&
> -					printf "%s %d\n" "$oid" "$(test-tool chmtime --get "$path")"
> +					printf "%s %d\n" "$oid" "$(test-tool chmtime --get "$path")" || exit 1
>  				done |
>  				sort -k1
>  			) >expect &&

With the loop being on the upstream of a pipe, does the added "exit
1" have any effect?

Everything else in these three patches looked very sensible, but
this one I found questionable.

Thanks.

Re: [PATCH 3/3] t: detect and signal failure within loop

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> "Eric Sunshine via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> diff --git a/t/t5329-pack-objects-cruft.sh b/t/t5329-pack-objects-cruft.sh
>> index 8968f7a08d8..6049e2c1d78 100755
>> --- a/t/t5329-pack-objects-cruft.sh
>> +++ b/t/t5329-pack-objects-cruft.sh
>> @@ -29,7 +29,7 @@ basic_cruft_pack_tests () {
>>  				while read oid
>>  				do
>>  					path="$objdir/$(test_oid_to_path "$oid")" &&
>> -					printf "%s %d\n" "$oid" "$(test-tool chmtime --get "$path")"
>> +					printf "%s %d\n" "$oid" "$(test-tool chmtime --get "$path")" || exit 1
>>  				done |
>>  				sort -k1
>>  			) >expect &&
>
> With the loop being on the upstream of a pipe, does the added "exit
> 1" have any effect?

And the answer is "no".  Without use of rhetorical question:

    The loop is on the upstream side of a pipe, so "exit 1" will be
    lost.  "sort -k1" will get a shortened output, unless the
    failure happens at the last iteration, so it is likely that the
    test may fail, but relying on the "expect" (what is supposed to
    have the _right_ answer) file not being right to get our
    breakage noticed does not sound right.

> Everything else in these three patches looked very sensible, but
> this one I found questionable.

As to the questionable one, we could probably do something like the
attached patch if we really wanted to.  We can guarantee that this
"expect" will never match any "actual", which is output from
pack-mtimes test tool command.  Whatever "tricky/ugly" approach we
choose to take, I think this one deserves to be done in a single
patch on its own with an explanation.

----- >8 --------- >8 --------- >8 --------- >8 ----
t5329: notice a failure within a loop

We try to write "|| return 1" at the end of a sequence of &&-chained
command in a loop of our tests, so that a failure of any step during
the earlier iteration of the loop can properly be caught.

There is one loop in this test script that is used to compute the
expected result, that will be later compared with an actual output
produced by the "test-tool pack-mtimes" command.  This particular
loop, however, is placed on the upstream side of a pipe, whose
non-zero exit code does not get noticed.

Emit a line that will never be produced by the "test-tool pack-mtimes"
to cause the later comparison to fail.  As we use test_cmp to compare
this "expected output" file with the "actual output", the "error
message" we are emitting into the expected output stream will stand
out and shown to the tester.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t5329-pack-objects-cruft.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git c/t/t5329-pack-objects-cruft.sh w/t/t5329-pack-objects-cruft.sh
index 6049e2c1d7..43d752acc7 100755
--- c/t/t5329-pack-objects-cruft.sh
+++ w/t/t5329-pack-objects-cruft.sh
@@ -29,7 +29,8 @@ basic_cruft_pack_tests () {
 				while read oid
 				do
 					path="$objdir/$(test_oid_to_path "$oid")" &&
-					printf "%s %d\n" "$oid" "$(test-tool chmtime --get "$path")"
+					printf "%s %d\n" "$oid" "$(test-tool chmtime --get "$path")" ||
+					echo "object list generation failed for $obj"
 				done |
 				sort -k1
 			) >expect &&

Re: [PATCH 3/3] t: detect and signal failure within loop

[Johannes Sixt]

Am 22.08.22 um 22:59 schrieb Junio C Hamano:
> t5329: notice a failure within a loop
> 
> We try to write "|| return 1" at the end of a sequence of &&-chained
> command in a loop of our tests, so that a failure of any step during
> the earlier iteration of the loop can properly be caught.
> 
> There is one loop in this test script that is used to compute the
> expected result, that will be later compared with an actual output
> produced by the "test-tool pack-mtimes" command.  This particular
> loop, however, is placed on the upstream side of a pipe, whose
> non-zero exit code does not get noticed.
> 
> Emit a line that will never be produced by the "test-tool pack-mtimes"
> to cause the later comparison to fail.  As we use test_cmp to compare
> this "expected output" file with the "actual output", the "error
> message" we are emitting into the expected output stream will stand
> out and shown to the tester.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  t/t5329-pack-objects-cruft.sh | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git c/t/t5329-pack-objects-cruft.sh w/t/t5329-pack-objects-cruft.sh
> index 6049e2c1d7..43d752acc7 100755
> --- c/t/t5329-pack-objects-cruft.sh
> +++ w/t/t5329-pack-objects-cruft.sh
> @@ -29,7 +29,8 @@ basic_cruft_pack_tests () {
>  				while read oid
>  				do
>  					path="$objdir/$(test_oid_to_path "$oid")" &&
> -					printf "%s %d\n" "$oid" "$(test-tool chmtime --get "$path")"
> +					printf "%s %d\n" "$oid" "$(test-tool chmtime --get "$path")" ||
> +					echo "object list generation failed for $obj"

This looks like the right thing to do. But write $oid, not $obj.

>  				done |
>  				sort -k1
>  			) >expect &&
> 
> 

-- Hannes

Re: [PATCH 3/3] t: detect and signal failure within loop

[Elijah Newren]

On Mon, Aug 22, 2022 at 11:26 AM Eric Sunshine via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Eric Sunshine <sunshine@sunshineco.com>
>
> Failures within `for` and `while` loops can go unnoticed if not detected
> and signaled manually since the loop itself does not abort when a
> contained command fails, nor will a failure necessarily be detected when
> the loop finishes since the loop returns the exit code of the last
> command it ran on the final iteration, which may not be the command
> which failed. Therefore, detect and signal failures manually within
> loops using the idiom `|| return 1` (or `|| exit 1` within subshells).
>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
[...]
> diff --git a/t/t6429-merge-sequence-rename-caching.sh b/t/t6429-merge-sequence-rename-caching.sh
> index e1ce9199164..650b3cd14ff 100755
> --- a/t/t6429-merge-sequence-rename-caching.sh
> +++ b/t/t6429-merge-sequence-rename-caching.sh
> @@ -725,7 +725,7 @@ test_expect_success 'avoid assuming we detected renames' '
>                 mkdir unrelated &&
>                 for i in $(test_seq 1 10)
>                 do
> -                       >unrelated/$i
> +                       >unrelated/$i || exit 1
>                 done &&
>                 test_seq  2 10 >numbers &&
>                 test_seq 12 20 >values &&
> --
> gitgitgadget

That's not something I'm likely ever going to remember to think of as
capable of failing and needing this special care.  Is this a
preliminary series before you send chainlint improvements that finds
this kind of thing for us?  Or did you notice this some other way?

Change is fine, of course, I'm just curious how it was found (and how
I can avoid adding more of these that you'll need to later fix up).

Re: [PATCH 3/3] t: detect and signal failure within loop

[Eric Sunshine]

On Mon, Aug 22, 2022 at 11:05 PM Elijah Newren <newren@gmail.com> wrote:
> On Mon, Aug 22, 2022 at 11:26 AM Eric Sunshine via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > Failures within `for` and `while` loops can go unnoticed if not detected
> > and signaled manually since the loop itself does not abort when a
> > contained command fails, nor will a failure necessarily be detected when
> > the loop finishes since the loop returns the exit code of the last
> > command it ran on the final iteration, which may not be the command
> > which failed. Therefore, detect and signal failures manually within
> > loops using the idiom `|| return 1` (or `|| exit 1` within subshells).
> >
> > diff --git a/t/t6429-merge-sequence-rename-caching.sh b/t/t6429-merge-sequence-rename-caching.sh
> >                 for i in $(test_seq 1 10)
> >                 do
> > -                       >unrelated/$i
> > +                       >unrelated/$i || exit 1
> >                 done &&
>
> That's not something I'm likely ever going to remember to think of as
> capable of failing and needing this special care.  Is this a
> preliminary series before you send chainlint improvements that finds
> this kind of thing for us?  Or did you notice this some other way?

This could fail due to lack of space on the filesystem or "inode"
exhaustion (indeed, I've seen out-of-space failures when I've
underallocated the ramdisk on which I run tests). But there are some
cases of added `|| return` or `|| exit` in the original series[1]
which are just churn because the code inside the loop wouldn't /
couldn't / shouldn't fail. I wasn't happy about adding those simply to
pacify a not-smart-enough linter, but I eventually convinced myself
that the small amount of inconvenience of those pointless cases was
greatly outweighed by the vast number of cases in which adding `||
return` or `|| exit` was the correct thing to do since those cases
could genuinely have allowed errors in Git or in the tests themselves
to go unnoticed.

Yes, this is another preliminary series before sending the new
`chainlint` series, and it was the new linter which found these cases.

[1]: https://lore.kernel.org/git/20211209051115.52629-1-sunshine@sunshineco.com/

> Change is fine, of course, I'm just curious how it was found (and how
> I can avoid adding more of these that you'll need to later fix up).

Although the implementation of the new linter has been complete for
over a year, I finally found time to work on polishing the patch
series itself. I had hoped to get it submitted this past week, but
time constraints prevented it. So, the answer is that the new linter
(once I submit it and once Junio accepts it -- if he does) should help
you avoid introducing more such cases.