Eric Sunshine <sunshine@xxxxxxxxxxxxxx> writes:
>> + git for-each-ref --format="|%(align:$option)refname is %(refname)%(end)|%(refname)" >actual &&
>
> This is not wrong, per se, but referencing $option inside the
> non-interpolating single-quote context of the test body makes it a bit
> harder to understand than it need be. As it is, at the time that the
> test body actually gets executed, $option still evaluates to the
> desired permutation value so it works.
I think I said this in one of my recent reviews to somebody else,
but this needs repeating.
My position on this has been and still is a complete opposite. It
is a designed behaviour that variable references made inside test
body that is quoted with sq pair can see the values of the variables
that are defined outside test_expect_success, and you can freely use
the global $TRASH_DIRECTORY, $_x40, etc. inside the test body that
is quoted with single quote. The $option here is no different.
In fact, it is more desirable to avoid double-quoting the test body.
The problem with using double-quote around the test body and
formulating the executable shell script using variable interpolation
before test_expect_success even runs is that you would be tempted to
do something silly like this:
HEAD_SHA1=$(git rev-parse --verify HEAD)
test_expect_success "check something" "
HEAD_SHA1=$(git rev-parse --verify HEAD) &&
... do other things that should not move the HEAD ... &&
test '$(git rev-parse HEAD)' = '$HEAD_SHA1' &&
test '$SOME_VAR' = '$OTHER_VAR' &&
"
It is not just error prone (if variable reference had a shell
metacharacter in it, e.g. a single-quote in OTHER_VAR's value, you'd
have a syntax error in the test body), but because two invocations
of rev-parse in the above are both made before test_expect_success
runs, and would yield the same value. It is not even testing the
right thing. If you broke rev-parse, the invocation will not fail
inside test_expect_success but outside when the arguments to that
helper is being prepared.
So please do not write something like this:
> test_align_permutations () {
> while ...
> do
> test_expect_success "align:$option" "
> git for-each-ref --format='...$option...' >actual &&
> ...
> "
> done
> }
but make it more like this:
for option in ...
do
test_expect_success "align:$option" '
git for-each-ref --format="...$option..." &&
...
'
done
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html