Re: [PATCH v3] t3310: avoid hiding failures from rev-parse in command substitutions
From: Eric Sunshine <hidden>
Date: 2026-03-07 06:29:45
On Thu, Mar 5, 2026 at 5:51 PM 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 and provide little
diagnostic information when `test` fails.
Use `test_cmp` when comparing against a stored expected value so
mismatches show both expected and actual output. Use `test_cmp_rev`
when comparing two revisions. These helpers produce clearer failure
output, making it easier to understand what went wrong.
Suggested-by: Eric Sunshine <redacted>
Signed-off-by: Francesco Paparatto <redacted>
---Thank you. This version looks much better and addresses my review comments on the previous round. I do have one actionable recommendation and one subjective comment, though...
quoted hunk ↗ jump to hunk
diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh@@ -569,13 +578,15 @@ EOF test_grep -q "refs/notes/m" output && - test_grep -q "$(git rev-parse refs/notes/m)" output && - test_grep -q "$(git rev-parse NOTES_MERGE_PARTIAL^1)" output && + git rev-parse refs/notes/m >actual && + test_grep -q "$(cat actual)" output && + git rev-parse NOTES_MERGE_PARTIAL^1 >actual && + test_grep -q "$(cat actual)" output &&
Storing the output of git-rev-parse in a file only to read it back out
of that file a moment later is unnecessarily roundabout. It would
instead be cleaner to do it this way:
oid=$(git rev-parse refs/notes/m) &&
test_grep -q "$oid" output &&
oid=$(git rev-parse NOTES_MERGE_PARTIAL^1) &&
test_grep -q "$oid" output &&
Unlike this original in which git-rev-parse's exit code was lost due
to being embedded in the test_grep invocation, this rewrite is safe
because the exit code of git-rev-parse becomes the exit code of the
variable assignment, thus correctly aborts the test (due to the
&&-chain) if git-rev-parse fails.
quoted hunk ↗ jump to hunk
@@ -606,8 +617,9 @@ test_expect_success 'switch cwd before committing notes merge' ' test_must_fail git notes merge refs/notes/other && ( cd .git/NOTES_MERGE_WORKTREE && - echo "foo" > $(git rev-parse HEAD) && - echo "bar" >> $(git rev-parse HEAD) && + oid=$(git rev-parse HEAD) && + echo "foo" >"$oid" && + echo "bar" >>"$oid" &&
This is purely subjective and you don't have to take the suggestion,
but although yours is a faithful rewrite (which is good), I probably
would have simplified this to:
oid=$(git rev-parse HEAD) &&
test_write_lines foo bar >"$oid" &&