Re: [PATCH 04/16] t2018: teach do_checkout() to accept `!` arg

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Dec 28, 2019, 08:34 UTC
Message-ID: <CAPig+cT242CjL9S0OcS1Pbsgf-u1N=SQzNfLdao03LU2OGEciQ@mail.gmail.com>
In-Reply-To: <4eab751a3cf00bbffaf4b1084928dad264fa1572.1577454401.git.liu.denton@gmail.com>

On Fri, Dec 27, 2019 at 8:47 AM Denton Liu <liu.denton@gmail.com> wrote:

Show 5 quoted lines

> Before, we were running `test_must_fail do_checkout`. However,
> `test_must_fail` should only be used on git commands. Teach
> do_checkout() to accept `!` as a potential first argument which will
> prepend `test_must_fail` to the enclosed git command and skips the
> remainder of the function.

There's a grammatical problem here. s/skips/skip/ is one way to fix it.

Use imperative mood when writing commit messages. Drop words such as "before" and "were". For instance:

    Stop using test_must_fail() with non-Git commands because...

(Same comment applies to pretty much all commit messages in this series.)

> This increases the granularity of the test as, instead of blindly
> checking that do_checkout() failed, we check that only the specific
> expected invocation of git fails.

This may be a case of trying to describe in prose too much of what is better described by the code itself. As a reviewer, I spent more time trying to figure out what this was saying that I did merely looking at the code and comprehending why the two checks following the git-checkout invocation should be skipped. Consequently, I lean toward dropping "...and skips the remainder..." through the end of the commit message.

More below...

Show 13 quoted lines

> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
> diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
> @@ -13,6 +13,12 @@ test_description='checkout'
>  #
>  # If <checkout options> is not specified, "git checkout" is run with -b.
>  do_checkout () {
> +       should_fail= &&
> +       if test "x$1" = "x!"
> +       then
> +               should_fail=test_must_fail &&
> +               shift
> +       fi &&

You forgot to update the function comment to talk about the new optional "!" argument.

> @@ -26,10 +32,13 @@ do_checkout () {
> -       git checkout $opts $exp_branch $exp_sha &&
> +       $should_fail git checkout $opts $exp_branch $exp_sha &&

If I read this literally, it says that the git checkout should always fail. A more wisely chosen variable name would help to alleviate this problem.

When you start parameterizing the actual invocation of a command like this (I'm not talking about the command arguments which are also parameterized), the abstraction level and cognitive load increase...

Show 8 quoted lines

> -       test $exp_ref = $(git rev-parse --symbolic-full-name HEAD) &&
> -       test $exp_sha = $(git rev-parse --verify HEAD)
> +       if test -z "$should_fail"
> +       then
> +               test $exp_ref = $(git rev-parse --symbolic-full-name HEAD) &&
> +               test $exp_sha = $(git rev-parse --verify HEAD)
> +       fi
>  }

You could reduce the cognitive load by making the code easier to understand at-a-glance (though at the cost of a minor bit of duplication) by structuring it instead like this:

    if test -n "$should_fail"
    then
        test_must_fail git checkout $opts $exp_branch $exp_sha
    else
        git checkout $opts $exp_branch $exp_sha &&
        test $exp_ref = $(git rev-parse --symbolic-full-name HEAD) &&
        test $exp_sha = $(git rev-parse --verify HEAD)
    fi

where 'should_fail' is either empty or non-empty depending upon whether "!" was supplied as an argument. (And, when coded this way, "should_fail" is a reasonable variable name.)