Re: [PATCH 2/8] t5512: generate references with generate_references()

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 25, 2020, 06:08 UTC
Message-ID: <CAPig+cQHZa4yELo3ncCVkbyrKWcPnZ4wjROMMbQGjm8dXK38GA@mail.gmail.com>
In-Reply-To: <674de50db28a50554d7af6e5c869c427d06f78aa.1585115341.git.liu.denton@gmail.com>

On Wed, Mar 25, 2020 at 1:55 AM Denton Liu <liu.denton@gmail.com> wrote:

> t5512: generate references with generate_references()

This summary doesn't say anything useful. How about this instead?

    t5512: stop losing git exit code in here-docs

> The expected references are generated using a here-doc with some inline
> subshells. If one of the `git rev-parse` invocations within the
> subshells failed, its return code is swallowed and we won't know about

s/failed/fails/

> it. Replace these here-docs with generate_references(), which actually
> reports when `git rev-parse` fails.

A couple nits below...

Show 9 quoted lines

> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
> diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
> @@ -4,6 +4,14 @@ test_description='git ls-remote'
> +generate_references () {
> +       for i
> +       do
> +               oid=$(git rev-parse "$i") || return 1
> +               printf '%s\t%s\n' "$oid" "$i"

I think the more usual way to say this in our test suite would be:

    oid=$(git rev-parse "$i") &&
    printf '%s\t%s\n' "$oid" "$i" || return 1

which has the nice property that someone can come along and insert additional code in the loop body before the final "|| return 1" without having to spend a lot of time trying to work out if the &&-chain is intact or broken.

Show 11 quoted lines

> +       done
> +}
> @@ -43,34 +51,19 @@ test_expect_success 'ls-remote self' '
>  test_expect_success 'ls-remote --sort="version:refname" --tags self' '
> -       cat >expect <<-EOF &&
> -       $(git rev-parse mark)   refs/tags/mark
> -       $(git rev-parse mark1.1)        refs/tags/mark1.1
> -       $(git rev-parse mark1.2)        refs/tags/mark1.2
> -       $(git rev-parse mark1.10)       refs/tags/mark1.10
> -       EOF
> +       generate_references refs/tags/mark refs/tags/mark1.1 refs/tags/mark1.2 refs/tags/mark1.10 >expect &&

This gets awfully wide and loses some readability. Perhaps:

    generate_references \
        refs/tags/mark \
        refs/tags/mark1.1 \
        refs/tags/mark1.2 \
        refs/tags/mark1.10 >expect &&

>         git ls-remote --sort="version:refname" --tags self >actual &&
>         test_cmp expect actual
>  '