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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 4, 2026, 22:21 UTC
Message-ID: <CAPig+cTHyB2sbBOELPb2=B5sU69OzSPU0JVn0p=2qMp=0=8vEg@mail.gmail.com>
In-Reply-To: <20260304121955.73794-1-francescopaparatto@gmail.com>

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.