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

[Eric Sunshine <sunshine@sunshineco.com>]

On Fri, Dec 27, 2019 at 8:47 AM Denton Liu <liu.denton@gmail.com> wrote:
> 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...

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

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