Re: [PATCH] t3310: avoid hiding failures from rev-parse in command substitutions
From: Eric Sunshine <hidden>
Date: 2026-03-04 22:22:09
On Wed, Mar 4, 2026 at 7:20 AM Francesco Paparatto [off-list ref] wrote:
Running `git` commands inside command substitutions like
test "$(git rev-parse A)" = "$(git rev-parse B)"
can hide failures from the `git` invocations. Extract the
`rev-parse` calls into variables so failures are not ignored.Okay, surfacing failures of `git` invocations is a laudable goal. However...
quoted hunk ↗ jump to hunk
Signed-off-by: Francesco Paparatto <redacted> ---diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh@@ -227,7 +227,8 @@ test_expect_success 'merge z into m (== y) with default ("manual") resolver => C - test "$(git rev-parse refs/notes/m)" = "$(cat pre_merge_y)" + m=$(git rev-parse refs/notes/m) && + test "$m" = "$(cat pre_merge_y)" '
...a failure exposed by `test` is not very developer-friendly since it
doesn't give any indication about what went wrong. Since the pre-merge
value of "y" (and also "z" in subsequent tests) is already in a file,
we can make the failure mode much more helpful by using `test_cmp
<expect> <actual>` which will show both the expected and actual values
when they don't match. Thus, the above transformation would be better
stated along these lines:
git rev-parse refs/notes/m >actual &&
test_cmp pre_merge_y actual
The same comment applies to other changes in this patch.
quoted hunk ↗ jump to hunk
@@ -569,13 +580,17 @@ EOF # Refs are unchanged - test "$(git rev-parse refs/notes/m)" = "$(git rev-parse refs/notes/w)" && - test "$(git rev-parse refs/notes/y)" = "$(git rev-parse NOTES_MERGE_PARTIAL^1)" && - test "$(git rev-parse refs/notes/m)" != "$(git rev-parse NOTES_MERGE_PARTIAL^1)" && + m=$(git rev-parse refs/notes/m) && + w=$(git rev-parse refs/notes/w) && + y=$(git rev-parse refs/notes/y) && + p1=$(git rev-parse NOTES_MERGE_PARTIAL^1) && + test "$m" = "$w" && + test "$y" = "$p1" && + test "$m" != "$p1" &&
In this case we can do even better by taking advantage of
`test_cmp_rev`, which would allow you to express the above more simply
along these lines:
test_cmp_rev refs/notes/m rev-parse refs/notes/w &&
test_cmp_rev refs/notes/y NOTES_MERGE_PARTIAL^1 &&
test_cmp_rev ! refs/notes/m NOTES_MERGE_PARTIAL^1 &&
Note the "!" for negation in the third line.
The same comment applies to other changes in this patch.