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