Re: [PATCH v2] t3310: avoid hiding failures from rev-parse in command substitutions

9 messages, 3 authors, 2026-03-08 · open the first message on its own page

Re: [PATCH v2] t3310: avoid hiding failures from rev-parse in command substitutions

From: Junio C Hamano <hidden>
Date: 2026-03-05 19:13:17 

Francesco Paparatto [off-list ref] writes:

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: Junio C Hamano <redacted>
Hmph, did I suggest this?  I know Eric had comments on a previous
round, and the improvements in this patch seems to be influenced a
lot stronger by his input than whatever I may have said.
quoted hunk
Signed-off-by: Francesco Paparatto <redacted>
---
 t/t3310-notes-merge-manual-resolve.sh | 60 ++++++++++++---------------
 1 file changed, 27 insertions(+), 33 deletions(-)

diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh
index 92a5951331..64c0a753ff 100755
On top of what commit is this patch designed to apply?

Thanks.

Re: [PATCH v2] t3310: avoid hiding failures from rev-parse in command substitutions

From: Eric Sunshine <hidden>
Date: 2026-03-05 22:34:34 

On Thu, Mar 5, 2026 at 2:13 PM Junio C Hamano [off-list ref] wrote:
quoted
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: Junio C Hamano <redacted>
Hmph, did I suggest this?  I know Eric had comments on a previous
round, and the improvements in this patch seems to be influenced a
lot stronger by his input than whatever I may have said.
Indeed. The use of test_cmp and test_cmp_rev makes this version much
more developer-friendly than v1. Nice.
quoted
 t/t3310-notes-merge-manual-resolve.sh | 60 ++++++++++++---------------
 1 file changed, 27 insertions(+), 33 deletions(-)

diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh
index 92a5951331..64c0a753ff 100755
On top of what commit is this patch designed to apply?
What Junio probably means is that you appear to have based v2 atop v1,
but instead you should squash v1 and v2 into a single patch, and send
that as v3 so that when the patch is finally accepted into his tree,
it will appear to have been perfect from the start (because v1 and v2
will only exist in the mailing list archive, not in the Git project
history).

Re: [PATCH v2] t3310: avoid hiding failures from rev-parse in command substitutions

From: Francesco Paparatto <hidden>
Date: 2026-03-05 22:42:24 

Junio C Hamano [off-list ref] writes:

Hmph, did I suggest this?  I know Eric had comments on a previous
round, and the improvements in this patch seems to be influenced a
lot stronger by his input than whatever I may have said.
Sorry about the Suggested-by line. I added it because of your earlier
comment here:

    https://public-inbox.org/git/xmqqv7fioueg.fsf@gitster.g/

but you're right that the concrete changes in this version were mostly
influenced by Eric's review, so I'll drop that trailer.

On top of what commit is this patch designed to apply?
This patch is based on top of:

    b3ec5aec2367262f464a33d6eab7a9f49fd413f1
    ("t3310: replace test -f/-d with test_path_is_file/test_path_is_dir")

I'll reroll and send a v3 of the patch shortly.

Thanks,
Francesco

Re: [PATCH v2] t3310: avoid hiding failures from rev-parse in command substitutions

From: Francesco Paparatto <hidden>
Date: 2026-03-05 22:49:43 

Eric Sunshine [off-list ref] writes:

What Junio probably means is that you appear to have based v2 atop v1,
but instead you should squash v1 and v2 into a single patch, and send
that as v3 so that when the patch is finally accepted into his tree,
it will appear to have been perfect from the start (because v1 and v2
will only exist in the mailing list archive, not in the Git project
history).
Sorry about that, and thanks for the clarification.

I've squashed the changes and rerolled the patch based on your
suggestions. I've just sent v3 to the list.

Thanks,
Francesco

[PATCH v3] t3310: avoid hiding failures from rev-parse in command substitutions

From: Francesco Paparatto <hidden>
Date: 2026-03-05 22:51:47 

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>
---
 t/t3310-notes-merge-manual-resolve.sh | 48 +++++++++++++++++----------
 1 file changed, 30 insertions(+), 18 deletions(-)

diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh
index f0054b0a39..64c0a753ff 100755
--- 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
 	# Verify that current notes tree (pre-merge) has not changed (m == y)
 	verify_notes y &&
 	verify_notes m &&
-	test "$(git rev-parse refs/notes/m)" = "$(cat pre_merge_y)"
+	git rev-parse refs/notes/m >actual &&
+	test_cmp pre_merge_y actual
 '
 
 cat <<EOF | sort >expect_notes_z
@@ -375,8 +376,10 @@ EOF
 	git notes merge --commit &&
 	notes_merge_files_gone &&
 	# Merge commit has pre-merge y and pre-merge z as parents
-	test "$(git rev-parse refs/notes/m^1)" = "$(cat pre_merge_y)" &&
-	test "$(git rev-parse refs/notes/m^2)" = "$(cat pre_merge_z)" &&
+	git rev-parse refs/notes/m^1 >actual &&
+	test_cmp pre_merge_y actual &&
+	git rev-parse refs/notes/m^2 >actual &&
+	test_cmp pre_merge_z actual &&
 	# Merge commit mentions the notes refs merged
 	git log -1 --format=%B refs/notes/m > merge_commit_msg &&
 	grep -q refs/notes/m merge_commit_msg &&
@@ -428,14 +431,16 @@ test_expect_success 'redo merge of z into m (== y) with default ("manual") resol
 	# Verify that current notes tree (pre-merge) has not changed (m == y)
 	verify_notes y &&
 	verify_notes m &&
-	test "$(git rev-parse refs/notes/m)" = "$(cat pre_merge_y)"
+	git rev-parse refs/notes/m >actual &&
+	test_cmp pre_merge_y actual
 '
 
 test_expect_success 'abort notes merge' '
 	git notes merge --abort &&
 	notes_merge_files_gone &&
 	# m has not moved (still == y)
-	test "$(git rev-parse refs/notes/m)" = "$(cat pre_merge_y)" &&
+	git rev-parse refs/notes/m >actual &&
+	test_cmp pre_merge_y actual &&
 	# Verify that other notes refs has not changed (w, x, y and z)
 	verify_notes w &&
 	verify_notes x &&
@@ -460,7 +465,8 @@ test_expect_success 'redo merge of z into m (== y) with default ("manual") resol
 	# Verify that current notes tree (pre-merge) has not changed (m == y)
 	verify_notes y &&
 	verify_notes m &&
-	test "$(git rev-parse refs/notes/m)" = "$(cat pre_merge_y)"
+	git rev-parse refs/notes/m >actual &&
+	test_cmp pre_merge_y actual
 '
 
 cat <<EOF | sort >expect_notes_m
@@ -500,8 +506,10 @@ EOF
 	git notes merge --commit &&
 	notes_merge_files_gone &&
 	# Merge commit has pre-merge y and pre-merge z as parents
-	test "$(git rev-parse refs/notes/m^1)" = "$(cat pre_merge_y)" &&
-	test "$(git rev-parse refs/notes/m^2)" = "$(cat pre_merge_z)" &&
+	git rev-parse refs/notes/m^1 >actual &&
+	test_cmp pre_merge_y actual &&
+	git rev-parse refs/notes/m^2 >actual &&
+	test_cmp pre_merge_z actual &&
 	# Merge commit mentions the notes refs merged
 	git log -1 --format=%B refs/notes/m > merge_commit_msg &&
 	grep -q refs/notes/m merge_commit_msg &&
@@ -539,7 +547,8 @@ test_expect_success 'redo merge of z into m (== y) with default ("manual") resol
 	# Verify that current notes tree (pre-merge) has not changed (m == y)
 	verify_notes y &&
 	verify_notes m &&
-	test "$(git rev-parse refs/notes/m)" = "$(cat pre_merge_y)"
+	git rev-parse refs/notes/m >actual &&
+	test_cmp pre_merge_y actual
 '
 
 cp expect_notes_w expect_notes_m
@@ -548,7 +557,7 @@ cp expect_log_w expect_log_m
 test_expect_success 'reset notes ref m to somewhere else (w)' '
 	git update-ref refs/notes/m refs/notes/w &&
 	verify_notes m &&
-	test "$(git rev-parse refs/notes/m)" = "$(git rev-parse refs/notes/w)"
+	test_cmp_rev refs/notes/m refs/notes/w
 '
 
 test_expect_success 'fail to finalize conflicting merge if underlying ref has moved in the meantime (m != NOTES_MERGE_PARTIAL^1)' '
@@ -569,13 +578,15 @@ EOF
 	test_path_is_file .git/NOTES_MERGE_WORKTREE/$commit_sha3 &&
 	test_path_is_file .git/NOTES_MERGE_WORKTREE/$commit_sha4 &&
 	# 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)" &&
+	test_cmp_rev refs/notes/m refs/notes/w &&
+	test_cmp_rev refs/notes/y NOTES_MERGE_PARTIAL^1 &&
+	test_cmp_rev ! refs/notes/m NOTES_MERGE_PARTIAL^1 &&
 	# Mention refs/notes/m, and its current and expected value in output
 	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 &&
 	# Verify that other notes refs has not changed (w, x, y and z)
 	verify_notes w &&
 	verify_notes x &&
@@ -587,7 +598,7 @@ test_expect_success 'resolve situation by aborting the notes merge' '
 	git notes merge --abort &&
 	notes_merge_files_gone &&
 	# m has not moved (still == w)
-	test "$(git rev-parse refs/notes/m)" = "$(git rev-parse refs/notes/w)" &&
+	test_cmp_rev refs/notes/m refs/notes/w &&
 	# Verify that other notes refs has not changed (w, x, y and z)
 	verify_notes w &&
 	verify_notes x &&
@@ -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" &&
 		git notes merge --commit
 	) &&
 	git notes show HEAD > actual_notes &&
-- 
2.52.0

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
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
@@ -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" &&

Re: [PATCH v3] t3310: avoid hiding failures from rev-parse in command substitutions

From: Francesco Paparatto <hidden>
Date: 2026-03-07 10:18:04 

Eric Sunshine [off-list ref] writes:
quoted
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
@@ -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" &&
Thanks for the review. Both suggestions make sense. I'll use the
variable assignment for the rev-parse cases and test_write_lines for
the foo/bar case. I will send v4 shortly.

[PATCH v4] t3310: avoid hiding failures from rev-parse in command substitutions

From: Francesco Paparatto <hidden>
Date: 2026-03-07 10:36:38 

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>
---
 t/t3310-notes-merge-manual-resolve.sh | 47 +++++++++++++++++----------
 1 file changed, 29 insertions(+), 18 deletions(-)

diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh
index f0054b0a39..0bb366fdb8 100755
--- 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
 	# Verify that current notes tree (pre-merge) has not changed (m == y)
 	verify_notes y &&
 	verify_notes m &&
-	test "$(git rev-parse refs/notes/m)" = "$(cat pre_merge_y)"
+	git rev-parse refs/notes/m >actual &&
+	test_cmp pre_merge_y actual
 '
 
 cat <<EOF | sort >expect_notes_z
@@ -375,8 +376,10 @@ EOF
 	git notes merge --commit &&
 	notes_merge_files_gone &&
 	# Merge commit has pre-merge y and pre-merge z as parents
-	test "$(git rev-parse refs/notes/m^1)" = "$(cat pre_merge_y)" &&
-	test "$(git rev-parse refs/notes/m^2)" = "$(cat pre_merge_z)" &&
+	git rev-parse refs/notes/m^1 >actual &&
+	test_cmp pre_merge_y actual &&
+	git rev-parse refs/notes/m^2 >actual &&
+	test_cmp pre_merge_z actual &&
 	# Merge commit mentions the notes refs merged
 	git log -1 --format=%B refs/notes/m > merge_commit_msg &&
 	grep -q refs/notes/m merge_commit_msg &&
@@ -428,14 +431,16 @@ test_expect_success 'redo merge of z into m (== y) with default ("manual") resol
 	# Verify that current notes tree (pre-merge) has not changed (m == y)
 	verify_notes y &&
 	verify_notes m &&
-	test "$(git rev-parse refs/notes/m)" = "$(cat pre_merge_y)"
+	git rev-parse refs/notes/m >actual &&
+	test_cmp pre_merge_y actual
 '
 
 test_expect_success 'abort notes merge' '
 	git notes merge --abort &&
 	notes_merge_files_gone &&
 	# m has not moved (still == y)
-	test "$(git rev-parse refs/notes/m)" = "$(cat pre_merge_y)" &&
+	git rev-parse refs/notes/m >actual &&
+	test_cmp pre_merge_y actual &&
 	# Verify that other notes refs has not changed (w, x, y and z)
 	verify_notes w &&
 	verify_notes x &&
@@ -460,7 +465,8 @@ test_expect_success 'redo merge of z into m (== y) with default ("manual") resol
 	# Verify that current notes tree (pre-merge) has not changed (m == y)
 	verify_notes y &&
 	verify_notes m &&
-	test "$(git rev-parse refs/notes/m)" = "$(cat pre_merge_y)"
+	git rev-parse refs/notes/m >actual &&
+	test_cmp pre_merge_y actual
 '
 
 cat <<EOF | sort >expect_notes_m
@@ -500,8 +506,10 @@ EOF
 	git notes merge --commit &&
 	notes_merge_files_gone &&
 	# Merge commit has pre-merge y and pre-merge z as parents
-	test "$(git rev-parse refs/notes/m^1)" = "$(cat pre_merge_y)" &&
-	test "$(git rev-parse refs/notes/m^2)" = "$(cat pre_merge_z)" &&
+	git rev-parse refs/notes/m^1 >actual &&
+	test_cmp pre_merge_y actual &&
+	git rev-parse refs/notes/m^2 >actual &&
+	test_cmp pre_merge_z actual &&
 	# Merge commit mentions the notes refs merged
 	git log -1 --format=%B refs/notes/m > merge_commit_msg &&
 	grep -q refs/notes/m merge_commit_msg &&
@@ -539,7 +547,8 @@ test_expect_success 'redo merge of z into m (== y) with default ("manual") resol
 	# Verify that current notes tree (pre-merge) has not changed (m == y)
 	verify_notes y &&
 	verify_notes m &&
-	test "$(git rev-parse refs/notes/m)" = "$(cat pre_merge_y)"
+	git rev-parse refs/notes/m >actual &&
+	test_cmp pre_merge_y actual
 '
 
 cp expect_notes_w expect_notes_m
@@ -548,7 +557,7 @@ cp expect_log_w expect_log_m
 test_expect_success 'reset notes ref m to somewhere else (w)' '
 	git update-ref refs/notes/m refs/notes/w &&
 	verify_notes m &&
-	test "$(git rev-parse refs/notes/m)" = "$(git rev-parse refs/notes/w)"
+	test_cmp_rev refs/notes/m refs/notes/w
 '
 
 test_expect_success 'fail to finalize conflicting merge if underlying ref has moved in the meantime (m != NOTES_MERGE_PARTIAL^1)' '
@@ -569,13 +578,15 @@ EOF
 	test_path_is_file .git/NOTES_MERGE_WORKTREE/$commit_sha3 &&
 	test_path_is_file .git/NOTES_MERGE_WORKTREE/$commit_sha4 &&
 	# 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)" &&
+	test_cmp_rev refs/notes/m refs/notes/w &&
+	test_cmp_rev refs/notes/y NOTES_MERGE_PARTIAL^1 &&
+	test_cmp_rev ! refs/notes/m NOTES_MERGE_PARTIAL^1 &&
 	# Mention refs/notes/m, and its current and expected value in output
 	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 &&
+	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 &&
 	# Verify that other notes refs has not changed (w, x, y and z)
 	verify_notes w &&
 	verify_notes x &&
@@ -587,7 +598,7 @@ test_expect_success 'resolve situation by aborting the notes merge' '
 	git notes merge --abort &&
 	notes_merge_files_gone &&
 	# m has not moved (still == w)
-	test "$(git rev-parse refs/notes/m)" = "$(git rev-parse refs/notes/w)" &&
+	test_cmp_rev refs/notes/m refs/notes/w &&
 	# Verify that other notes refs has not changed (w, x, y and z)
 	verify_notes w &&
 	verify_notes x &&
@@ -606,8 +617,8 @@ 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) &&
+		test_write_lines foo bar >"$oid" &&
 		git notes merge --commit
 	) &&
 	git notes show HEAD > actual_notes &&
-- 
2.52.0

Re: [PATCH v4] t3310: avoid hiding failures from rev-parse in command substitutions

From: Eric Sunshine <hidden>
Date: 2026-03-08 04:13:15 

On Sat, Mar 7, 2026 at 5:36 AM Francesco Paparatto
[off-list ref] wrote:
quoted hunk
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>
---
@@ -569,13 +578,15 @@ EOF
-       test_grep -q "$(git rev-parse refs/notes/m)" output &&
-       test_grep -q "$(git rev-parse NOTES_MERGE_PARTIAL^1)" output &&
+       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 &&
@@ -606,8 +617,8 @@ test_expect_success 'switch cwd before committing notes merge' '
-               echo "foo" > $(git rev-parse HEAD) &&
-               echo "bar" >> $(git rev-parse HEAD) &&
+               oid=$(git rev-parse HEAD) &&
+               test_write_lines foo bar >"$oid" &&
Thank you, this version (v4) looks good; it addresses all my review
comments. For what it's worth:

    Reviewed-by: Eric Sunshine [off-list ref]
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help