On 8/6/24 06:02, Eric Sunshine wrote:
On Mon, Aug 5, 2024 at 8:00 PM AbdAlRahman Gad <abdobngad@xxxxxxxxx> 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@xxxxxxxxx> --- 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?
