{"thread":{"id":"65133","subject":"[PATCH] t3310: avoid hiding failures from rev-parse in command substitutions","startedAt":"2026-03-04T12:20:00Z","lastAt":"2026-03-08T06:04:14Z","messageCount":15,"participants":["Francesco Paparatto","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"537770","messageId":"20260304121955.73794-1-francescopaparatto@gmail.com","threadId":"65133","inReplyTo":null,"subject":"[PATCH] t3310: avoid hiding failures from rev-parse in command substitutions","fromName":"Francesco Paparatto","fromEmail":"francescopaparatto@gmail.com","sentAt":"2026-03-04T12:19:55Z","receivedAt":"2026-03-04T12:20:00Z","isPatch":true,"sender":{"key":"francescopaparatto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/133123206?v=4"},"body":"Running `git` commands inside command substitutions like\n\n    test \"$(git rev-parse A)\" = \"$(git rev-parse B)\"\n\ncan hide failures from the `git` invocations. Extract the\n`rev-parse` calls into variables so failures are not ignored.\n\nSuggested-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Francesco Paparatto <francescopaparatto@gmail.com>\n---\n t/t3310-notes-merge-manual-resolve.sh | 54 ++++++++++++++++++---------\n 1 file changed, 36 insertions(+), 18 deletions(-)\n\ndiff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh\nindex f0054b0a39..92a5951331 100755\n--- a/t/t3310-notes-merge-manual-resolve.sh\n+++ b/t/t3310-notes-merge-manual-resolve.sh\n@@ -227,7 +227,8 @@ test_expect_success 'merge z into m (== y) with default (\"manual\") resolver => C\n \t# Verify that current notes tree (pre-merge) has not changed (m == y)\n \tverify_notes y &&\n \tverify_notes m &&\n-\ttest \"$(git rev-parse refs/notes/m)\" = \"$(cat pre_merge_y)\"\n+\tm=$(git rev-parse refs/notes/m) &&\n+\ttest \"$m\" = \"$(cat pre_merge_y)\"\n '\n \n cat <<EOF | sort >expect_notes_z\n@@ -375,8 +376,10 @@ EOF\n \tgit notes merge --commit &&\n \tnotes_merge_files_gone &&\n \t# Merge commit has pre-merge y and pre-merge z as parents\n-\ttest \"$(git rev-parse refs/notes/m^1)\" = \"$(cat pre_merge_y)\" &&\n-\ttest \"$(git rev-parse refs/notes/m^2)\" = \"$(cat pre_merge_z)\" &&\n+\tm1=$(git rev-parse refs/notes/m^1) &&\n+\tm2=$(git rev-parse refs/notes/m^2) &&\n+\ttest \"$m1\" = \"$(cat pre_merge_y)\" &&\n+\ttest \"$m2\" = \"$(cat pre_merge_z)\" &&\n \t# Merge commit mentions the notes refs merged\n \tgit log -1 --format=%B refs/notes/m > merge_commit_msg &&\n \tgrep -q refs/notes/m merge_commit_msg &&\n@@ -428,14 +431,16 @@ test_expect_success 'redo merge of z into m (== y) with default (\"manual\") resol\n \t# Verify that current notes tree (pre-merge) has not changed (m == y)\n \tverify_notes y &&\n \tverify_notes m &&\n-\ttest \"$(git rev-parse refs/notes/m)\" = \"$(cat pre_merge_y)\"\n+\tm=$(git rev-parse refs/notes/m) &&\n+\ttest \"$m\" = \"$(cat pre_merge_y)\"\n '\n \n test_expect_success 'abort notes merge' '\n \tgit notes merge --abort &&\n \tnotes_merge_files_gone &&\n \t# m has not moved (still == y)\n-\ttest \"$(git rev-parse refs/notes/m)\" = \"$(cat pre_merge_y)\" &&\n+\tm=$(git rev-parse refs/notes/m) &&\n+\ttest \"$m\" = \"$(cat pre_merge_y)\" &&\n \t# Verify that other notes refs has not changed (w, x, y and z)\n \tverify_notes w &&\n \tverify_notes x &&\n@@ -460,7 +465,8 @@ test_expect_success 'redo merge of z into m (== y) with default (\"manual\") resol\n \t# Verify that current notes tree (pre-merge) has not changed (m == y)\n \tverify_notes y &&\n \tverify_notes m &&\n-\ttest \"$(git rev-parse refs/notes/m)\" = \"$(cat pre_merge_y)\"\n+\tm=$(git rev-parse refs/notes/m) &&\n+\ttest \"$m\" = \"$(cat pre_merge_y)\"\n '\n \n cat <<EOF | sort >expect_notes_m\n@@ -500,8 +506,10 @@ EOF\n \tgit notes merge --commit &&\n \tnotes_merge_files_gone &&\n \t# Merge commit has pre-merge y and pre-merge z as parents\n-\ttest \"$(git rev-parse refs/notes/m^1)\" = \"$(cat pre_merge_y)\" &&\n-\ttest \"$(git rev-parse refs/notes/m^2)\" = \"$(cat pre_merge_z)\" &&\n+\tm1=$(git rev-parse refs/notes/m^1) &&\n+\tm2=$(git rev-parse refs/notes/m^2) &&\n+\ttest \"$m1\" = \"$(cat pre_merge_y)\" &&\n+\ttest \"$m2\" = \"$(cat pre_merge_z)\" &&\n \t# Merge commit mentions the notes refs merged\n \tgit log -1 --format=%B refs/notes/m > merge_commit_msg &&\n \tgrep -q refs/notes/m merge_commit_msg &&\n@@ -539,7 +547,8 @@ test_expect_success 'redo merge of z into m (== y) with default (\"manual\") resol\n \t# Verify that current notes tree (pre-merge) has not changed (m == y)\n \tverify_notes y &&\n \tverify_notes m &&\n-\ttest \"$(git rev-parse refs/notes/m)\" = \"$(cat pre_merge_y)\"\n+\tm=$(git rev-parse refs/notes/m) &&\n+\ttest \"$m\" = \"$(cat pre_merge_y)\"\n '\n \n cp expect_notes_w expect_notes_m\n@@ -548,7 +557,9 @@ cp expect_log_w expect_log_m\n test_expect_success 'reset notes ref m to somewhere else (w)' '\n \tgit update-ref refs/notes/m refs/notes/w &&\n \tverify_notes m &&\n-\ttest \"$(git rev-parse refs/notes/m)\" = \"$(git rev-parse refs/notes/w)\"\n+\tm=$(git rev-parse refs/notes/m) &&\n+\tw=$(git rev-parse refs/notes/w) &&\n+\ttest \"$m\" = \"$w\"\n '\n \n test_expect_success 'fail to finalize conflicting merge if underlying ref has moved in the meantime (m != NOTES_MERGE_PARTIAL^1)' '\n@@ -569,13 +580,17 @@ EOF\n \ttest_path_is_file .git/NOTES_MERGE_WORKTREE/$commit_sha3 &&\n \ttest_path_is_file .git/NOTES_MERGE_WORKTREE/$commit_sha4 &&\n \t# Refs are unchanged\n-\ttest \"$(git rev-parse refs/notes/m)\" = \"$(git rev-parse refs/notes/w)\" &&\n-\ttest \"$(git rev-parse refs/notes/y)\" = \"$(git rev-parse NOTES_MERGE_PARTIAL^1)\" &&\n-\ttest \"$(git rev-parse refs/notes/m)\" != \"$(git rev-parse NOTES_MERGE_PARTIAL^1)\" &&\n+\tm=$(git rev-parse refs/notes/m) &&\n+\tw=$(git rev-parse refs/notes/w) &&\n+\ty=$(git rev-parse refs/notes/y) &&\n+\tp1=$(git rev-parse NOTES_MERGE_PARTIAL^1) &&\n+\ttest \"$m\" = \"$w\" &&\n+\ttest \"$y\" = \"$p1\" &&\n+\ttest \"$m\" != \"$p1\" &&\n \t# Mention refs/notes/m, and its current and expected value in output\n \ttest_grep -q \"refs/notes/m\" output &&\n-\ttest_grep -q \"$(git rev-parse refs/notes/m)\" output &&\n-\ttest_grep -q \"$(git rev-parse NOTES_MERGE_PARTIAL^1)\" output &&\n+\ttest_grep -q \"$m\" output &&\n+\ttest_grep -q \"$p1\" output &&\n \t# Verify that other notes refs has not changed (w, x, y and z)\n \tverify_notes w &&\n \tverify_notes x &&\n@@ -587,7 +602,9 @@ test_expect_success 'resolve situation by aborting the notes merge' '\n \tgit notes merge --abort &&\n \tnotes_merge_files_gone &&\n \t# m has not moved (still == w)\n-\ttest \"$(git rev-parse refs/notes/m)\" = \"$(git rev-parse refs/notes/w)\" &&\n+\tm=$(git rev-parse refs/notes/m) &&\n+\tw=$(git rev-parse refs/notes/w) &&\n+\ttest \"$m\" = \"$w\" &&\n \t# Verify that other notes refs has not changed (w, x, y and z)\n \tverify_notes w &&\n \tverify_notes x &&\n@@ -606,8 +623,9 @@ test_expect_success 'switch cwd before committing notes merge' '\n \ttest_must_fail git notes merge refs/notes/other &&\n \t(\n \t\tcd .git/NOTES_MERGE_WORKTREE &&\n-\t\techo \"foo\" > $(git rev-parse HEAD) &&\n-\t\techo \"bar\" >> $(git rev-parse HEAD) &&\n+\t\toid=$(git rev-parse HEAD) &&\n+\t\techo \"foo\" >\"$oid\" &&\n+\t\techo \"bar\" >>\"$oid\" &&\n \t\tgit notes merge --commit\n \t) &&\n \tgit notes show HEAD > actual_notes &&\n-- \n2.52.0\n\n"},{"id":"537849","messageId":"CAPig+cTHyB2sbBOELPb2=B5sU69OzSPU0JVn0p=2qMp=0=8vEg@mail.gmail.com","threadId":"65133","inReplyTo":"20260304121955.73794-1-francescopaparatto@gmail.com","subject":"Re: [PATCH] t3310: avoid hiding failures from rev-parse in command substitutions","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2026-03-04T22:21:56Z","receivedAt":"2026-03-04T22:22:09Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Mar 4, 2026 at 7:20 AM Francesco Paparatto\n<francescopaparatto@gmail.com> wrote:\n> Running `git` commands inside command substitutions like\n>\n>     test \"$(git rev-parse A)\" = \"$(git rev-parse B)\"\n>\n> can hide failures from the `git` invocations. Extract the\n> `rev-parse` calls into variables so failures are not ignored.\n\nOkay, surfacing failures of `git` invocations is a laudable goal. However...\n\n> Signed-off-by: Francesco Paparatto <francescopaparatto@gmail.com>\n> ---\n> diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh\n> @@ -227,7 +227,8 @@ test_expect_success 'merge z into m (== y) with default (\"manual\") resolver => C\n> -       test \"$(git rev-parse refs/notes/m)\" = \"$(cat pre_merge_y)\"\n> +       m=$(git rev-parse refs/notes/m) &&\n> +       test \"$m\" = \"$(cat pre_merge_y)\"\n>  '\n\n...a failure exposed by `test` is not very developer-friendly since it\ndoesn't give any indication about what went wrong. Since the pre-merge\nvalue of \"y\" (and also \"z\" in subsequent tests) is already in a file,\nwe can make the failure mode much more helpful by using `test_cmp\n<expect> <actual>` which will show both the expected and actual values\nwhen they don't match. Thus, the above transformation would be better\nstated along these lines:\n\n    git rev-parse refs/notes/m >actual &&\n    test_cmp pre_merge_y actual\n\nThe same comment applies to other changes in this patch.\n\n> @@ -569,13 +580,17 @@ EOF\n>         # Refs are unchanged\n> -       test \"$(git rev-parse refs/notes/m)\" = \"$(git rev-parse refs/notes/w)\" &&\n> -       test \"$(git rev-parse refs/notes/y)\" = \"$(git rev-parse NOTES_MERGE_PARTIAL^1)\" &&\n> -       test \"$(git rev-parse refs/notes/m)\" != \"$(git rev-parse NOTES_MERGE_PARTIAL^1)\" &&\n> +       m=$(git rev-parse refs/notes/m) &&\n> +       w=$(git rev-parse refs/notes/w) &&\n> +       y=$(git rev-parse refs/notes/y) &&\n> +       p1=$(git rev-parse NOTES_MERGE_PARTIAL^1) &&\n> +       test \"$m\" = \"$w\" &&\n> +       test \"$y\" = \"$p1\" &&\n> +       test \"$m\" != \"$p1\" &&\n\nIn this case we can do even better by taking advantage of\n`test_cmp_rev`, which would allow you to express the above more simply\nalong these lines:\n\n    test_cmp_rev refs/notes/m rev-parse refs/notes/w &&\n    test_cmp_rev refs/notes/y NOTES_MERGE_PARTIAL^1 &&\n    test_cmp_rev ! refs/notes/m NOTES_MERGE_PARTIAL^1 &&\n\nNote the \"!\" for negation in the third line.\n\nThe same comment applies to other changes in this patch.\n"},{"id":"537909","messageId":"20260305090602.22436-1-francescopaparatto@gmail.com","threadId":"65133","inReplyTo":"CAPig+cTHyB2sbBOELPb2=B5sU69OzSPU0JVn0p=2qMp=0=8vEg@mail.gmail.com","subject":"[PATCH v2] t3310: avoid hiding failures from rev-parse in command substitutions","fromName":"Francesco Paparatto","fromEmail":"francescopaparatto@gmail.com","sentAt":"2026-03-05T09:06:02Z","receivedAt":"2026-03-05T09:06:47Z","isPatch":true,"sender":{"key":"francescopaparatto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/133123206?v=4"},"body":"Running `git` commands inside command substitutions like\n\n\ttest \"$(git rev-parse A)\" = \"$(git rev-parse B)\"\n\ncan hide failures from the `git` invocations and provide little\ndiagnostic information when `test` fails.\n\nUse `test_cmp` when comparing against a stored expected value so\nmismatches show both expected and actual output. Use `test_cmp_rev`\nwhen comparing two revisions. These helpers produce clearer failure\noutput, making it easier to understand what went wrong.\n\nSuggested-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Francesco Paparatto <francescopaparatto@gmail.com>\n---\n t/t3310-notes-merge-manual-resolve.sh | 60 ++++++++++++---------------\n 1 file changed, 27 insertions(+), 33 deletions(-)\n\ndiff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh\nindex 92a5951331..64c0a753ff 100755\n--- a/t/t3310-notes-merge-manual-resolve.sh\n+++ b/t/t3310-notes-merge-manual-resolve.sh\n@@ -227,8 +227,8 @@ test_expect_success 'merge z into m (== y) with default (\"manual\") resolver => C\n \t# Verify that current notes tree (pre-merge) has not changed (m == y)\n \tverify_notes y &&\n \tverify_notes m &&\n-\tm=$(git rev-parse refs/notes/m) &&\n-\ttest \"$m\" = \"$(cat pre_merge_y)\"\n+\tgit rev-parse refs/notes/m >actual &&\n+\ttest_cmp pre_merge_y actual\n '\n \n cat <<EOF | sort >expect_notes_z\n@@ -376,10 +376,10 @@ EOF\n \tgit notes merge --commit &&\n \tnotes_merge_files_gone &&\n \t# Merge commit has pre-merge y and pre-merge z as parents\n-\tm1=$(git rev-parse refs/notes/m^1) &&\n-\tm2=$(git rev-parse refs/notes/m^2) &&\n-\ttest \"$m1\" = \"$(cat pre_merge_y)\" &&\n-\ttest \"$m2\" = \"$(cat pre_merge_z)\" &&\n+\tgit rev-parse refs/notes/m^1 >actual &&\n+\ttest_cmp pre_merge_y actual &&\n+\tgit rev-parse refs/notes/m^2 >actual &&\n+\ttest_cmp pre_merge_z actual &&\n \t# Merge commit mentions the notes refs merged\n \tgit log -1 --format=%B refs/notes/m > merge_commit_msg &&\n \tgrep -q refs/notes/m merge_commit_msg &&\n@@ -431,16 +431,16 @@ test_expect_success 'redo merge of z into m (== y) with default (\"manual\") resol\n \t# Verify that current notes tree (pre-merge) has not changed (m == y)\n \tverify_notes y &&\n \tverify_notes m &&\n-\tm=$(git rev-parse refs/notes/m) &&\n-\ttest \"$m\" = \"$(cat pre_merge_y)\"\n+\tgit rev-parse refs/notes/m >actual &&\n+\ttest_cmp pre_merge_y actual\n '\n \n test_expect_success 'abort notes merge' '\n \tgit notes merge --abort &&\n \tnotes_merge_files_gone &&\n \t# m has not moved (still == y)\n-\tm=$(git rev-parse refs/notes/m) &&\n-\ttest \"$m\" = \"$(cat pre_merge_y)\" &&\n+\tgit rev-parse refs/notes/m >actual &&\n+\ttest_cmp pre_merge_y actual &&\n \t# Verify that other notes refs has not changed (w, x, y and z)\n \tverify_notes w &&\n \tverify_notes x &&\n@@ -465,8 +465,8 @@ test_expect_success 'redo merge of z into m (== y) with default (\"manual\") resol\n \t# Verify that current notes tree (pre-merge) has not changed (m == y)\n \tverify_notes y &&\n \tverify_notes m &&\n-\tm=$(git rev-parse refs/notes/m) &&\n-\ttest \"$m\" = \"$(cat pre_merge_y)\"\n+\tgit rev-parse refs/notes/m >actual &&\n+\ttest_cmp pre_merge_y actual\n '\n \n cat <<EOF | sort >expect_notes_m\n@@ -506,10 +506,10 @@ EOF\n \tgit notes merge --commit &&\n \tnotes_merge_files_gone &&\n \t# Merge commit has pre-merge y and pre-merge z as parents\n-\tm1=$(git rev-parse refs/notes/m^1) &&\n-\tm2=$(git rev-parse refs/notes/m^2) &&\n-\ttest \"$m1\" = \"$(cat pre_merge_y)\" &&\n-\ttest \"$m2\" = \"$(cat pre_merge_z)\" &&\n+\tgit rev-parse refs/notes/m^1 >actual &&\n+\ttest_cmp pre_merge_y actual &&\n+\tgit rev-parse refs/notes/m^2 >actual &&\n+\ttest_cmp pre_merge_z actual &&\n \t# Merge commit mentions the notes refs merged\n \tgit log -1 --format=%B refs/notes/m > merge_commit_msg &&\n \tgrep -q refs/notes/m merge_commit_msg &&\n@@ -547,8 +547,8 @@ test_expect_success 'redo merge of z into m (== y) with default (\"manual\") resol\n \t# Verify that current notes tree (pre-merge) has not changed (m == y)\n \tverify_notes y &&\n \tverify_notes m &&\n-\tm=$(git rev-parse refs/notes/m) &&\n-\ttest \"$m\" = \"$(cat pre_merge_y)\"\n+\tgit rev-parse refs/notes/m >actual &&\n+\ttest_cmp pre_merge_y actual\n '\n \n cp expect_notes_w expect_notes_m\n@@ -557,9 +557,7 @@ cp expect_log_w expect_log_m\n test_expect_success 'reset notes ref m to somewhere else (w)' '\n \tgit update-ref refs/notes/m refs/notes/w &&\n \tverify_notes m &&\n-\tm=$(git rev-parse refs/notes/m) &&\n-\tw=$(git rev-parse refs/notes/w) &&\n-\ttest \"$m\" = \"$w\"\n+\ttest_cmp_rev refs/notes/m refs/notes/w\n '\n \n test_expect_success 'fail to finalize conflicting merge if underlying ref has moved in the meantime (m != NOTES_MERGE_PARTIAL^1)' '\n@@ -580,17 +578,15 @@ EOF\n \ttest_path_is_file .git/NOTES_MERGE_WORKTREE/$commit_sha3 &&\n \ttest_path_is_file .git/NOTES_MERGE_WORKTREE/$commit_sha4 &&\n \t# Refs are unchanged\n-\tm=$(git rev-parse refs/notes/m) &&\n-\tw=$(git rev-parse refs/notes/w) &&\n-\ty=$(git rev-parse refs/notes/y) &&\n-\tp1=$(git rev-parse NOTES_MERGE_PARTIAL^1) &&\n-\ttest \"$m\" = \"$w\" &&\n-\ttest \"$y\" = \"$p1\" &&\n-\ttest \"$m\" != \"$p1\" &&\n+\ttest_cmp_rev refs/notes/m refs/notes/w &&\n+\ttest_cmp_rev refs/notes/y NOTES_MERGE_PARTIAL^1 &&\n+\ttest_cmp_rev ! refs/notes/m NOTES_MERGE_PARTIAL^1 &&\n \t# Mention refs/notes/m, and its current and expected value in output\n \ttest_grep -q \"refs/notes/m\" output &&\n-\ttest_grep -q \"$m\" output &&\n-\ttest_grep -q \"$p1\" output &&\n+\tgit rev-parse refs/notes/m >actual &&\n+\ttest_grep -q \"$(cat actual)\" output &&\n+\tgit rev-parse NOTES_MERGE_PARTIAL^1 >actual &&\n+\ttest_grep -q \"$(cat actual)\" output &&\n \t# Verify that other notes refs has not changed (w, x, y and z)\n \tverify_notes w &&\n \tverify_notes x &&\n@@ -602,9 +598,7 @@ test_expect_success 'resolve situation by aborting the notes merge' '\n \tgit notes merge --abort &&\n \tnotes_merge_files_gone &&\n \t# m has not moved (still == w)\n-\tm=$(git rev-parse refs/notes/m) &&\n-\tw=$(git rev-parse refs/notes/w) &&\n-\ttest \"$m\" = \"$w\" &&\n+\ttest_cmp_rev refs/notes/m refs/notes/w &&\n \t# Verify that other notes refs has not changed (w, x, y and z)\n \tverify_notes w &&\n \tverify_notes x &&\n-- \n2.52.0\n\n"},{"id":"537989","messageId":"xmqq5x7a3x9w.fsf@gitster.g","threadId":"65133","inReplyTo":"20260305090602.22436-1-francescopaparatto@gmail.com","subject":"Re: [PATCH v2] t3310: avoid hiding failures from rev-parse in command substitutions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-05T19:13:15Z","receivedAt":"2026-03-05T19:13:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Francesco Paparatto <francescopaparatto@gmail.com> writes:\n\n> Running `git` commands inside command substitutions like\n>\n> \ttest \"$(git rev-parse A)\" = \"$(git rev-parse B)\"\n>\n> can hide failures from the `git` invocations and provide little\n> diagnostic information when `test` fails.\n>\n> Use `test_cmp` when comparing against a stored expected value so\n> mismatches show both expected and actual output. Use `test_cmp_rev`\n> when comparing two revisions. These helpers produce clearer failure\n> output, making it easier to understand what went wrong.\n>\n> Suggested-by: Junio C Hamano <gitster@pobox.com>\n\nHmph, did I suggest this?  I know Eric had comments on a previous\nround, and the improvements in this patch seems to be influenced a\nlot stronger by his input than whatever I may have said.\n\n> Signed-off-by: Francesco Paparatto <francescopaparatto@gmail.com>\n> ---\n>  t/t3310-notes-merge-manual-resolve.sh | 60 ++++++++++++---------------\n>  1 file changed, 27 insertions(+), 33 deletions(-)\n>\n> diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh\n> index 92a5951331..64c0a753ff 100755\n\nOn top of what commit is this patch designed to apply?\n\nThanks.\n"},{"id":"538008","messageId":"CAPig+cTsYWVg0nrU7kMakOKQaqFSo=i_nZ=_YuCJK_hq5gdZPQ@mail.gmail.com","threadId":"65133","inReplyTo":"xmqq5x7a3x9w.fsf@gitster.g","subject":"Re: [PATCH v2] t3310: avoid hiding failures from rev-parse in command substitutions","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2026-03-05T22:34:21Z","receivedAt":"2026-03-05T22:34:34Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Mar 5, 2026 at 2:13 PM Junio C Hamano <gitster@pobox.com> wrote:\n> > Running `git` commands inside command substitutions like\n> >\n> >       test \"$(git rev-parse A)\" = \"$(git rev-parse B)\"\n> >\n> > can hide failures from the `git` invocations and provide little\n> > diagnostic information when `test` fails.\n> >\n> > Use `test_cmp` when comparing against a stored expected value so\n> > mismatches show both expected and actual output. Use `test_cmp_rev`\n> > when comparing two revisions. These helpers produce clearer failure\n> > output, making it easier to understand what went wrong.\n> >\n> > Suggested-by: Junio C Hamano <gitster@pobox.com>\n>\n> Hmph, did I suggest this?  I know Eric had comments on a previous\n> round, and the improvements in this patch seems to be influenced a\n> lot stronger by his input than whatever I may have said.\n\nIndeed. The use of test_cmp and test_cmp_rev makes this version much\nmore developer-friendly than v1. Nice.\n\n> >  t/t3310-notes-merge-manual-resolve.sh | 60 ++++++++++++---------------\n> >  1 file changed, 27 insertions(+), 33 deletions(-)\n> >\n> > diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh\n> > index 92a5951331..64c0a753ff 100755\n>\n> On top of what commit is this patch designed to apply?\n\nWhat Junio probably means is that you appear to have based v2 atop v1,\nbut instead you should squash v1 and v2 into a single patch, and send\nthat as v3 so that when the patch is finally accepted into his tree,\nit will appear to have been perfect from the start (because v1 and v2\nwill only exist in the mailing list archive, not in the Git project\nhistory).\n"},{"id":"538009","messageId":"CAEaT9_-h2MEshMHoyoW9kWQgt_EfQJXcxWSn+cXTSL4mKME=5w@mail.gmail.com","threadId":"65133","inReplyTo":"xmqq5x7a3x9w.fsf@gitster.g","subject":"Re: [PATCH v2] t3310: avoid hiding failures from rev-parse in command substitutions","fromName":"Francesco Paparatto","fromEmail":"francescopaparatto@gmail.com","sentAt":"2026-03-05T22:42:12Z","receivedAt":"2026-03-05T22:42:24Z","isPatch":true,"sender":{"key":"francescopaparatto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/133123206?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Hmph, did I suggest this?  I know Eric had comments on a previous\n> round, and the improvements in this patch seems to be influenced a\n> lot stronger by his input than whatever I may have said.\n\nSorry about the Suggested-by line. I added it because of your earlier\ncomment here:\n\n    https://public-inbox.org/git/xmqqv7fioueg.fsf@gitster.g/\n\nbut you're right that the concrete changes in this version were mostly\ninfluenced by Eric's review, so I'll drop that trailer.\n\n> On top of what commit is this patch designed to apply?\n\nThis patch is based on top of:\n\n    b3ec5aec2367262f464a33d6eab7a9f49fd413f1\n    (\"t3310: replace test -f/-d with test_path_is_file/test_path_is_dir\")\n\nI'll reroll and send a v3 of the patch shortly.\n\nThanks,\nFrancesco\n"},{"id":"538010","messageId":"CAEaT9__LELMsCVZoY57+JZ8S0AtcE4n=K2W-rqjJhz2UDPiUBA@mail.gmail.com","threadId":"65133","inReplyTo":"CAPig+cTsYWVg0nrU7kMakOKQaqFSo=i_nZ=_YuCJK_hq5gdZPQ@mail.gmail.com","subject":"Re: [PATCH v2] t3310: avoid hiding failures from rev-parse in command substitutions","fromName":"Francesco Paparatto","fromEmail":"francescopaparatto@gmail.com","sentAt":"2026-03-05T22:49:31Z","receivedAt":"2026-03-05T22:49:43Z","isPatch":true,"sender":{"key":"francescopaparatto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/133123206?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> What Junio probably means is that you appear to have based v2 atop v1,\n> but instead you should squash v1 and v2 into a single patch, and send\n> that as v3 so that when the patch is finally accepted into his tree,\n> it will appear to have been perfect from the start (because v1 and v2\n> will only exist in the mailing list archive, not in the Git project\n> history).\n\nSorry about that, and thanks for the clarification.\n\nI've squashed the changes and rerolled the patch based on your\nsuggestions. I've just sent v3 to the list.\n\nThanks,\nFrancesco\n"},{"id":"538011","messageId":"20260305225128.54283-1-francescopaparatto@gmail.com","threadId":"65133","inReplyTo":"CAEaT9_-h2MEshMHoyoW9kWQgt_EfQJXcxWSn+cXTSL4mKME=5w@mail.gmail.com","subject":"[PATCH v3] t3310: avoid hiding failures from rev-parse in command substitutions","fromName":"Francesco Paparatto","fromEmail":"francescopaparatto@gmail.com","sentAt":"2026-03-05T22:51:28Z","receivedAt":"2026-03-05T22:51:47Z","isPatch":true,"sender":{"key":"francescopaparatto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/133123206?v=4"},"body":"Running `git` commands inside command substitutions like\n\n    test \"$(git rev-parse A)\" = \"$(git rev-parse B)\"\n\ncan hide failures from the `git` invocations and provide little\ndiagnostic information when `test` fails.\n\nUse `test_cmp` when comparing against a stored expected value so\nmismatches show both expected and actual output. Use `test_cmp_rev`\nwhen comparing two revisions. These helpers produce clearer failure\noutput, making it easier to understand what went wrong.\n\nSuggested-by: Eric Sunshine <sunshine@sunshineco.com>\nSigned-off-by: Francesco Paparatto <francescopaparatto@gmail.com>\n---\n t/t3310-notes-merge-manual-resolve.sh | 48 +++++++++++++++++----------\n 1 file changed, 30 insertions(+), 18 deletions(-)\n\ndiff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh\nindex f0054b0a39..64c0a753ff 100755\n--- a/t/t3310-notes-merge-manual-resolve.sh\n+++ b/t/t3310-notes-merge-manual-resolve.sh\n@@ -227,7 +227,8 @@ test_expect_success 'merge z into m (== y) with default (\"manual\") resolver => C\n \t# Verify that current notes tree (pre-merge) has not changed (m == y)\n \tverify_notes y &&\n \tverify_notes m &&\n-\ttest \"$(git rev-parse refs/notes/m)\" = \"$(cat pre_merge_y)\"\n+\tgit rev-parse refs/notes/m >actual &&\n+\ttest_cmp pre_merge_y actual\n '\n \n cat <<EOF | sort >expect_notes_z\n@@ -375,8 +376,10 @@ EOF\n \tgit notes merge --commit &&\n \tnotes_merge_files_gone &&\n \t# Merge commit has pre-merge y and pre-merge z as parents\n-\ttest \"$(git rev-parse refs/notes/m^1)\" = \"$(cat pre_merge_y)\" &&\n-\ttest \"$(git rev-parse refs/notes/m^2)\" = \"$(cat pre_merge_z)\" &&\n+\tgit rev-parse refs/notes/m^1 >actual &&\n+\ttest_cmp pre_merge_y actual &&\n+\tgit rev-parse refs/notes/m^2 >actual &&\n+\ttest_cmp pre_merge_z actual &&\n \t# Merge commit mentions the notes refs merged\n \tgit log -1 --format=%B refs/notes/m > merge_commit_msg &&\n \tgrep -q refs/notes/m merge_commit_msg &&\n@@ -428,14 +431,16 @@ test_expect_success 'redo merge of z into m (== y) with default (\"manual\") resol\n \t# Verify that current notes tree (pre-merge) has not changed (m == y)\n \tverify_notes y &&\n \tverify_notes m &&\n-\ttest \"$(git rev-parse refs/notes/m)\" = \"$(cat pre_merge_y)\"\n+\tgit rev-parse refs/notes/m >actual &&\n+\ttest_cmp pre_merge_y actual\n '\n \n test_expect_success 'abort notes merge' '\n \tgit notes merge --abort &&\n \tnotes_merge_files_gone &&\n \t# m has not moved (still == y)\n-\ttest \"$(git rev-parse refs/notes/m)\" = \"$(cat pre_merge_y)\" &&\n+\tgit rev-parse refs/notes/m >actual &&\n+\ttest_cmp pre_merge_y actual &&\n \t# Verify that other notes refs has not changed (w, x, y and z)\n \tverify_notes w &&\n \tverify_notes x &&\n@@ -460,7 +465,8 @@ test_expect_success 'redo merge of z into m (== y) with default (\"manual\") resol\n \t# Verify that current notes tree (pre-merge) has not changed (m == y)\n \tverify_notes y &&\n \tverify_notes m &&\n-\ttest \"$(git rev-parse refs/notes/m)\" = \"$(cat pre_merge_y)\"\n+\tgit rev-parse refs/notes/m >actual &&\n+\ttest_cmp pre_merge_y actual\n '\n \n cat <<EOF | sort >expect_notes_m\n@@ -500,8 +506,10 @@ EOF\n \tgit notes merge --commit &&\n \tnotes_merge_files_gone &&\n \t# Merge commit has pre-merge y and pre-merge z as parents\n-\ttest \"$(git rev-parse refs/notes/m^1)\" = \"$(cat pre_merge_y)\" &&\n-\ttest \"$(git rev-parse refs/notes/m^2)\" = \"$(cat pre_merge_z)\" &&\n+\tgit rev-parse refs/notes/m^1 >actual &&\n+\ttest_cmp pre_merge_y actual &&\n+\tgit rev-parse refs/notes/m^2 >actual &&\n+\ttest_cmp pre_merge_z actual &&\n \t# Merge commit mentions the notes refs merged\n \tgit log -1 --format=%B refs/notes/m > merge_commit_msg &&\n \tgrep -q refs/notes/m merge_commit_msg &&\n@@ -539,7 +547,8 @@ test_expect_success 'redo merge of z into m (== y) with default (\"manual\") resol\n \t# Verify that current notes tree (pre-merge) has not changed (m == y)\n \tverify_notes y &&\n \tverify_notes m &&\n-\ttest \"$(git rev-parse refs/notes/m)\" = \"$(cat pre_merge_y)\"\n+\tgit rev-parse refs/notes/m >actual &&\n+\ttest_cmp pre_merge_y actual\n '\n \n cp expect_notes_w expect_notes_m\n@@ -548,7 +557,7 @@ cp expect_log_w expect_log_m\n test_expect_success 'reset notes ref m to somewhere else (w)' '\n \tgit update-ref refs/notes/m refs/notes/w &&\n \tverify_notes m &&\n-\ttest \"$(git rev-parse refs/notes/m)\" = \"$(git rev-parse refs/notes/w)\"\n+\ttest_cmp_rev refs/notes/m refs/notes/w\n '\n \n test_expect_success 'fail to finalize conflicting merge if underlying ref has moved in the meantime (m != NOTES_MERGE_PARTIAL^1)' '\n@@ -569,13 +578,15 @@ EOF\n \ttest_path_is_file .git/NOTES_MERGE_WORKTREE/$commit_sha3 &&\n \ttest_path_is_file .git/NOTES_MERGE_WORKTREE/$commit_sha4 &&\n \t# Refs are unchanged\n-\ttest \"$(git rev-parse refs/notes/m)\" = \"$(git rev-parse refs/notes/w)\" &&\n-\ttest \"$(git rev-parse refs/notes/y)\" = \"$(git rev-parse NOTES_MERGE_PARTIAL^1)\" &&\n-\ttest \"$(git rev-parse refs/notes/m)\" != \"$(git rev-parse NOTES_MERGE_PARTIAL^1)\" &&\n+\ttest_cmp_rev refs/notes/m refs/notes/w &&\n+\ttest_cmp_rev refs/notes/y NOTES_MERGE_PARTIAL^1 &&\n+\ttest_cmp_rev ! refs/notes/m NOTES_MERGE_PARTIAL^1 &&\n \t# Mention refs/notes/m, and its current and expected value in output\n \ttest_grep -q \"refs/notes/m\" output &&\n-\ttest_grep -q \"$(git rev-parse refs/notes/m)\" output &&\n-\ttest_grep -q \"$(git rev-parse NOTES_MERGE_PARTIAL^1)\" output &&\n+\tgit rev-parse refs/notes/m >actual &&\n+\ttest_grep -q \"$(cat actual)\" output &&\n+\tgit rev-parse NOTES_MERGE_PARTIAL^1 >actual &&\n+\ttest_grep -q \"$(cat actual)\" output &&\n \t# Verify that other notes refs has not changed (w, x, y and z)\n \tverify_notes w &&\n \tverify_notes x &&\n@@ -587,7 +598,7 @@ test_expect_success 'resolve situation by aborting the notes merge' '\n \tgit notes merge --abort &&\n \tnotes_merge_files_gone &&\n \t# m has not moved (still == w)\n-\ttest \"$(git rev-parse refs/notes/m)\" = \"$(git rev-parse refs/notes/w)\" &&\n+\ttest_cmp_rev refs/notes/m refs/notes/w &&\n \t# Verify that other notes refs has not changed (w, x, y and z)\n \tverify_notes w &&\n \tverify_notes x &&\n@@ -606,8 +617,9 @@ test_expect_success 'switch cwd before committing notes merge' '\n \ttest_must_fail git notes merge refs/notes/other &&\n \t(\n \t\tcd .git/NOTES_MERGE_WORKTREE &&\n-\t\techo \"foo\" > $(git rev-parse HEAD) &&\n-\t\techo \"bar\" >> $(git rev-parse HEAD) &&\n+\t\toid=$(git rev-parse HEAD) &&\n+\t\techo \"foo\" >\"$oid\" &&\n+\t\techo \"bar\" >>\"$oid\" &&\n \t\tgit notes merge --commit\n \t) &&\n \tgit notes show HEAD > actual_notes &&\n-- \n2.52.0\n\n"},{"id":"538013","messageId":"xmqqv7f927x0.fsf@gitster.g","threadId":"65133","inReplyTo":"CAPig+cTsYWVg0nrU7kMakOKQaqFSo=i_nZ=_YuCJK_hq5gdZPQ@mail.gmail.com","subject":"Re: [PATCH v2] t3310: avoid hiding failures from rev-parse in command substitutions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-05T23:06:19Z","receivedAt":"2026-03-05T23:06:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n>> > diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh\n>> > index 92a5951331..64c0a753ff 100755\n>>\n>> On top of what commit is this patch designed to apply?\n>\n> What Junio probably means is that you appear to have based v2 atop v1,\n> but instead you should squash v1 and v2 into a single patch, and send\n> that as v3 so that when the patch is finally accepted into his tree,\n> it will appear to have been perfect from the start (because v1 and v2\n> will only exist in the mailing list archive, not in the Git project\n> history).\n\nNo.  The v1 and this one touch separate areas and can go\nindependently.  The thing I had trouble with was that this did not\napply to either on top of v1 (which by the way is already in 'next')\nnor on top of 'master'.\n"},{"id":"538019","messageId":"xmqqqzpx27jt.fsf@gitster.g","threadId":"65133","inReplyTo":"xmqqv7f927x0.fsf@gitster.g","subject":"Re: [PATCH v2] t3310: avoid hiding failures from rev-parse in command substitutions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-05T23:14:14Z","receivedAt":"2026-03-05T23:14:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Eric Sunshine <sunshine@sunshineco.com> writes:\n>\n>>> > diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh\n>>> > index 92a5951331..64c0a753ff 100755\n>>>\n>>> On top of what commit is this patch designed to apply?\n>>\n>> What Junio probably means is that you appear to have based v2 atop v1,\n>> but instead you should squash v1 and v2 into a single patch, and send\n>> that as v3 so that when the patch is finally accepted into his tree,\n>> it will appear to have been perfect from the start (because v1 and v2\n>> will only exist in the mailing list archive, not in the Git project\n>> history).\n>\n> No.  The v1 and this one touch separate areas and can go\n> independently.  The thing I had trouble with was that this did not\n> apply to either on top of v1 (which by the way is already in 'next')\n> nor on top of 'master'.\n\nAh, sorry, no.  I was utterly confused.  Somehow I mixed two\nunrelated patches on this same t3310 script.  What went to 'next'\nwas the other unrelated one, and I did not even take the v1 of this\ntopic.\n\nI'll look at v3 now.\n\nSorry for the confusion, and thanks for helping.\n"},{"id":"538164","messageId":"CAPig+cQWCK48GJEnGX7bP6exu847WR8HU3Y8sna525w6NEhmmw@mail.gmail.com","threadId":"65133","inReplyTo":"20260305225128.54283-1-francescopaparatto@gmail.com","subject":"Re: [PATCH v3] t3310: avoid hiding failures from rev-parse in command substitutions","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2026-03-07T06:29:29Z","receivedAt":"2026-03-07T06:29:45Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Mar 5, 2026 at 5:51 PM Francesco Paparatto\n<francescopaparatto@gmail.com> wrote:\n> Running `git` commands inside command substitutions like\n>\n>     test \"$(git rev-parse A)\" = \"$(git rev-parse B)\"\n>\n> can hide failures from the `git` invocations and provide little\n> diagnostic information when `test` fails.\n>\n> Use `test_cmp` when comparing against a stored expected value so\n> mismatches show both expected and actual output. Use `test_cmp_rev`\n> when comparing two revisions. These helpers produce clearer failure\n> output, making it easier to understand what went wrong.\n>\n> Suggested-by: Eric Sunshine <sunshine@sunshineco.com>\n> Signed-off-by: Francesco Paparatto <francescopaparatto@gmail.com>\n> ---\n\nThank you. This version looks much better and addresses my review\ncomments on the previous round. I do have one actionable\nrecommendation and one subjective comment, though...\n\n> diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh\n> @@ -569,13 +578,15 @@ EOF\n>         test_grep -q \"refs/notes/m\" output &&\n> -       test_grep -q \"$(git rev-parse refs/notes/m)\" output &&\n> -       test_grep -q \"$(git rev-parse NOTES_MERGE_PARTIAL^1)\" output &&\n> +       git rev-parse refs/notes/m >actual &&\n> +       test_grep -q \"$(cat actual)\" output &&\n> +       git rev-parse NOTES_MERGE_PARTIAL^1 >actual &&\n> +       test_grep -q \"$(cat actual)\" output &&\n\nStoring the output of git-rev-parse in a file only to read it back out\nof that file a moment later is unnecessarily roundabout. It would\ninstead be cleaner to do it this way:\n\n    oid=$(git rev-parse refs/notes/m) &&\n    test_grep -q \"$oid\" output &&\n    oid=$(git rev-parse NOTES_MERGE_PARTIAL^1) &&\n    test_grep -q \"$oid\" output &&\n\nUnlike this original in which git-rev-parse's exit code was lost due\nto being embedded in the test_grep invocation, this rewrite is safe\nbecause the exit code of git-rev-parse becomes the exit code of the\nvariable assignment, thus correctly aborts the test (due to the\n&&-chain) if git-rev-parse fails.\n\n> @@ -606,8 +617,9 @@ test_expect_success 'switch cwd before committing notes merge' '\n>         test_must_fail git notes merge refs/notes/other &&\n>         (\n>                 cd .git/NOTES_MERGE_WORKTREE &&\n> -               echo \"foo\" > $(git rev-parse HEAD) &&\n> -               echo \"bar\" >> $(git rev-parse HEAD) &&\n> +               oid=$(git rev-parse HEAD) &&\n> +               echo \"foo\" >\"$oid\" &&\n> +               echo \"bar\" >>\"$oid\" &&\n\nThis is purely subjective and you don't have to take the suggestion,\nbut although yours is a faithful rewrite (which is good), I probably\nwould have simplified this to:\n\n    oid=$(git rev-parse HEAD) &&\n    test_write_lines foo bar >\"$oid\" &&\n"},{"id":"538172","messageId":"CAEaT9_9_F_kcPbTrisX_At6RANJ9MHCEGka6M=WRezTO+-3A-g@mail.gmail.com","threadId":"65133","inReplyTo":"CAPig+cQWCK48GJEnGX7bP6exu847WR8HU3Y8sna525w6NEhmmw@mail.gmail.com","subject":"Re: [PATCH v3] t3310: avoid hiding failures from rev-parse in command substitutions","fromName":"Francesco Paparatto","fromEmail":"francescopaparatto@gmail.com","sentAt":"2026-03-07T10:17:51Z","receivedAt":"2026-03-07T10:18:04Z","isPatch":true,"sender":{"key":"francescopaparatto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/133123206?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> > diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh\n> > @@ -569,13 +578,15 @@ EOF\n> >         test_grep -q \"refs/notes/m\" output &&\n> > -       test_grep -q \"$(git rev-parse refs/notes/m)\" output &&\n> > -       test_grep -q \"$(git rev-parse NOTES_MERGE_PARTIAL^1)\" output &&\n> > +       git rev-parse refs/notes/m >actual &&\n> > +       test_grep -q \"$(cat actual)\" output &&\n> > +       git rev-parse NOTES_MERGE_PARTIAL^1 >actual &&\n> > +       test_grep -q \"$(cat actual)\" output &&\n>\n> Storing the output of git-rev-parse in a file only to read it back out\n> of that file a moment later is unnecessarily roundabout. It would\n> instead be cleaner to do it this way:\n>\n>     oid=$(git rev-parse refs/notes/m) &&\n>     test_grep -q \"$oid\" output &&\n>     oid=$(git rev-parse NOTES_MERGE_PARTIAL^1) &&\n>     test_grep -q \"$oid\" output &&\n>\n> Unlike this original in which git-rev-parse's exit code was lost due\n> to being embedded in the test_grep invocation, this rewrite is safe\n> because the exit code of git-rev-parse becomes the exit code of the\n> variable assignment, thus correctly aborts the test (due to the\n> &&-chain) if git-rev-parse fails.\n>\n> > @@ -606,8 +617,9 @@ test_expect_success 'switch cwd before committing notes merge' '\n> >         test_must_fail git notes merge refs/notes/other &&\n> >         (\n> >                 cd .git/NOTES_MERGE_WORKTREE &&\n> > -               echo \"foo\" > $(git rev-parse HEAD) &&\n> > -               echo \"bar\" >> $(git rev-parse HEAD) &&\n> > +               oid=$(git rev-parse HEAD) &&\n> > +               echo \"foo\" >\"$oid\" &&\n> > +               echo \"bar\" >>\"$oid\" &&\n>\n> This is purely subjective and you don't have to take the suggestion,\n> but although yours is a faithful rewrite (which is good), I probably\n> would have simplified this to:\n>\n>     oid=$(git rev-parse HEAD) &&\n>     test_write_lines foo bar >\"$oid\" &&\n\nThanks for the review. Both suggestions make sense. I'll use the\nvariable assignment for the rev-parse cases and test_write_lines for\nthe foo/bar case. I will send v4 shortly.\n"},{"id":"538174","messageId":"20260307103631.89829-1-francescopaparatto@gmail.com","threadId":"65133","inReplyTo":"20260305225128.54283-1-francescopaparatto@gmail.com","subject":"[PATCH v4] t3310: avoid hiding failures from rev-parse in command substitutions","fromName":"Francesco Paparatto","fromEmail":"francescopaparatto@gmail.com","sentAt":"2026-03-07T10:36:31Z","receivedAt":"2026-03-07T10:36:38Z","isPatch":true,"sender":{"key":"francescopaparatto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/133123206?v=4"},"body":"Running `git` commands inside command substitutions like\n\n    test \"$(git rev-parse A)\" = \"$(git rev-parse B)\"\n\ncan hide failures from the `git` invocations and provide little\ndiagnostic information when `test` fails.\n\nUse `test_cmp` when comparing against a stored expected value so\nmismatches show both expected and actual output. Use `test_cmp_rev`\nwhen comparing two revisions. These helpers produce clearer failure\noutput, making it easier to understand what went wrong.\n\nSuggested-by: Eric Sunshine <sunshine@sunshineco.com>\nSigned-off-by: Francesco Paparatto <francescopaparatto@gmail.com>\n---\n t/t3310-notes-merge-manual-resolve.sh | 47 +++++++++++++++++----------\n 1 file changed, 29 insertions(+), 18 deletions(-)\n\ndiff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh\nindex f0054b0a39..0bb366fdb8 100755\n--- a/t/t3310-notes-merge-manual-resolve.sh\n+++ b/t/t3310-notes-merge-manual-resolve.sh\n@@ -227,7 +227,8 @@ test_expect_success 'merge z into m (== y) with default (\"manual\") resolver => C\n \t# Verify that current notes tree (pre-merge) has not changed (m == y)\n \tverify_notes y &&\n \tverify_notes m &&\n-\ttest \"$(git rev-parse refs/notes/m)\" = \"$(cat pre_merge_y)\"\n+\tgit rev-parse refs/notes/m >actual &&\n+\ttest_cmp pre_merge_y actual\n '\n \n cat <<EOF | sort >expect_notes_z\n@@ -375,8 +376,10 @@ EOF\n \tgit notes merge --commit &&\n \tnotes_merge_files_gone &&\n \t# Merge commit has pre-merge y and pre-merge z as parents\n-\ttest \"$(git rev-parse refs/notes/m^1)\" = \"$(cat pre_merge_y)\" &&\n-\ttest \"$(git rev-parse refs/notes/m^2)\" = \"$(cat pre_merge_z)\" &&\n+\tgit rev-parse refs/notes/m^1 >actual &&\n+\ttest_cmp pre_merge_y actual &&\n+\tgit rev-parse refs/notes/m^2 >actual &&\n+\ttest_cmp pre_merge_z actual &&\n \t# Merge commit mentions the notes refs merged\n \tgit log -1 --format=%B refs/notes/m > merge_commit_msg &&\n \tgrep -q refs/notes/m merge_commit_msg &&\n@@ -428,14 +431,16 @@ test_expect_success 'redo merge of z into m (== y) with default (\"manual\") resol\n \t# Verify that current notes tree (pre-merge) has not changed (m == y)\n \tverify_notes y &&\n \tverify_notes m &&\n-\ttest \"$(git rev-parse refs/notes/m)\" = \"$(cat pre_merge_y)\"\n+\tgit rev-parse refs/notes/m >actual &&\n+\ttest_cmp pre_merge_y actual\n '\n \n test_expect_success 'abort notes merge' '\n \tgit notes merge --abort &&\n \tnotes_merge_files_gone &&\n \t# m has not moved (still == y)\n-\ttest \"$(git rev-parse refs/notes/m)\" = \"$(cat pre_merge_y)\" &&\n+\tgit rev-parse refs/notes/m >actual &&\n+\ttest_cmp pre_merge_y actual &&\n \t# Verify that other notes refs has not changed (w, x, y and z)\n \tverify_notes w &&\n \tverify_notes x &&\n@@ -460,7 +465,8 @@ test_expect_success 'redo merge of z into m (== y) with default (\"manual\") resol\n \t# Verify that current notes tree (pre-merge) has not changed (m == y)\n \tverify_notes y &&\n \tverify_notes m &&\n-\ttest \"$(git rev-parse refs/notes/m)\" = \"$(cat pre_merge_y)\"\n+\tgit rev-parse refs/notes/m >actual &&\n+\ttest_cmp pre_merge_y actual\n '\n \n cat <<EOF | sort >expect_notes_m\n@@ -500,8 +506,10 @@ EOF\n \tgit notes merge --commit &&\n \tnotes_merge_files_gone &&\n \t# Merge commit has pre-merge y and pre-merge z as parents\n-\ttest \"$(git rev-parse refs/notes/m^1)\" = \"$(cat pre_merge_y)\" &&\n-\ttest \"$(git rev-parse refs/notes/m^2)\" = \"$(cat pre_merge_z)\" &&\n+\tgit rev-parse refs/notes/m^1 >actual &&\n+\ttest_cmp pre_merge_y actual &&\n+\tgit rev-parse refs/notes/m^2 >actual &&\n+\ttest_cmp pre_merge_z actual &&\n \t# Merge commit mentions the notes refs merged\n \tgit log -1 --format=%B refs/notes/m > merge_commit_msg &&\n \tgrep -q refs/notes/m merge_commit_msg &&\n@@ -539,7 +547,8 @@ test_expect_success 'redo merge of z into m (== y) with default (\"manual\") resol\n \t# Verify that current notes tree (pre-merge) has not changed (m == y)\n \tverify_notes y &&\n \tverify_notes m &&\n-\ttest \"$(git rev-parse refs/notes/m)\" = \"$(cat pre_merge_y)\"\n+\tgit rev-parse refs/notes/m >actual &&\n+\ttest_cmp pre_merge_y actual\n '\n \n cp expect_notes_w expect_notes_m\n@@ -548,7 +557,7 @@ cp expect_log_w expect_log_m\n test_expect_success 'reset notes ref m to somewhere else (w)' '\n \tgit update-ref refs/notes/m refs/notes/w &&\n \tverify_notes m &&\n-\ttest \"$(git rev-parse refs/notes/m)\" = \"$(git rev-parse refs/notes/w)\"\n+\ttest_cmp_rev refs/notes/m refs/notes/w\n '\n \n test_expect_success 'fail to finalize conflicting merge if underlying ref has moved in the meantime (m != NOTES_MERGE_PARTIAL^1)' '\n@@ -569,13 +578,15 @@ EOF\n \ttest_path_is_file .git/NOTES_MERGE_WORKTREE/$commit_sha3 &&\n \ttest_path_is_file .git/NOTES_MERGE_WORKTREE/$commit_sha4 &&\n \t# Refs are unchanged\n-\ttest \"$(git rev-parse refs/notes/m)\" = \"$(git rev-parse refs/notes/w)\" &&\n-\ttest \"$(git rev-parse refs/notes/y)\" = \"$(git rev-parse NOTES_MERGE_PARTIAL^1)\" &&\n-\ttest \"$(git rev-parse refs/notes/m)\" != \"$(git rev-parse NOTES_MERGE_PARTIAL^1)\" &&\n+\ttest_cmp_rev refs/notes/m refs/notes/w &&\n+\ttest_cmp_rev refs/notes/y NOTES_MERGE_PARTIAL^1 &&\n+\ttest_cmp_rev ! refs/notes/m NOTES_MERGE_PARTIAL^1 &&\n \t# Mention refs/notes/m, and its current and expected value in output\n \ttest_grep -q \"refs/notes/m\" output &&\n-\ttest_grep -q \"$(git rev-parse refs/notes/m)\" output &&\n-\ttest_grep -q \"$(git rev-parse NOTES_MERGE_PARTIAL^1)\" output &&\n+\toid=$(git rev-parse refs/notes/m) &&\n+\ttest_grep -q \"$oid\" output &&\n+\toid=$(git rev-parse NOTES_MERGE_PARTIAL^1) &&\n+\ttest_grep -q \"$oid\" output &&\n \t# Verify that other notes refs has not changed (w, x, y and z)\n \tverify_notes w &&\n \tverify_notes x &&\n@@ -587,7 +598,7 @@ test_expect_success 'resolve situation by aborting the notes merge' '\n \tgit notes merge --abort &&\n \tnotes_merge_files_gone &&\n \t# m has not moved (still == w)\n-\ttest \"$(git rev-parse refs/notes/m)\" = \"$(git rev-parse refs/notes/w)\" &&\n+\ttest_cmp_rev refs/notes/m refs/notes/w &&\n \t# Verify that other notes refs has not changed (w, x, y and z)\n \tverify_notes w &&\n \tverify_notes x &&\n@@ -606,8 +617,8 @@ test_expect_success 'switch cwd before committing notes merge' '\n \ttest_must_fail git notes merge refs/notes/other &&\n \t(\n \t\tcd .git/NOTES_MERGE_WORKTREE &&\n-\t\techo \"foo\" > $(git rev-parse HEAD) &&\n-\t\techo \"bar\" >> $(git rev-parse HEAD) &&\n+\t\toid=$(git rev-parse HEAD) &&\n+\t\ttest_write_lines foo bar >\"$oid\" &&\n \t\tgit notes merge --commit\n \t) &&\n \tgit notes show HEAD > actual_notes &&\n-- \n2.52.0\n\n"},{"id":"538186","messageId":"CAPig+cTmRGBjV=yG4PvyyvFOgTZ0zK4GtkiO1xGSm1+OeM4ScQ@mail.gmail.com","threadId":"65133","inReplyTo":"20260307103631.89829-1-francescopaparatto@gmail.com","subject":"Re: [PATCH v4] t3310: avoid hiding failures from rev-parse in command substitutions","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2026-03-08T04:13:00Z","receivedAt":"2026-03-08T04:13:15Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Mar 7, 2026 at 5:36 AM Francesco Paparatto\n<francescopaparatto@gmail.com> wrote:\n> Running `git` commands inside command substitutions like\n>\n>     test \"$(git rev-parse A)\" = \"$(git rev-parse B)\"\n>\n> can hide failures from the `git` invocations and provide little\n> diagnostic information when `test` fails.\n>\n> Use `test_cmp` when comparing against a stored expected value so\n> mismatches show both expected and actual output. Use `test_cmp_rev`\n> when comparing two revisions. These helpers produce clearer failure\n> output, making it easier to understand what went wrong.\n>\n> Suggested-by: Eric Sunshine <sunshine@sunshineco.com>\n> Signed-off-by: Francesco Paparatto <francescopaparatto@gmail.com>\n> ---\n> @@ -569,13 +578,15 @@ EOF\n> -       test_grep -q \"$(git rev-parse refs/notes/m)\" output &&\n> -       test_grep -q \"$(git rev-parse NOTES_MERGE_PARTIAL^1)\" output &&\n> +       oid=$(git rev-parse refs/notes/m) &&\n> +       test_grep -q \"$oid\" output &&\n> +       oid=$(git rev-parse NOTES_MERGE_PARTIAL^1) &&\n> +       test_grep -q \"$oid\" output &&\n> @@ -606,8 +617,8 @@ test_expect_success 'switch cwd before committing notes merge' '\n> -               echo \"foo\" > $(git rev-parse HEAD) &&\n> -               echo \"bar\" >> $(git rev-parse HEAD) &&\n> +               oid=$(git rev-parse HEAD) &&\n> +               test_write_lines foo bar >\"$oid\" &&\n\nThank you, this version (v4) looks good; it addresses all my review\ncomments. For what it's worth:\n\n    Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>\n"},{"id":"538190","messageId":"xmqqecluuaar.fsf@gitster.g","threadId":"65133","inReplyTo":"CAPig+cTmRGBjV=yG4PvyyvFOgTZ0zK4GtkiO1xGSm1+OeM4ScQ@mail.gmail.com","subject":"Re: [PATCH v4] t3310: avoid hiding failures from rev-parse in command substitutions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-08T06:04:12Z","receivedAt":"2026-03-08T06:04:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n>> @@ -569,13 +578,15 @@ EOF\n>> -       test_grep -q \"$(git rev-parse refs/notes/m)\" output &&\n>> -       test_grep -q \"$(git rev-parse NOTES_MERGE_PARTIAL^1)\" output &&\n>> +       oid=$(git rev-parse refs/notes/m) &&\n>> +       test_grep -q \"$oid\" output &&\n>> +       oid=$(git rev-parse NOTES_MERGE_PARTIAL^1) &&\n>> +       test_grep -q \"$oid\" output &&\n>> @@ -606,8 +617,8 @@ test_expect_success 'switch cwd before committing notes merge' '\n>> -               echo \"foo\" > $(git rev-parse HEAD) &&\n>> -               echo \"bar\" >> $(git rev-parse HEAD) &&\n>> +               oid=$(git rev-parse HEAD) &&\n>> +               test_write_lines foo bar >\"$oid\" &&\n>\n> Thank you, this version (v4) looks good; it addresses all my review\n> comments. For what it's worth:\n>\n>     Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>\n\nYup, looking good.\n"}]}