On Fri, Dec 27, 2019 at 8:47 AM Denton Liu [off-list ref] 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...

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

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