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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 7, 2026, 06:29 UTC
Message-ID: <CAPig+cQWCK48GJEnGX7bP6exu847WR8HU3Y8sna525w6NEhmmw@mail.gmail.com>
In-Reply-To: <20260305225128.54283-1-francescopaparatto@gmail.com>

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