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

[Eric Sunshine <sunshine@sunshineco.com>]

On Fri, Jul 29, 2022 at 6:21 AM Jiang Xin <worldhello.net@gmail.com> wrote:
> 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.

> +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

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

Same here:

    esac &&
    shift

> +       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 `|`.

> +       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.

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

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