Re: [PATCH v3 12/15] ref-filter: align: introduce long-form syntax

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 25, 2016, 23:45 UTC
Message-ID: <xmqqa8ntdxzs.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAPig+cT56AiO-3GNzia7UsGZFM5zu5zsEC31XPuq-by1p4+sbw@mail.gmail.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 7 quoted lines

>> +               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:

Show 9 quoted lines

>     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