Re: [PATCH v4 8/8] [Newcomer] t7004: Use single quotes instead of double quotes

[AbdAlRahman Gad <abdobngad@gmail.com>]

On 8/6/24 06:02, Eric Sunshine wrote:
> On Mon, Aug 5, 2024 at 8:00 PM AbdAlRahman Gad <abdobngad@gmail.com> wrote:
>> Some test bodies and test description are surrounded with double
>> quotes instead of single quotes, violating our coding style.
>>
>> Signed-off-by: AbdAlRahman Gad <abdobngad@gmail.com>
>> ---
>> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
>> @@ -1585,7 +1585,7 @@ test_expect_success 'creating third commit without tag' '
>> -test_expect_success 'checking that first commit is in all tags (hash)' "
>> +test_expect_success 'checking that first commit is in all tags (hash)' '
>>          hash3=$(git rev-parse HEAD) &&
>>          ...
>>          git tag -l --contains $hash1 v* >actual &&
>>          test_cmp expected actual
>> -"
>> +'
> 
> We need to exercise a bit of caution when making this sort of change
> because a "..." string differs from a '...' string in shell. For
> instance, in a "..." string, interpolation occurs as the string is
> scanned by the shell, whereas a '...' string is taken as a literal.
> Thus, in the above, when it was using a double-quoted string,
> expressions such as `$(git rev-parse HEAD)` and `$hash1` were being
> evaluated and interpolated into the string _before_ the
> test_expect_success() function is even called. On the other hand, with
> a single-quoted string, those expression do not get evaluated when the
> string is scanned, thus remain as-is when passed to
> test_expect_success() as an argument, and they are evaluated only
> later when test_expect_success() invokes `eval` on that argument.
> Depending upon the situation, this difference in evaluation context
> may have a significant impact.
> 
> As a practical example, consider a test with a body like this:
> 
>      echo nothing >nothing &&
>      git add nothing &&
>      git commit -m nothing &&
>      hash=$(git rev-parse HEAD) &&
>      ...
> 
> If this body is inside a double-quoted string, then `$(git rev-parse
> HEAD)` will be evaluated and its value assigned to `hash` _before_
> test_expect_success() is called, thus also before the `git commit`
> command inside the test body (which is almost certainly not what the
> author intended). On the other hand, inside a single-quoted string,
> `$(git rev-parse HEAD)` will be evaluated only once
> test_expect_success() runs the test body, meaning `$(git rev-parse
> HEAD)` will correctly be evaluated _after_ `git commit`. Hence, `hash`
> will have a different value depending upon whether single- or
> double-quotes are used.
> 
> Having said all that, in this case, you seem to have lucked out and
> nothing broke by changing the double-quotes to single-quotes around
> the test bodies.
> 
> However...
> 
>> @@ -1800,16 +1800,16 @@ test_expect_success 'mixing incompatibles modes and options is forbidden' '
>>   for option in --contains --with --no-contains --without --merged --no-merged --points-at
>>   do
>> -       test_expect_success "mixing incompatible modes with $option is forbidden" "
>> +       test_expect_success 'mixing incompatible modes with $option is forbidden' '
>>                  test_must_fail git tag -d $option HEAD &&
>>                  test_must_fail git tag -d $option HEAD some-tag &&
>>                  test_must_fail git tag -v $option HEAD
>> -       "
>> -       test_expect_success "Doing 'git tag --list-like $option <commit> <pattern> is permitted" "
>> +       '
>> +       test_expect_success 'Doing "git tag --list-like $option <commit> <pattern> is permitted' '
>>                  git tag -n $option HEAD HEAD &&
>>                  git tag $option HEAD HEAD &&
>>                  git tag $option
>> -       "
>> +       '
>>   done
> 
> ... changing the double-quotes to single-quotes for the test _titles_
> in these instances is actively wrong. In this case, we _want_
> interpolation of `$option` to happen in the title string so that the
> output looks like this:
> 
>      ok 167 - mixing incompatible modes with --contains is forbidden
>      ok 169 - mixing incompatible modes with --with is forbidden
>      ok 171 - mixing incompatible modes with --no-contains is forbidden
> 
> By changing the title to use single-quotes, you suppress interpolation
> of `$option`, with the result that the displayed titles become rather
> useless:
> 
>      ok 167 - mixing incompatible modes with $option is forbidden
>      ok 169 - mixing incompatible modes with $option is forbidden
>      ok 171 - mixing incompatible modes with $option is forbidden

Thanks for the through explanation! Should I not include this patch in 
v5 as the test body is error-prune or should I revert changing the 
double-quotes to single-quotes for test description and keep the changes 
to the test body and send the patch with v5?