From: AbdAlRahman Gad Date: Tue, 06 Aug 2024 10:10:32 GMT Subject: Re: [PATCH v4 8/8] [Newcomer] t7004: Use single quotes instead of double quotes Message-ID: <207c4f3c-c444-4405-bed7-34186efa1144@gmail.com> In-Reply-To: On 8/6/24 06:02, Eric Sunshine wrote: > On Mon, Aug 5, 2024 at 8:00 PM AbdAlRahman Gad 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 >> --- >> 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 is permitted" " >> + ' >> + test_expect_success 'Doing "git tag --list-like $option 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?