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

[Karthik Nayak <karthik.188@gmail.com>]

On Tue, Jan 26, 2016 at 5:15 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> 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
>

Thanks for this descriptive explanation, I shall use the suggested format.

-- 
Regards,
Karthik Nayak