| Message ID | 20240805235917.190699-9-abdobngad@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | t7004: Modernize the style | expand |
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
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?
Eric Sunshine <sunshine@sunshineco.com> writes: > 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, I know it is just your finger slipping, but the variable "hash" is not assigned to before test_expect_success is called even with the body inside dq. What happens is that the value of HEAD is expanded in the string that will be evaled by test_expect_success so the 4th line in the above becomes "hash=3469a23659d8197190d2765cf9f31dec5ab602fa &&"; as the resulting string is then eval'ed by test_expect_success, the end result is as you descirbed, i.e., ... > thus also before the `git commit` > command inside the test body (which is almost certainly not what the > author intended). ... $hash does not get the name of the commit object resulting from the "git commit" command before it. >> - " >> - 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' ' > > ... 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 Yes, these has to be done carefully, both for titles and bodies. Thanks.
On Tue, Aug 6, 2024 at 12:35 PM Junio C Hamano <gitster@pobox.com> wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > > 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, > > I know it is just your finger slipping, but the variable "hash" is > not assigned to before test_expect_success is called even with the > body inside dq. Ugh, you're quite correct, of course. The "and its value assigned to `hash`" part was one of those last-moment-edits which come back to haunt the author. That's not so much a finger slip as an outright bungle. Thanks for catching and correcting this. > What happens is that the value of HEAD is expanded in the string > that will be evaled by test_expect_success so the 4th line in the > above becomes "hash=3469a23659d8197190d2765cf9f31dec5ab602fa &&"; > as the resulting string is then eval'ed by test_expect_success, > the end result is as you descirbed, i.e., ... > ... $hash does not get the name of the commit object resulting from > the "git commit" command before it. Including an actual example of the resulting string, as you did here, does a much better job of illustrating the issue than my bungled last-moment-edit.
On Tue, Aug 6, 2024 at 6:10 AM AbdAlRahman Gad <abdobngad@gmail.com> wrote: > On 8/6/24 06:02, Eric Sunshine wrote: > > We need to exercise a bit of caution when making this sort of change > > because a "..." string differs from a '...' string in shell. [...] > > [...] > > 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... > > ... 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 > > > > 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 > > 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? Overall, the changes made by this patch are valuable since it brings the script more in line with modern practice and because single-quotes around the test body make it easier for readers to reason about test behavior. Coupled with the fact that none of the tests broke by changing the bodies from double- to single-quote, it's a good idea to keep this patch in v5 but omit the parts which break the test descriptions.
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh index a46f8141d4..06f07b5524 100755 --- a/t/t7004-tag.sh +++ b/t/t7004-tag.sh @@ -1585,7 +1585,7 @@ test_expect_success 'creating third commit without tag' ' # simple linear checks of --continue -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) && cat >expected <<-\EOF && v0.2.1 @@ -1596,10 +1596,10 @@ test_expect_success 'checking that first commit is in all tags (hash)' " EOF git tag -l --contains $hash1 v* >actual && test_cmp expected actual -" +' # other ways of specifying the commit -test_expect_success 'checking that first commit is in all tags (tag)' " +test_expect_success 'checking that first commit is in all tags (tag)' ' cat >expected <<-\EOF && v0.2.1 v1.0 @@ -1609,9 +1609,9 @@ test_expect_success 'checking that first commit is in all tags (tag)' " EOF git tag -l --contains v1.0 v* >actual && test_cmp expected actual -" +' -test_expect_success 'checking that first commit is in all tags (relative)' " +test_expect_success 'checking that first commit is in all tags (relative)' ' cat >expected <<-\EOF && v0.2.1 v1.0 @@ -1621,33 +1621,33 @@ test_expect_success 'checking that first commit is in all tags (relative)' " EOF git tag -l --contains HEAD~2 v* >actual && test_cmp expected actual -" +' # All the --contains tests above, but with --no-contains -test_expect_success 'checking that first commit is not listed in any tag with --no-contains (hash)' " +test_expect_success 'checking that first commit is not listed in any tag with --no-contains (hash)' ' git tag -l --no-contains $hash1 v* >actual && test_must_be_empty actual -" +' -test_expect_success 'checking that first commit is in all tags (tag)' " +test_expect_success 'checking that first commit is in all tags (tag)' ' git tag -l --no-contains v1.0 v* >actual && test_must_be_empty actual -" +' -test_expect_success 'checking that first commit is in all tags (relative)' " +test_expect_success 'checking that first commit is in all tags (relative)' ' git tag -l --no-contains HEAD~2 v* >actual && test_must_be_empty actual -" +' -test_expect_success 'checking that second commit only has one tag' " +test_expect_success 'checking that second commit only has one tag' ' cat >expected <<-\EOF && v2.0 EOF git tag -l --contains $hash2 v* >actual && test_cmp expected actual -" +' -test_expect_success 'inverse of the last test, with --no-contains' " +test_expect_success 'inverse of the last test, with --no-contains' ' cat >expected <<-\EOF && v0.2.1 v1.0 @@ -1656,14 +1656,14 @@ test_expect_success 'inverse of the last test, with --no-contains' " EOF git tag -l --no-contains $hash2 v* >actual && test_cmp expected actual -" +' -test_expect_success 'checking that third commit has no tags' " +test_expect_success 'checking that third commit has no tags' ' git tag -l --contains $hash3 v* >actual && test_must_be_empty actual -" +' -test_expect_success 'conversely --no-contains on the third commit lists all tags' " +test_expect_success 'conversely --no-contains on the third commit lists all tags' ' cat >expected <<-\EOF && v0.2.1 v1.0 @@ -1673,7 +1673,7 @@ test_expect_success 'conversely --no-contains on the third commit lists all tags EOF git tag -l --no-contains $hash3 v* >actual && test_cmp expected actual -" +' # how about a simple merge? @@ -1694,7 +1694,7 @@ test_expect_success 'checking that branch head only has one tag' ' test_cmp expected actual ' -test_expect_success 'checking that branch head with --no-contains lists all but one tag' " +test_expect_success 'checking that branch head with --no-contains lists all but one tag' ' cat >expected <<-\EOF && v0.2.1 v1.0 @@ -1704,22 +1704,22 @@ test_expect_success 'checking that branch head with --no-contains lists all but EOF git tag -l --no-contains $hash4 v* >actual && test_cmp expected actual -" +' test_expect_success 'merging original branch into this branch' ' git merge --strategy=ours main && git tag v4.0 ' -test_expect_success 'checking that original branch head has one tag now' " +test_expect_success 'checking that original branch head has one tag now' ' cat >expected <<-\EOF && v4.0 EOF git tag -l --contains $hash3 v* >actual && test_cmp expected actual -" +' -test_expect_success 'checking that original branch head with --no-contains lists all but one tag now' " +test_expect_success 'checking that original branch head with --no-contains lists all but one tag now' ' cat >expected <<-\EOF && v0.2.1 v1.0 @@ -1730,9 +1730,9 @@ test_expect_success 'checking that original branch head with --no-contains lists EOF git tag -l --no-contains $hash3 v* >actual && test_cmp expected actual -" +' -test_expect_success 'checking that initial commit is in all tags' " +test_expect_success 'checking that initial commit is in all tags' ' cat >expected <<-\EOF && v0.2.1 v1.0 @@ -1744,7 +1744,7 @@ test_expect_success 'checking that initial commit is in all tags' " EOF git tag -l --contains $hash1 v* >actual && test_cmp expected actual -" +' test_expect_success 'checking that --contains can be used in non-list mode' ' cat >expected <<-\EOF && @@ -1760,10 +1760,10 @@ test_expect_success 'checking that --contains can be used in non-list mode' ' test_cmp expected actual ' -test_expect_success 'checking that initial commit is in all tags with --no-contains' " +test_expect_success 'checking that initial commit is in all tags with --no-contains' ' git tag -l --no-contains $hash1 v* >actual && test_must_be_empty actual -" +' # mixing modes and options: @@ -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 # check points-at @@ -2185,7 +2185,7 @@ test_expect_success 'git tag -l with --format="%(rest)" must fail' ' test_must_fail git tag -l --format="%(rest)" "v1*" ' -test_expect_success "set up color tests" ' +test_expect_success 'set up color tests' ' echo "<RED>v1.0<RESET>" >expect.color && echo "v1.0" >expect.bare && color_args="--format=%(color:red)%(refname:short) --list v1.0"
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> --- t/t7004-tag.sh | 70 +++++++++++++++++++++++++------------------------- 1 file changed, 35 insertions(+), 35 deletions(-)