git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
AbdAlRahman Gad <abdobngad@gmail.com>
Date
Aug 6, 2024, 10:10 UTC
Message-ID
<207c4f3c-c444-4405-bed7-34186efa1144@gmail.com>
In-Reply-To
<CAPig+cSzq+6bua=T7w0M-+7+2zbzfD6ihXLkWfondZ8NbqUG4A@mail.gmail.com>
On 8/6/24 06:02, Eric Sunshine wrote:
Show 92 quoted lines
> 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?

Previous: Eric SunshineNext: Eric Sunshine
Message 16 of 21 in “t7004: Modernize the style”
  1. 0/8 t7004: Modernize the styleAbdAlRahman Gad, Aug 5, 2024
  2. 1/8 [Newcomer] t7004: Remove space after redirect operatorsAbdAlRahman Gad, Aug 5, 2024
  3. 2/8 [Newcomer] t7004: Do not lose exit status to pipeAbdAlRahman Gad, Aug 5, 2024
  4. Eric SunshineAug 6, 2024
  5. AbdAlRahman GadAug 6, 2024
  6. Eric SunshineAug 6, 2024
  7. 3/8 [Newcomer] t7004: One command per lineAbdAlRahman Gad, Aug 5, 2024
  8. 4/8 [Newcomer] t7004: Do not prepare things outside test_expect_successAbdAlRahman Gad, Aug 5, 2024
  9. Eric SunshineAug 6, 2024
  10. AbdAlRahman GadAug 6, 2024
  11. 5/8 [Newcomer] t7004: Use indented here-docAbdAlRahman Gad, Aug 5, 2024
  12. 6/8 [Newcomer] t7004: Description on the same line as test_expect_successAbdAlRahman Gad, Aug 5, 2024
  13. 7/8 [Newcomer] t7004: test Description and test body seperated with backslashAbdAlRahman Gad, Aug 5, 2024
  14. 8/8 [Newcomer] t7004: Use single quotes instead of double quotesAbdAlRahman Gad, Aug 5, 2024
  15. Eric SunshineAug 6, 2024
  16. AbdAlRahman GadAug 6, 2024
  17. Eric SunshineAug 6, 2024
  18. Junio C HamanoAug 6, 2024
  19. Eric SunshineAug 6, 2024
  20. Junio C HamanoAug 6, 2024
  21. AbdAlRahman GadAug 6, 2024

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.