From: Eric Sunshine Date: Sat, 07 Mar 2026 06:29:29 GMT Subject: Re: [PATCH v3] t3310: avoid hiding failures from rev-parse in command substitutions Message-ID: In-Reply-To: <20260305225128.54283-1-francescopaparatto@gmail.com> On Thu, Mar 5, 2026 at 5:51 PM Francesco Paparatto wrote: > Running `git` commands inside command substitutions like > > test "$(git rev-parse A)" = "$(git rev-parse B)" > > can hide failures from the `git` invocations and provide little > diagnostic information when `test` fails. > > Use `test_cmp` when comparing against a stored expected value so > mismatches show both expected and actual output. Use `test_cmp_rev` > when comparing two revisions. These helpers produce clearer failure > output, making it easier to understand what went wrong. > > Suggested-by: Eric Sunshine > Signed-off-by: Francesco Paparatto > --- 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... > 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" &&