| Message ID | 20240804071137.30326-5-abdobngad@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | t7004: modernize the style | expand |
On Sun, Aug 4, 2024 at 3:12 AM AbdAlRahman Gad <abdobngad@gmail.com> wrote: > do not prepare expect outside test_expect_success > > Signed-off-by: AbdAlRahman Gad <abdobngad@gmail.com> > --- > diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh > @@ -131,10 +131,10 @@ test_expect_success 'listing all tags if one exists should succeed' ' > -cat >expect <<EOF > -mytag > -EOF > test_expect_success 'Multiple -l or --list options are equivalent to one -l option' ' > + cat >expect <<-\EOF && > + mytag > + EOF > git tag -l -l >actual && This patch also changes bare `EOF` to `\EOF` which correctly signals to the reader that no variable interpolation or other processing is happening inside the here-doc body. Such a change of semantics is significant enough that it normally warrants mention in the commit message. In fact, often the `EOF` to `\EOF` change would be done in its own commit. However, it's probably not worth a reroll in this case; v3 is likely a good place to stop unless a reviewer finds a problem which demands v4.
On 8/4/24 10:32 AM, Eric Sunshine wrote: > On Sun, Aug 4, 2024 at 3:12 AM AbdAlRahman Gad <abdobngad@gmail.com> wrote: >> do not prepare expect outside test_expect_success >> >> Signed-off-by: AbdAlRahman Gad <abdobngad@gmail.com> >> --- >> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh >> @@ -131,10 +131,10 @@ test_expect_success 'listing all tags if one exists should succeed' ' >> -cat >expect <<EOF >> -mytag >> -EOF >> test_expect_success 'Multiple -l or --list options are equivalent to one -l option' ' >> + cat >expect <<-\EOF && >> + mytag >> + EOF >> git tag -l -l >actual && > > This patch also changes bare `EOF` to `\EOF` which correctly signals > to the reader that no variable interpolation or other processing is > happening inside the here-doc body. Such a change of semantics is > significant enough that it normally warrants mention in the commit > message. In fact, often the `EOF` to `\EOF` change would be done in > its own commit. However, it's probably not worth a reroll in this > case; v3 is likely a good place to stop unless a reviewer finds a > problem which demands v4. Thanks for the feedback. I'll keep that in mind in the upcoming patches.
There are other "prepare for next test" remaining even after applying this patch, though. > diff --git c/t/t7004-tag.sh w/t/t7004-tag.sh > index cfe7653317..96aa908eaf 100755 > --- c/t/t7004-tag.sh > +++ w/t/t7004-tag.sh > ... > -cat >expect <<EOF > -mytag > -EOF > test_expect_success 'Multiple -l or --list options are equivalent to one -l option' ' > + cat >expect <<-\EOF && > + mytag > + EOF > git tag -l -l >actual && > test_cmp expect actual && > git tag --list --list >actual && > test_cmp expect actual && > git tag --list -l --list >actual && > test_cmp expect actual > ' Good. > -cat >expect <<EOF > -myhead > -mytag > -EOF > test_expect_success \ > 'trying to delete tags without params should succeed and do nothing' ' > + cat >expect <<-\EOF && > + myhead > + mytag > + EOF > git tag -l >actual && > test_cmp expect actual && > git tag -d && > git tag -l >actual && > test_cmp expect actual > ' Good too. > # creating annotated tags: > > get_tag_msg () { > git cat-file tag "$1" | sed -e "/BEGIN PGP/q" > } > > # run test_tick before committing always gives the time in that timezone > get_tag_header () { > cat <<EOF > object $2 > type $3 > tag $1 > tagger C O Mitter <committer@example.com> $4 -0700 > > EOF > } > > commit=$(git rev-parse HEAD) > time=$test_tick > > get_tag_header annotated-tag $commit commit $time >expect > echo "A message" >>expect > test_expect_success \ > 'creating an annotated tag with -m message should succeed' ' > git tag -m "A message" annotated-tag && > get_tag_msg annotated-tag >actual && > test_cmp expect actual > ' The creation of $commit, holding the timestamp in $time, calling the get_tag_header helper helper function, and addition of the message to 'expect' file. Moving all of them inside expect_success block is perfectly in line with the theme of this step. > get_tag_header annotated-tag-edit $commit commit $time >expect > echo "An edited message" >>expect > test_expect_success 'set up editor' ' > write_script fakeeditor <<-\EOF > sed -e "s/A message/An edited message/g" <"$1" >"$1-" > mv "$1-" "$1" > EOF > ' > test_expect_success \ > ... Ditto here. In addition, there is a blank line lacking after the above test piece and before the next one. > cat >msgfile <<EOF > Another message > in a file. > EOF > get_tag_header file-annotated-tag $commit commit $time >expect > cat msgfile >>expect > test_expect_success \ > 'creating an annotated tag with -F messagefile should succeed' ' > git tag -F msgfile file-annotated-tag && > get_tag_msg file-annotated-tag >actual && > test_cmp expect actual > ' Ditto. > get_tag_header file-annotated-tag-edit $commit commit $time >expect > sed -e "s/Another message/Another edited message/g" msgfile >>expect > test_expect_success 'set up editor' ' > write_script fakeeditor <<-\EOF > sed -e "s/Another message/Another edited message/g" <"$1" >"$1-" > mv "$1-" "$1" > EOF > ' > test_expect_success \ > 'creating an annotated tag with -F messagefile --edit should succeed' ' > GIT_EDITOR=./fakeeditor git tag -F msgfile --edit file-annotated-tag-edit && > get_tag_msg file-annotated-tag-edit >actual && > test_cmp expect actual > ' > > cat >inputmsg <<EOF > A message from the > standard input > EOF > get_tag_header stdin-annotated-tag $commit commit $time >expect > cat inputmsg >>expect > test_expect_success 'creating an annotated tag with -F - should succeed' ' > git tag -F - stdin-annotated-tag <inputmsg && > get_tag_msg stdin-annotated-tag >actual && > test_cmp expect actual > ' Ditto. > # blank and empty messages: > > get_tag_header empty-annotated-tag $commit commit $time >expect > test_expect_success \ > 'creating a tag with an empty -m message should succeed' ' > git tag -m "" empty-annotated-tag && > get_tag_msg empty-annotated-tag >actual && > test_cmp expect actual > ' > > >emptyfile > get_tag_header emptyfile-annotated-tag $commit commit $time >expect > test_expect_success \ > 'creating a tag with an empty -F messagefile should succeed' ' > git tag -F emptyfile emptyfile-annotated-tag && > get_tag_msg emptyfile-annotated-tag >actual && > test_cmp expect actual > ' > > printf '\n\n \n\t\nLeading blank lines\n' >blanksfile > printf '\n\t \t \nRepeated blank lines\n' >>blanksfile > printf '\n\n\nTrailing spaces \t \n' >>blanksfile > printf '\nTrailing blank lines\n\n\t \n\n' >>blanksfile > get_tag_header blanks-annotated-tag $commit commit $time >expect > cat >>expect <<EOF > Leading blank lines > > Repeated blank lines > > Trailing spaces > > Trailing blank lines > EOF > test_expect_success \ > 'extra blanks in the message for an annotated tag should be removed' ' > git tag -F blanksfile blanks-annotated-tag && > get_tag_msg blanks-annotated-tag >actual && > test_cmp expect actual > ' Ditto. > get_tag_header blank-annotated-tag $commit commit $time >expect > test_expect_success \ > 'creating a tag with blank -m message with spaces should succeed' ' > git tag -m " " blank-annotated-tag && > get_tag_msg blank-annotated-tag >actual && > test_cmp expect actual > ' > > echo ' ' >blankfile > echo '' >>blankfile > echo ' ' >>blankfile > get_tag_header blankfile-annotated-tag $commit commit $time >expect > test_expect_success \ > 'creating a tag with blank -F messagefile with spaces should succeed' ' > git tag -F blankfile blankfile-annotated-tag && > get_tag_msg blankfile-annotated-tag >actual && > test_cmp expect actual > ' > > printf ' ' >blanknonlfile > get_tag_header blanknonlfile-annotated-tag $commit commit $time >expect > test_expect_success \ > 'creating a tag with -F file of spaces and no newline should succeed' ' > git tag -F blanknonlfile blanknonlfile-annotated-tag && > get_tag_msg blanknonlfile-annotated-tag >actual && > test_cmp expect actual > ' > > # messages with commented lines: > > cat >commentsfile <<EOF > # A comment > > ############ > The message. > ############ > One line. > > > # commented lines > # commented lines > > Another line. > # comments > > Last line. > EOF > get_tag_header comments-annotated-tag $commit commit $time >expect > cat >>expect <<EOF > The message. > One line. > > Another line. > > Last line. > EOF > test_expect_success \ > 'creating a tag using a -F messagefile with #comments should succeed' ' > git tag -F commentsfile comments-annotated-tag && > get_tag_msg comments-annotated-tag >actual && > test_cmp expect actual > ' Ditto. > get_tag_header comment-annotated-tag $commit commit $time >expect > test_expect_success \ > 'creating a tag with a #comment in the -m message should succeed' ' > git tag -m "#comment" comment-annotated-tag && > get_tag_msg comment-annotated-tag >actual && > test_cmp expect actual > ' > > echo '#comment' >commentfile > echo '' >>commentfile > echo '####' >>commentfile > get_tag_header commentfile-annotated-tag $commit commit $time >expect > test_expect_success \ > 'creating a tag with #comments in the -F messagefile should succeed' ' > git tag -F commentfile commentfile-annotated-tag && > get_tag_msg commentfile-annotated-tag >actual && > test_cmp expect actual > ' > > printf '#comment' >commentnonlfile > get_tag_header commentnonlfile-annotated-tag $commit commit $time >expect > test_expect_success \ > 'creating a tag with a file of #comment and no newline should succeed' ' > git tag -F commentnonlfile commentnonlfile-annotated-tag && > get_tag_msg commentnonlfile-annotated-tag >actual && > test_cmp expect actual > ' Ditto. > echo 'tag line one' >annotagmsg > echo 'tag line two' >>annotagmsg > echo 'tag line three' >>annotagmsg > test_expect_success \ > 'listing many message lines of a non-signed tag should succeed' ' > git tag -F annotagmsg tag-lines && > ... Ditto. > # creating and verifying signed tags: > > get_tag_header signed-tag $commit commit $time >expect > echo 'A signed tag message' >>expect > echo '-----BEGIN PGP SIGNATURE-----' >>expect > test_expect_success GPG 'creating a signed tag with -m message should succeed' ' > git tag -s -m "A signed tag message" signed-tag && > get_tag_msg signed-tag >actual && > test_cmp expect actual > ' Ditto. > get_tag_header u-signed-tag $commit commit $time >expect > echo 'Another message' >>expect > echo '-----BEGIN PGP SIGNATURE-----' >>expect > test_expect_success GPG 'sign with a given key id' ' > > git tag -u committer@example.com -m "Another message" u-signed-tag && > get_tag_msg u-signed-tag >actual && > test_cmp expect actual > > ' Ditto. If it is too much to include all of the above in the same patch I think it would be OK to stop at "move prepatation of the expect file using cat and here-doc into test_expect_success block", as long as we make it clear in the log message that we are drawing the line there in the patch, and leaving other similar issues untouched to keep the step manageably short. Adding a new patch to clean up the "other similar issues" is entirely optional, but doing so would make the series even more thorough. Thanks.
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh index cfe7653317..96aa908eaf 100755 --- a/t/t7004-tag.sh +++ b/t/t7004-tag.sh @@ -131,10 +131,10 @@ test_expect_success 'listing all tags if one exists should succeed' ' git tag ' -cat >expect <<EOF -mytag -EOF test_expect_success 'Multiple -l or --list options are equivalent to one -l option' ' + cat >expect <<-\EOF && + mytag + EOF git tag -l -l >actual && test_cmp expect actual && git tag --list --list >actual && @@ -209,12 +209,12 @@ test_expect_success 'trying to delete an unknown tag should fail' ' test_must_fail git tag -d unknown-tag ' -cat >expect <<EOF -myhead -mytag -EOF test_expect_success \ 'trying to delete tags without params should succeed and do nothing' ' + cat >expect <<-\EOF && + myhead + mytag + EOF git tag -l >actual && test_cmp expect actual && git tag -d && @@ -252,18 +252,18 @@ test_expect_success 'trying to delete an already deleted tag should fail' \ # listing various tags with pattern matching: -cat >expect <<EOF -a1 -aa1 -cba -t210 -t211 -v0.2.1 -v1.0 -v1.0.1 -v1.1.3 -EOF test_expect_success 'listing all tags should print them ordered' ' + cat >expect <<-\EOF && + a1 + aa1 + cba + t210 + t211 + v0.2.1 + v1.0 + v1.0.1 + v1.1.3 + EOF git tag v1.0.1 && git tag t211 && git tag aa1 && @@ -279,62 +279,62 @@ test_expect_success 'listing all tags should print them ordered' ' test_cmp expect actual ' -cat >expect <<EOF -a1 -aa1 -cba -EOF test_expect_success \ 'listing tags with substring as pattern must print those matching' ' + cat >expect <<-\EOF && + a1 + aa1 + cba + EOF rm *a* && git tag -l "*a*" >current && test_cmp expect current ' -cat >expect <<EOF -v0.2.1 -v1.0.1 -EOF test_expect_success \ 'listing tags with a suffix as pattern must print those matching' ' + cat >expect <<-\EOF && + v0.2.1 + v1.0.1 + EOF git tag -l "*.1" >actual && test_cmp expect actual ' -cat >expect <<EOF -t210 -t211 -EOF test_expect_success \ 'listing tags with a prefix as pattern must print those matching' ' + cat >expect <<-\EOF && + t210 + t211 + EOF git tag -l "t21*" >actual && test_cmp expect actual ' -cat >expect <<EOF -a1 -EOF test_expect_success \ 'listing tags using a name as pattern must print that one matching' ' + cat >expect <<-\EOF && + a1 + EOF git tag -l a1 >actual && test_cmp expect actual ' -cat >expect <<EOF -v1.0 -EOF test_expect_success \ 'listing tags using a name as pattern must print that one matching' ' + cat >expect <<-\EOF && + v1.0 + EOF git tag -l v1.0 >actual && test_cmp expect actual ' -cat >expect <<EOF -v1.0.1 -v1.1.3 -EOF test_expect_success \ 'listing tags with ? in the pattern should print those matching' ' + cat >expect <<-\EOF && + v1.0.1 + v1.1.3 + EOF git tag -l "v1.?.?" >actual && test_cmp expect actual ' @@ -345,19 +345,25 @@ test_expect_success \ test_must_be_empty actual ' -cat >expect <<EOF -v0.2.1 -v1.0 -v1.0.1 -v1.1.3 -EOF test_expect_success \ 'listing tags using v* should print only those having v' ' + cat >expect <<-\EOF && + v0.2.1 + v1.0 + v1.0.1 + v1.1.3 + EOF git tag -l "v*" >actual && test_cmp expect actual ' test_expect_success 'tag -l can accept multiple patterns' ' + cat >expect <<-\EOF && + v0.2.1 + v1.0 + v1.0.1 + v1.1.3 + EOF git tag -l "v1*" "v0*" >actual && test_cmp expect actual ' @@ -371,6 +377,12 @@ test_expect_success 'tag -l can accept multiple patterns' ' # out if we're going to break this long-documented form of taking # multiple patterns. test_expect_success 'tag -l <pattern> -l <pattern> works, as our buggy documentation previously suggested' ' + cat >expect <<-\EOF && + v0.2.1 + v1.0 + v1.0.1 + v1.1.3 + EOF git tag -l "v1*" -l "v0*" >actual && test_cmp expect actual ' @@ -1683,23 +1695,21 @@ test_expect_success 'checking that first commit is in all tags (relative)' " test_must_be_empty actual " -cat >expected <<EOF -v2.0 -EOF - 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 " -cat >expected <<EOF -v0.2.1 -v1.0 -v1.0.1 -v1.1.3 -EOF - test_expect_success 'inverse of the last test, with --no-contains' " + cat >expected <<-\EOF && + v0.2.1 + v1.0 + v1.0.1 + v1.1.3 + EOF git tag -l --no-contains $hash2 v* >actual && test_cmp expected actual " @@ -1709,15 +1719,14 @@ test_expect_success 'checking that third commit has no tags' " test_must_be_empty actual " -cat >expected <<EOF -v0.2.1 -v1.0 -v1.0.1 -v1.1.3 -v2.0 -EOF - test_expect_success 'conversely --no-contains on the third commit lists all tags' " + cat >expected <<-\EOF && + v0.2.1 + v1.0 + v1.0.1 + v1.1.3 + v2.0 + EOF git tag -l --no-contains $hash3 v* >actual && test_cmp expected actual " @@ -1734,24 +1743,22 @@ test_expect_success 'creating simple branch' ' hash4=$(git rev-parse HEAD) -cat >expected <<EOF -v3.0 -EOF - test_expect_success 'checking that branch head only has one tag' " + cat >expected <<-\EOF && + v3.0 + EOF git tag -l --contains $hash4 v* >actual && test_cmp expected actual " -cat >expected <<EOF -v0.2.1 -v1.0 -v1.0.1 -v1.1.3 -v2.0 -EOF - test_expect_success 'checking that branch head with --no-contains lists all but one tag' " + cat >expected <<-\EOF && + v0.2.1 + v1.0 + v1.0.1 + v1.1.3 + v2.0 + EOF git tag -l --no-contains $hash4 v* >actual && test_cmp expected actual " @@ -1761,50 +1768,65 @@ test_expect_success 'merging original branch into this branch' ' git tag v4.0 ' -cat >expected <<EOF -v4.0 -EOF - 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 " -cat >expected <<EOF -v0.2.1 -v1.0 -v1.0.1 -v1.1.3 -v2.0 -v3.0 -EOF - 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 + v1.0.1 + v1.1.3 + v2.0 + v3.0 + EOF git tag -l --no-contains $hash3 v* >actual && test_cmp expected actual " -cat >expected <<EOF -v0.2.1 -v1.0 -v1.0.1 -v1.1.3 -v2.0 -v3.0 -v4.0 -EOF - test_expect_success 'checking that initial commit is in all tags' " + cat >expected <<-\EOF && + v0.2.1 + v1.0 + v1.0.1 + v1.1.3 + v2.0 + v3.0 + v4.0 + 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 && + v0.2.1 + v1.0 + v1.0.1 + v1.1.3 + v2.0 + v3.0 + v4.0 + EOF git tag --contains $hash1 v* >actual && test_cmp expected actual ' test_expect_success 'checking that initial commit is in all tags with --no-contains' " + cat >expected <<-\EOF && + v0.2.1 + v1.0 + v1.0.1 + v1.1.3 + v2.0 + v3.0 + v4.0 + EOF git tag -l --no-contains $hash1 v* >actual && test_must_be_empty actual "
do not prepare expect outside test_expect_success Signed-off-by: AbdAlRahman Gad <abdobngad@gmail.com> --- t/t7004-tag.sh | 224 +++++++++++++++++++++++++++---------------------- 1 file changed, 123 insertions(+), 101 deletions(-)