Re: [PATCH 1/9] t1416: more testcases for reference-transaction hook

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jul 30, 2022, 06:44 UTC
Message-ID: <CAPig+cQyW4Bz1kL5MriXeU6Zd93oYQU8ZuA-1gaEmAERpbTaDA@mail.gmail.com>
In-Reply-To: <20220729101245.6469-2-worldhello.net@gmail.com>

On Fri, Jul 29, 2022 at 6:21 AM Jiang Xin <worldhello.net@gmail.com> wrote:

Show 26 quoted lines

> Append more testcases in t1416 for various git commands that may trigger
> the "reference-transaction" hook.
> [...]
> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> ---
> diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh
> @@ -133,4 +133,1072 @@ test_expect_success 'interleaving hook calls succeed' '
> +# Create commits in <repo> and assign each commit's oid to shell variables
> +# given in the arguments (A, B, and C). E.g.:
> +#
> +#     create_commits_in <repo> A B C
> +#
> +# NOTE: Never calling this function from a subshell since variable
> +# assignments will disappear when subshell exits.
> +create_commits_in () {
> +       repo="$1" && test -d "$repo" ||
> +       error "Repository $repo does not exist."
> +       shift &&
> +       while test $# -gt 0
> +       do
> +               name=$1 &&
> +               shift &&
> +               test_commit -C "$repo" --no-tag "$name" &&
> +               eval $name=$(git -C "$repo" rev-parse HEAD)
> +       done
> +}

Since tests call this function within an &&-chain, we should make sure that &&-chain inside the function itself does the right thing. There are a couple important and one (somewhat optional) minor fix needed for this function. First, the function should manually break from the loop and indicate failure (using `|| return 1`) if any command inside the loop fails. Second, the `eval` is always going to return success even if the embedded `git rev-parse` command fails. Finally, the minor fix is that the `test ... || error ...` could be difficult for an &&-chain linter to grok if we ever start linting function bodies. To fix all these problems, you could perhaps write the function like this:

    create_commits_in () {
        local repo="$1" &&
        if ! test -d "$repo"
        then
            error "Repository $repo does not exist."
        fi &&
        shift &&
        while test $# -gt 0
        do
            local name=$1 &&
            shift &&
            test_commit -C "$repo" --no-tag "$name" &&
            local rev=$(git -C "$repo" rev-parse HEAD) &&
            eval "$name=$rev" || return 1
        done
    }

Now that the function breaks out of the loop properly with `|| return 1` upon failure, it's no longer necessary to perform the directory check at the top of the function since the call to test_commit() will correctly fail if the directory does not exist. So, the function can be shortened to:

    create_commits_in () {
        local repo="$1" &&
        shift &&
        while test $# -gt 0
        do
            local name=$1 &&
            shift &&
            test_commit -C "$repo" --no-tag "$name" &&
            local rev=$(git -C "$repo" rev-parse HEAD) &&
            eval $name=$rev || return 1
        done
    }

Having said all that, it almost seems overkill to build the loop into this function considering that it sets only four shell variables in the entire test script, so it might be simpler to drop the loop altogether:

    create_commits_in () {
        local repo="$1" name="$2" &&
        test_commit -C "$repo" --no-tag "$name" &&
        local rev=$(git -C "$repo" rev-parse HEAD) &&
        echo $rev
    }

and change the callers to invoke it individually for each variable:

    A=$(create_commits_in base A) &&
    B=$(create_commits_in base B) &&
    C=$(create_commits_in base C) &&

or even drop the function entirely:

    test_commit -C base --no-tag A &&
    A=$(git -C base rev-parse HEAD) &&
    test_commit -C base --no-tag B &&
    B=$(git -C base rev-parse HEAD) &&
    test_commit -C base --no-tag C &&
    C=$(git -C base rev-parse HEAD) &&

though, it's a matter of taste whether that's better.

Show 9 quoted lines

> +test_cmp_heads_and_tags () {
> +       indir= &&
> +       while test $# != 0
> +       do
> +               case "$1" in
> +               -C)
> +                       indir="$2"
> +                       shift
> +                       ;;

It wouldn't hurt to keep the &&-chain intact here in case the &&-chain linter is some day updated to check function bodies, so:

    indir="$2" &&
    shift

Show 5 quoted lines

> +               *)
> +                       break
> +                       ;;
> +               esac
> +               shift

Same here:

    esac &&
    shift

Show 8 quoted lines

> +       done &&
> +       expect=${1:-expect} &&
> +       actual=${2:-actual-heads-and-tags} &&
> +       indir=${indir:+"$indir"/} &&
> +       test_path_is_file "$expect" &&
> +       test_when_finished "rm -f \"$actual\"" &&
> +       git ${indir:+ -C "$indir"} show-ref --heads --tags | \
> +               make_user_friendly_and_stable_output >"$actual" &&

The exit code from `git show-ref` is being lost down the pipe. You also don't need the `\` after `|`.

Show 6 quoted lines

> +       test_cmp "$expect" "$actual"
> +}
> +
> +test_expect_success 'setup git config and common reference-transaction hook' '
> +       git config --global \
> +               core.hooksPath "$HOME/test-hooks" &&

Nit: This would fit nicely on a single line; no need for the line splicing.

Show 8 quoted lines

> +       git config --global core.abbrev 7 &&
> +       mkdir "test-hooks" &&
> +       write_script "test-hooks/reference-transaction" <<-EOF
> +               exec >>"$HOME/$HOOK_OUTPUT"
> +               printf "## Call hook: reference-transaction %9s ##\n" "\$@"
> +               while read -r line
> +               do
> +                   printf "%s\n" "\$line"

Nit This is the same as:

    echo "\$line"

Show 9 quoted lines

> +               done
> +       EOF
> +'
> +
> +test_expect_success "update-ref: create new refs" '
> +       test_when_finished "rm -f $HOOK_OUTPUT" &&
> +
> +       cat >expect <<-EOF &&
> +               ## Call hook: reference-transaction  prepared ##

This and a bunch of other here-doc tags in subsequent tests are missing the backslash:

    cat >expect <<-\EOF &&