threads / patch / 65133

patcht3310: avoid hiding failures from rev-parse in command substitutions

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

## tl;dr

15 messages between Mar 4, 2026 and Mar 8, 2026. Diffs are folded; open one to read it.

replies: 14people: 3as markdown or json

Francesco Paparatto· Mar 4, 2026, 12:19 UTC · lore

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.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Francesco Paparatto <francescopaparatto@gmail.com>
---
 t/t3310-notes-merge-manual-resolve.sh | 54 ++++++++++++++++++---------
 1 file changed, 36 insertions(+), 18 deletions(-)

Show changes to t/t3310-notes-merge-manual-resolve.sh +36 −18

diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh
index f0054b0a39..92a5951331 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)"
+	m=$(git rev-parse refs/notes/m) &&
+	test "$m" = "$(cat pre_merge_y)"
 '
 
 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)" &&
+	m1=$(git rev-parse refs/notes/m^1) &&
+	m2=$(git rev-parse refs/notes/m^2) &&
+	test "$m1" = "$(cat pre_merge_y)" &&
+	test "$m2" = "$(cat pre_merge_z)" &&
 	# 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)"
+	m=$(git rev-parse refs/notes/m) &&
+	test "$m" = "$(cat pre_merge_y)"
 '
 
 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)" &&
+	m=$(git rev-parse refs/notes/m) &&
+	test "$m" = "$(cat pre_merge_y)" &&
 	# 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)"
+	m=$(git rev-parse refs/notes/m) &&
+	test "$m" = "$(cat pre_merge_y)"
 '
 
 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)" &&
+	m1=$(git rev-parse refs/notes/m^1) &&
+	m2=$(git rev-parse refs/notes/m^2) &&
+	test "$m1" = "$(cat pre_merge_y)" &&
+	test "$m2" = "$(cat pre_merge_z)" &&
 	# 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)"
+	m=$(git rev-parse refs/notes/m) &&
+	test "$m" = "$(cat pre_merge_y)"
 '
 
 cp expect_notes_w expect_notes_m
@@ -548,7 +557,9 @@ 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)"
+	m=$(git rev-parse refs/notes/m) &&
+	w=$(git rev-parse refs/notes/w) &&
+	test "$m" = "$w"
 '
 
 test_expect_success 'fail to finalize conflicting merge if underlying ref has moved in the meantime (m != NOTES_MERGE_PARTIAL^1)' '
@@ -569,13 +580,17 @@ 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)" &&
+	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" &&
 	# 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 &&
+	test_grep -q "$m" output &&
+	test_grep -q "$p1" output &&
 	# Verify that other notes refs has not changed (w, x, y and z)
 	verify_notes w &&
 	verify_notes x &&
@@ -587,7 +602,9 @@ 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)" &&
+	m=$(git rev-parse refs/notes/m) &&
+	w=$(git rev-parse refs/notes/w) &&
+	test "$m" = "$w" &&
 	# Verify that other notes refs has not changed (w, x, y and z)
 	verify_notes w &&
 	verify_notes x &&
@@ -606,8 +623,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

Eric Sunshine· Mar 4, 2026, 22:21 UTC · re: Francesco Paparatto · lore

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

On Wed, Mar 4, 2026 at 7:20 AM Francesco Paparatto <francescopaparatto@gmail.com> wrote:

Show 6 quoted lines

> 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...

Show 8 quoted lines

> Signed-off-by: Francesco Paparatto <francescopaparatto@gmail.com>
> ---
> 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.

Show 12 quoted lines

> @@ -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.

Francesco Paparatto· Mar 5, 2026, 09:06 UTC · re: Eric Sunshine · lore

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

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 <gitster@pobox.com>
Signed-off-by: Francesco Paparatto <francescopaparatto@gmail.com>
---
 t/t3310-notes-merge-manual-resolve.sh | 60 ++++++++++++---------------
 1 file changed, 27 insertions(+), 33 deletions(-)

Show changes to t/t3310-notes-merge-manual-resolve.sh +27 −33

diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh
index 92a5951331..64c0a753ff 100755
--- a/t/t3310-notes-merge-manual-resolve.sh
+++ b/t/t3310-notes-merge-manual-resolve.sh
@@ -227,8 +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 &&
-	m=$(git rev-parse refs/notes/m) &&
-	test "$m" = "$(cat pre_merge_y)"
+	git rev-parse refs/notes/m >actual &&
+	test_cmp pre_merge_y actual
 '
 
 cat <<EOF | sort >expect_notes_z
@@ -376,10 +376,10 @@ EOF
 	git notes merge --commit &&
 	notes_merge_files_gone &&
 	# Merge commit has pre-merge y and pre-merge z as parents
-	m1=$(git rev-parse refs/notes/m^1) &&
-	m2=$(git rev-parse refs/notes/m^2) &&
-	test "$m1" = "$(cat pre_merge_y)" &&
-	test "$m2" = "$(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 &&
@@ -431,16 +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 &&
-	m=$(git rev-parse refs/notes/m) &&
-	test "$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)
-	m=$(git rev-parse refs/notes/m) &&
-	test "$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 &&
@@ -465,8 +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 &&
-	m=$(git rev-parse refs/notes/m) &&
-	test "$m" = "$(cat pre_merge_y)"
+	git rev-parse refs/notes/m >actual &&
+	test_cmp pre_merge_y actual
 '
 
 cat <<EOF | sort >expect_notes_m
@@ -506,10 +506,10 @@ EOF
 	git notes merge --commit &&
 	notes_merge_files_gone &&
 	# Merge commit has pre-merge y and pre-merge z as parents
-	m1=$(git rev-parse refs/notes/m^1) &&
-	m2=$(git rev-parse refs/notes/m^2) &&
-	test "$m1" = "$(cat pre_merge_y)" &&
-	test "$m2" = "$(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 &&
@@ -547,8 +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 &&
-	m=$(git rev-parse refs/notes/m) &&
-	test "$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
@@ -557,9 +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 &&
-	m=$(git rev-parse refs/notes/m) &&
-	w=$(git rev-parse refs/notes/w) &&
-	test "$m" = "$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)' '
@@ -580,17 +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
-	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" &&
+	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 "$m" output &&
-	test_grep -q "$p1" 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 &&
@@ -602,9 +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)
-	m=$(git rev-parse refs/notes/m) &&
-	w=$(git rev-parse refs/notes/w) &&
-	test "$m" = "$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 &&

-- 
2.52.0

Junio C Hamano· Mar 5, 2026, 19:13 UTC · re: Francesco Paparatto · lore

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

Francesco Paparatto <francescopaparatto@gmail.com> writes:

Show 13 quoted lines

> 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 <gitster@pobox.com>

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.

Show 7 quoted lines

> Signed-off-by: Francesco Paparatto <francescopaparatto@gmail.com>
> ---
>  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.

Eric Sunshine· Mar 5, 2026, 22:34 UTC · re: Junio C Hamano · lore

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

On Thu, Mar 5, 2026 at 2:13 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 17 quoted lines

> > 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 <gitster@pobox.com>
>
> 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.

Show 7 quoted lines

> >  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).

Francesco Paparatto· Mar 5, 2026, 22:49 UTC · re: Eric Sunshine · lore

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

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 6 quoted lines

> 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

Junio C Hamano· Mar 5, 2026, 23:06 UTC · re: Eric Sunshine · lore

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

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 11 quoted lines

>> > 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).

No. The v1 and this one touch separate areas and can go independently. The thing I had trouble with was that this did not apply to either on top of v1 (which by the way is already in 'next') nor on top of 'master'.

Junio C Hamano· Mar 5, 2026, 23:14 UTC · re: Junio C Hamano · lore

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

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

Show 18 quoted lines

> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>>> > 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).
>
> No.  The v1 and this one touch separate areas and can go
> independently.  The thing I had trouble with was that this did not
> apply to either on top of v1 (which by the way is already in 'next')
> nor on top of 'master'.

Ah, sorry, no. I was utterly confused. Somehow I mixed two unrelated patches on this same t3310 script. What went to 'next' was the other unrelated one, and I did not even take the v1 of this topic.

I'll look at v3 now.

Sorry for the confusion, and thanks for helping.

Francesco Paparatto· Mar 5, 2026, 22:42 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> 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

Francesco Paparatto· Mar 5, 2026, 22:51 UTC · re: Francesco Paparatto · lore

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

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.

Suggested-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Francesco Paparatto <francescopaparatto@gmail.com>
---
 t/t3310-notes-merge-manual-resolve.sh | 48 +++++++++++++++++----------
 1 file changed, 30 insertions(+), 18 deletions(-)

Show changes to t/t3310-notes-merge-manual-resolve.sh +30 −18

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

Eric Sunshine· Mar 7, 2026, 06:29 UTC · re: Francesco Paparatto · lore

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

On Thu, Mar 5, 2026 at 5:51 PM Francesco Paparatto <francescopaparatto@gmail.com> wrote:

Show 15 quoted lines

> 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 <sunshine@sunshineco.com>
> Signed-off-by: Francesco Paparatto <francescopaparatto@gmail.com>
> ---

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...

Show 9 quoted lines

> 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.

Show 9 quoted lines

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

Francesco Paparatto· Mar 7, 2026, 10:17 UTC · re: Eric Sunshine · lore

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

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 41 quoted lines

> > 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.
>
> > @@ -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.

Francesco Paparatto· Mar 7, 2026, 10:36 UTC · re: Francesco Paparatto · lore

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

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.

Suggested-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Francesco Paparatto <francescopaparatto@gmail.com>
---
 t/t3310-notes-merge-manual-resolve.sh | 47 +++++++++++++++++----------
 1 file changed, 29 insertions(+), 18 deletions(-)

Show changes to t/t3310-notes-merge-manual-resolve.sh +29 −18

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

Eric Sunshine· Mar 8, 2026, 04:13 UTC · re: Francesco Paparatto · lore

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

On Sat, Mar 7, 2026 at 5:36 AM Francesco Paparatto <francescopaparatto@gmail.com> wrote:

Show 27 quoted lines

> 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 <sunshine@sunshineco.com>
> Signed-off-by: Francesco Paparatto <francescopaparatto@gmail.com>
> ---
> @@ -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 <sunshine@sunshineco.com>

Junio C Hamano· Mar 8, 2026, 06:04 UTC · re: Eric Sunshine · lore

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

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 17 quoted lines

>> @@ -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 <sunshine@sunshineco.com>

Yup, looking good.