{"thread":{"id":"45468","subject":"EOF test fixes (t5615/t7004)","startedAt":"2017-03-22T17:35:37Z","lastAt":"2017-03-24T18:16:34Z","messageCount":46,"participants":["Jan Palus","Stefan Beller","Junio C Hamano","Jeff King","Santiago Torres","Jonathan Nieder"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"314930","messageId":"20170322173528.ho43ulndlozq35tu@kalarepa","threadId":"45468","inReplyTo":null,"subject":"EOF test fixes (t5615/t7004)","fromName":"Jan Palus","fromEmail":"jan.palus@gmail.com","sentAt":"2017-03-22T17:35:28Z","receivedAt":"2017-03-22T17:35:37Z","isPatch":false,"sender":{"key":"jan.palus@gmail.com","avatar":null},"body":"Hi,\n\nattached patch fixes 2 out of 3 tests failing in my env -- missing EOF,\nincorrectly placed \"&&\" after EOF. One test seems to be incorrect as it\nfails even after fixing. I suspect the main difference between my env and\nothers is in shell used -- mksh in my case and probably bash in others.\nLooks like bash has a nasty behavior when it encounters syntax error:\n\n$ cat test.sh\ncat >/dev/null <<-\\EOF\ntagname : forged-tag\nEOF &&\necho foo\n\n$ bash test.sh && echo success || echo failed\ntest.sh: line 4: warning: here-document at line 1 delimited by\nend-of-file (wanted `EOF')\nsuccess\n\n# notice no \"foo\" printed\n\n$ mksh test.sh && echo success || echo failed \ntest.sh[5]: here document 'EOF' unclosed\nfailed\n\nTest that fails even after fixing EOFs:\nt7004-tag.sh:verifying a forged tag with --format fail and format accordingly\n\nNote that I'm not subscribed to mailing list so in case of any questions\nplease mail me directly.\n\n\nRegards\nJan\n\n\ndiff -urN git-2.11.1.orig/t/t5615-alternate-env.sh git-2.11.1/t/t5615-alternate-env.sh\n--- git-2.11.1.orig/t/t5615-alternate-env.sh\t2017-02-03 02:24:45.115143042 +0100\n+++ git-2.11.1/t/t5615-alternate-env.sh\t2017-02-03 02:24:58.081809318 +0100\n@@ -77,6 +77,7 @@\n \tcheck_obj \"$quoted:$unquoted\" <<-EOF\n \t$one blob\n \t$two blob\n+\tEOF\n '\n \n test_expect_success !MINGW 'broken quoting falls back to interpreting raw' '\ndiff -ur git-2.12.0.orig/t/t7004-tag.sh git-2.12.0/t/t7004-tag.sh\n--- git-2.12.0.orig/t/t7004-tag.sh\t2017-02-25 14:10:53.059367179 +0100\n+++ git-2.12.0/t/t7004-tag.sh\t2017-02-25 14:11:45.105827700 +0100\n@@ -880,17 +880,17 @@\n '\n \n test_expect_success 'verifying a proper tag with --format pass and format accordingly' '\n-\tcat >expect <<-\\EOF\n+\tcat >expect <<-\\EOF &&\n \ttagname : signed-tag\n-\tEOF &&\n+\tEOF\n \tgit tag -v --format=\"tagname : %(tag)\" \"signed-tag\" >actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success 'verifying a forged tag with --format fail and format accordingly' '\n-\tcat >expect <<-\\EOF\n+\tcat >expect <<-\\EOF &&\n \ttagname : forged-tag\n-\tEOF &&\n+\tEOF\n \ttest_must_fail git tag -v --format=\"tagname : %(tag)\" \"forged-tag\" >actual &&\n \ttest_cmp expect actual\n '\n"},{"id":"314939","messageId":"20170322182841.wkavsj6hj67jzud7@kalarepa","threadId":"45468","inReplyTo":"20170322173528.ho43ulndlozq35tu@kalarepa","subject":"Re: EOF test fixes (t7030/t7406)","fromName":"Jan Palus","fromEmail":"jan.palus@gmail.com","sentAt":"2017-03-22T18:28:41Z","receivedAt":"2017-03-22T18:28:52Z","isPatch":false,"sender":{"key":"jan.palus@gmail.com","avatar":null},"body":"It appears more tests are affected:\n\n$ grep -r '^[[:space:]]*EOF &&' . \n./t7406-submodule-update.sh:\tEOF &&\n./t7030-verify-tag.sh:\tEOF &&\n./t7030-verify-tag.sh:\tEOF &&\n./t7004-tag.sh:\tEOF &&\n./t7004-tag.sh:\tEOF &&\n\nattaching patch for t7406 and t7030 which both fail even after fix is\napplied.\n\n\ndiff -ur git-2.12.1.orig/t/t7030-verify-tag.sh git-2.12.1/t/t7030-verify-tag.sh\n--- git-2.12.1.orig/t/t7030-verify-tag.sh\t2017-03-22 19:20:49.614759549 +0100\n+++ git-2.12.1/t/t7030-verify-tag.sh\t2017-03-22 19:26:27.608511234 +0100\n@@ -126,17 +126,17 @@\n '\n \n test_expect_success 'verifying tag with --format' '\n-\tcat >expect <<-\\EOF\n+\tcat >expect <<-\\EOF &&\n \ttagname : fourth-signed\n-\tEOF &&\n+\tEOF\n \tgit verify-tag --format=\"tagname : %(tag)\" \"fourth-signed\" >actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success 'verifying a forged tag with --format fail and format accordingly' '\n-\tcat >expect <<-\\EOF\n+\tcat >expect <<-\\EOF &&\n \ttagname : 7th forged-signed\n-\tEOF &&\n+\tEOF\n \ttest_must_fail git verify-tag --format=\"tagname : %(tag)\" $(cat forged1.tag) >actual-forged &&\n \ttest_cmp expect actual-forged\n '\ndiff -ur git-2.12.1.orig/t/t7406-submodule-update.sh git-2.12.1/t/t7406-submodule-update.sh\n--- git-2.12.1.orig/t/t7406-submodule-update.sh\t2017-03-22 19:20:49.614759549 +0100\n+++ git-2.12.1/t/t7406-submodule-update.sh\t2017-03-22 19:25:34.105528379 +0100\n@@ -442,9 +442,9 @@\n '\n \n test_expect_success 'submodule update - command run for initial population of submodule' '\n-\tcat <<-\\ EOF >expect\n+\tcat >expect <<-\\EOF &&\n \tExecution of '\\''false $submodulesha1'\\'' failed in submodule path '\\''submodule'\\''\n-\tEOF &&\n+\tEOF\n \trm -rf super/submodule &&\n \ttest_must_fail git -C super submodule update >../actual &&\n \ttest_cmp expect actual &&\n"},{"id":"314943","messageId":"CAGZ79kYe73_iAPU7J9Z+7q_J3F6sUmbHpiC-u2G89auns3bP3Q@mail.gmail.com","threadId":"45468","inReplyTo":"20170322173528.ho43ulndlozq35tu@kalarepa","subject":"Re: EOF test fixes (t5615/t7004)","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-22T18:47:03Z","receivedAt":"2017-03-22T18:47:16Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Mar 22, 2017 at 10:35 AM, Jan Palus wrote:\n> Hi,\n>\n> attached patch fixes 2 out of 3 tests failing in my env -- missing EOF,\n> incorrectly placed \"&&\" after EOF. One test seems to be incorrect as it\n> fails even after fixing. I suspect the main difference between my env and\n> others is in shell used -- mksh in my case and probably bash in others.\n> Looks like bash has a nasty behavior when it encounters syntax error:\n>\n> $ cat test.sh\n> cat >/dev/null <<-\\EOF\n> tagname : forged-tag\n> EOF &&\n> echo foo\n>\n> $ bash test.sh && echo success || echo failed\n> test.sh: line 4: warning: here-document at line 1 delimited by\n> end-of-file (wanted `EOF')\n> success\n>\n> # notice no \"foo\" printed\n>\n> $ mksh test.sh && echo success || echo failed\n> test.sh[5]: here document 'EOF' unclosed\n> failed\n>\n> Test that fails even after fixing EOFs:\n> t7004-tag.sh:verifying a forged tag with --format fail and format accordingly\n>\n> Note that I'm not subscribed to mailing list so in case of any questions\n> please mail me directly.\n\nThanks for catching these bugs!\n\nPlease have a look at Documentation/SubmittingPatches.\n(the most important thing is the \"Sign-off-by\" line indicating you\nare legally permitted to send such a patch;\nfor one-off patches the format can be negotiated, but it is easier\nfor maintainers to take the proper format.)\n\nThis email conveys the actual problem very well, so only a little\nchange is needed to make it a proper commit message.\n(c.f. git clone git://git.kernel.org/pub/scm/git/git.git/ &&\ngit -C git log)\n\nThanks,\nStefan\n"},{"id":"314952","messageId":"xmqqo9wtjdvl.fsf@gitster.mtv.corp.google.com","threadId":"45468","inReplyTo":"20170322173528.ho43ulndlozq35tu@kalarepa","subject":"Re: EOF test fixes (t5615/t7004)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-22T19:43:58Z","receivedAt":"2017-03-22T19:44:32Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jan Palus writes:\n\n> diff -ur git-2.12.0.orig/t/t7004-tag.sh git-2.12.0/t/t7004-tag.sh\n> --- git-2.12.0.orig/t/t7004-tag.sh\t2017-02-25 14:10:53.059367179 +0100\n> +++ git-2.12.0/t/t7004-tag.sh\t2017-02-25 14:11:45.105827700 +0100\n> @@ -880,17 +880,17 @@\n> '\n> \n> test_expect_success 'verifying a proper tag with --format pass and format accordingly' '\n> -\tcat >expect <<-\\EOF\n> +\tcat >expect <<-\\EOF &&\n> \ttagname : signed-tag\n> -\tEOF &&\n> +\tEOF\n> \tgit tag -v --format=\"tagname : %(tag)\" \"signed-tag\" >actual &&\n> \ttest_cmp expect actual\n> '\n> \n> test_expect_success 'verifying a forged tag with --format fail and format accordingly' '\n> -\tcat >expect <<-\\EOF\n> +\tcat >expect <<-\\EOF &&\n> \ttagname : forged-tag\n> -\tEOF &&\n> +\tEOF\n> \ttest_must_fail git tag -v --format=\"tagname : %(tag)\" \"forged-tag\" >actual &&\n> \ttest_cmp expect actual\n> '\n\nOuch. These shouldn't even have passed our review.\n\nThanks for spotting.\n"},{"id":"314957","messageId":"20170322200805.23837-1-gitster@pobox.com","threadId":"45468","inReplyTo":"20170322173528.ho43ulndlozq35tu@kalarepa","subject":"[PATCH 0/3] fix \"here-doc\" syntax errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-22T20:08:02Z","receivedAt":"2017-03-22T20:08:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Because I'd prefer to be able to queue fixes on individual topics\nthat introduced the breakages, I splitted the fixes in your two\nmessages into three patches.\n\nPlease respond to the list, saying it is OK to add your \"sign-off\"\n(see Documentation/SubmittingPatches) to these patches.\n\nThanks.\n\nJan Palus (3):\n t5615: fix a here-doc syntax error\n t7406: fix here-doc syntax errors\n t7004, t7030: fix here-doc syntax errors\n\n t/t5615-alternate-env.sh | 1 +\n t/t7004-tag.sh | 8 ++++----\n t/t7030-verify-tag.sh | 8 ++++----\n t/t7406-submodule-update.sh | 4 ++--\n 4 files changed, 11 insertions(+), 10 deletions(-)\n\n-- \n2.12.1-430-gafd6726309\n\n"},{"id":"314958","messageId":"20170322200805.23837-2-gitster@pobox.com","threadId":"45468","inReplyTo":"20170322200805.23837-1-gitster@pobox.com","subject":"[PATCH 1/3] t5615: fix a here-doc syntax error","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-22T20:08:03Z","receivedAt":"2017-03-22T20:08:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Jan Palus \n\nThis came as part of jk/quote-env-path-list-component and was merged\nto 2.11.1 and later.\n\nSigned-off-by: Junio C Hamano \n---\n\n * This should be applied on top of 5e74824f (\"t5615-alternate-env:\n double-quotes in file names do not work on Windows\", 2016-12-21)\n\n t/t5615-alternate-env.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/t5615-alternate-env.sh b/t/t5615-alternate-env.sh\nindex 26ebb0375d..d2d883f3a1 100755\n--- a/t/t5615-alternate-env.sh\n+++ b/t/t5615-alternate-env.sh\n@@ -77,6 +77,7 @@ test_expect_success 'mix of quoted and unquoted alternates' '\n \tcheck_obj \"$quoted:$unquoted\" <<-EOF\n \t$one blob\n \t$two blob\n+\tEOF\n '\n \n test_expect_success !MINGW 'broken quoting falls back to interpreting raw' '\n-- \n2.12.1-430-gafd6726309\n\n"},{"id":"314959","messageId":"20170322200805.23837-3-gitster@pobox.com","threadId":"45468","inReplyTo":"20170322200805.23837-1-gitster@pobox.com","subject":"[PATCH 2/3] t7406: fix here-doc syntax errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-22T20:08:04Z","receivedAt":"2017-03-22T20:08:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Jan Palus \n\nThe came in an earlier sb/submodule-update-initial-runs-custom-script\ntopic that was merged to 2.12.\n\nSigned-off-by: Junio C Hamano \n---\n\n * This should be applied on top of e7b37caf (\"submodule update: run\n custom update script for initial populating as well\", 2017-01-25)\n\n t/t7406-submodule-update.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex 347857fa7c..a20df9420a 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -442,9 +442,9 @@ test_expect_success 'submodule update - command in .git/config catches failure -\n '\n \n test_expect_success 'submodule update - command run for initial population of submodule' '\n-\tcat <<-\\ EOF >expect\n+\tcat >expect <<-\\EOF &&\n \tExecution of '\\''false $submodulesha1'\\'' failed in submodule path '\\''submodule'\\''\n-\tEOF &&\n+\tEOF\n \trm -rf super/submodule &&\n \ttest_must_fail git -C super submodule update >../actual &&\n \ttest_cmp expect actual &&\n-- \n2.12.1-430-gafd6726309\n\n"},{"id":"314960","messageId":"20170322200805.23837-4-gitster@pobox.com","threadId":"45468","inReplyTo":"20170322200805.23837-1-gitster@pobox.com","subject":"[PATCH 3/3] t7004, t7030: fix here-doc syntax errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-22T20:08:05Z","receivedAt":"2017-03-22T20:08:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Jan Palus \n\nThese all came as part of an earlier st/verify-tag topic that was\nmerged to 2.12.\n\nSigned-off-by: Junio C Hamano \n---\n\n * This should be applied on top of 4fea72f4 (\"t/t7004-tag: Add\n --format specifier tests\", 2017-01-17)\n\n t/t7004-tag.sh | 8 ++++----\n t/t7030-verify-tag.sh | 8 ++++----\n 2 files changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex b4698ab5f5..efc6dde7b8 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -896,17 +896,17 @@ test_expect_success GPG 'verifying a forged tag should fail' '\n '\n \n test_expect_success 'verifying a proper tag with --format pass and format accordingly' '\n-\tcat >expect <<-\\EOF\n+\tcat >expect <<-\\EOF &&\n \ttagname : signed-tag\n-\tEOF &&\n+\tEOF\n \tgit tag -v --format=\"tagname : %(tag)\" \"signed-tag\" >actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success 'verifying a forged tag with --format fail and format accordingly' '\n-\tcat >expect <<-\\EOF\n+\tcat >expect <<-\\EOF &&\n \ttagname : forged-tag\n-\tEOF &&\n+\tEOF\n \ttest_must_fail git tag -v --format=\"tagname : %(tag)\" \"forged-tag\" >actual &&\n \ttest_cmp expect actual\n '\ndiff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh\nindex d62ccbb98e..ce37fd9864 100755\n--- a/t/t7030-verify-tag.sh\n+++ b/t/t7030-verify-tag.sh\n@@ -126,17 +126,17 @@ test_expect_success GPG 'verify multiple tags' '\n '\n \n test_expect_success 'verifying tag with --format' '\n-\tcat >expect <<-\\EOF\n+\tcat >expect <<-\\EOF &&\n \ttagname : fourth-signed\n-\tEOF &&\n+\tEOF\n \tgit verify-tag --format=\"tagname : %(tag)\" \"fourth-signed\" >actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success 'verifying a forged tag with --format fail and format accordingly' '\n-\tcat >expect <<-\\EOF\n+\tcat >expect <<-\\EOF &&\n \ttagname : 7th forged-signed\n-\tEOF &&\n+\tEOF\n \ttest_must_fail git verify-tag --format=\"tagname : %(tag)\" $(cat forged1.tag) >actual-forged &&\n \ttest_cmp expect actual-forged\n '\n-- \n2.12.1-430-gafd6726309\n\n"},{"id":"314964","messageId":"20170322210218.6jyzg53k55ixodt2@sigill.intra.peff.net","threadId":"45468","inReplyTo":"20170322200805.23837-2-gitster@pobox.com","subject":"Re: [PATCH 1/3] t5615: fix a here-doc syntax error","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-22T21:02:18Z","receivedAt":"2017-03-22T21:02:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 22, 2017 at 01:08:03PM -0700, Junio C Hamano wrote:\n\n> From: Jan Palus \n> \n> This came as part of jk/quote-env-path-list-component and was merged\n> to 2.11.1 and later.\n> \n> Signed-off-by: Junio C Hamano \n> ---\n> \n> * This should be applied on top of 5e74824f (\"t5615-alternate-env:\n> double-quotes in file names do not work on Windows\", 2016-12-21)\n> \n> t/t5615-alternate-env.sh | 1 +\n> 1 file changed, 1 insertion(+)\n> \n> diff --git a/t/t5615-alternate-env.sh b/t/t5615-alternate-env.sh\n> index 26ebb0375d..d2d883f3a1 100755\n> --- a/t/t5615-alternate-env.sh\n> +++ b/t/t5615-alternate-env.sh\n> @@ -77,6 +77,7 @@ test_expect_success 'mix of quoted and unquoted alternates' '\n> \tcheck_obj \"$quoted:$unquoted\" <<-EOF\n> \t$one blob\n> \t$two blob\n> +\tEOF\n> '\n\nThanks, this one is my fault, and the solution is obviously correct.\n\n-Peff\n"},{"id":"314966","messageId":"20170322210733.ohb7duy7bmmon6ny@sigill.intra.peff.net","threadId":"45468","inReplyTo":"20170322200805.23837-3-gitster@pobox.com","subject":"Re: [PATCH 2/3] t7406: fix here-doc syntax errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-22T21:07:33Z","receivedAt":"2017-03-22T21:07:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 22, 2017 at 01:08:04PM -0700, Junio C Hamano wrote:\n\n> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\n> index 347857fa7c..a20df9420a 100755\n> --- a/t/t7406-submodule-update.sh\n> +++ b/t/t7406-submodule-update.sh\n> @@ -442,9 +442,9 @@ test_expect_success 'submodule update - command in .git/config catches failure -\n> '\n> \n> test_expect_success 'submodule update - command run for initial population of submodule' '\n> -\tcat <<-\\ EOF >expect\n> +\tcat >expect <<-\\EOF &&\n> \tExecution of '\\''false $submodulesha1'\\'' failed in submodule path '\\''submodule'\\''\n> -\tEOF &&\n> +\tEOF\n\nAfter applying this, I get a failure:\n\n --- expect\t2017-03-22 21:01:53.350721155 +0000\n +++ actual\t2017-03-22 21:01:53.346721155 +0000\n @@ -1 +1 @@\n -Execution of 'false $submodulesha1' failed in submodule path 'submodule'\n +Execution of 'false 4301fd3e4110d3b6212c19aed3094150392545b9' failed in submodule path '../submodule'\n\nAt the very least, we need to drop the \"\\\" from EOF to expand\n$submodulesha1. But the submodule path seems wrong, too. I'm not sure if\nthe expectation is wrong, or if there's a bug. +cc Stefan\n\n-Peff\n"},{"id":"314968","messageId":"20170322211003.b52cql3iwig2xqcd@sigill.intra.peff.net","threadId":"45468","inReplyTo":"20170322200805.23837-4-gitster@pobox.com","subject":"Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-22T21:10:03Z","receivedAt":"2017-03-22T21:10:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 22, 2017 at 01:08:05PM -0700, Junio C Hamano wrote:\n\n> From: Jan Palus \n> \n> These all came as part of an earlier st/verify-tag topic that was\n> merged to 2.12.\n> \n> Signed-off-by: Junio C Hamano \n> ---\n> \n> * This should be applied on top of 4fea72f4 (\"t/t7004-tag: Add\n> --format specifier tests\", 2017-01-17)\n> \n> t/t7004-tag.sh | 8 ++++----\n> t/t7030-verify-tag.sh | 8 ++++----\n> 2 files changed, 8 insertions(+), 8 deletions(-)\n\nLike 2/3, this one also produces test failures for me. It looks like\n\"verify-tag\" does not show a tag which has been forged. I'm not sure if\nthat's intentional (and the test is wrong) or a bug. +cc Santiago\n\n-Peff\n"},{"id":"314975","messageId":"CAGZ79kafNKwfLhE1nXyt5mc5-D-kdxRxDP9pV7gc3btvjiHV7Q@mail.gmail.com","threadId":"45468","inReplyTo":"20170322210733.ohb7duy7bmmon6ny@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] t7406: fix here-doc syntax errors","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-22T21:32:22Z","receivedAt":"2017-03-22T21:33:10Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Mar 22, 2017 at 2:07 PM, Jeff King wrote:\n> On Wed, Mar 22, 2017 at 01:08:04PM -0700, Junio C Hamano wrote:\n>\n>> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\n>> index 347857fa7c..a20df9420a 100755\n>> --- a/t/t7406-submodule-update.sh\n>> +++ b/t/t7406-submodule-update.sh\n>> @@ -442,9 +442,9 @@ test_expect_success 'submodule update - command in .git/config catches failure -\n>> '\n>>\n>> test_expect_success 'submodule update - command run for initial population of submodule' '\n>> - cat <<-\\ EOF >expect\n>> + cat >expect <<-\\EOF &&\n>> Execution of '\\''false $submodulesha1'\\'' failed in submodule path '\\''submodule'\\''\n>> - EOF &&\n>> + EOF\n>\n> After applying this, I get a failure:\n>\n> --- expect 2017-03-22 21:01:53.350721155 +0000\n> +++ actual 2017-03-22 21:01:53.346721155 +0000\n> @@ -1 +1 @@\n> -Execution of 'false $submodulesha1' failed in submodule path 'submodule'\n> +Execution of 'false 4301fd3e4110d3b6212c19aed3094150392545b9' failed in submodule path '../submodule'\n>\n> At the very least, we need to drop the \"\\\" from EOF to expand\n> $submodulesha1.\n\nyes.\n\n> But the submodule path seems wrong, too. I'm not sure if\n> the expectation is wrong, or if there's a bug. +cc Stefan\n\nYes the actual output is wrong, too.\nBut that is not because Git is wrong, but the test suite is.\n\n test_must_fail git -C super submodule update >../actual &&\n\ncontains 2 errors:\n\n* we are interested in stderr, so make it 2>\n* the -C switch doesn't apply to redirection. (obviously!)\n So if you have run the test suite in a normal setup, you may have\n a file \"t/actual\" which is empty. This is scary as it managed to break\n out of the test suite and overwrote a potential file in your git.git.\n\nI'll send a patch on top of the one under discussion momentarily.\n\nThanks,\nStefan\n"},{"id":"314976","messageId":"xmqqfui5hu7p.fsf@gitster.mtv.corp.google.com","threadId":"45468","inReplyTo":"20170322210733.ohb7duy7bmmon6ny@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] t7406: fix here-doc syntax errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-22T21:34:02Z","receivedAt":"2017-03-22T21:34:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> After applying this, I get a failure:\n>\n> --- expect\t2017-03-22 21:01:53.350721155 +0000\n> +++ actual\t2017-03-22 21:01:53.346721155 +0000\n> @@ -1 +1 @@\n> -Execution of 'false $submodulesha1' failed in submodule path 'submodule'\n> +Execution of 'false 4301fd3e4110d3b6212c19aed3094150392545b9' failed in submodule path '../submodule'\n>\n> At the very least, we need to drop the \"\\\" from EOF to expand\n> $submodulesha1.\n\nRight.\n\n> But the submodule path seems wrong, too. I'm not sure if\n> the expectation is wrong, or if there's a bug. +cc Stefan\n"},{"id":"314979","messageId":"20170322213953.oddbgw5nfrvmzjgm@sigill.intra.peff.net","threadId":"45468","inReplyTo":"CAGZ79kafNKwfLhE1nXyt5mc5-D-kdxRxDP9pV7gc3btvjiHV7Q@mail.gmail.com","subject":"Re: [PATCH 2/3] t7406: fix here-doc syntax errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-22T21:39:53Z","receivedAt":"2017-03-22T21:40:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 22, 2017 at 02:32:22PM -0700, Stefan Beller wrote:\n\n> > But the submodule path seems wrong, too. I'm not sure if\n> > the expectation is wrong, or if there's a bug. +cc Stefan\n> \n> Yes the actual output is wrong, too.\n> But that is not because Git is wrong, but the test suite is.\n> \n> test_must_fail git -C super submodule update >../actual &&\n> \n> contains 2 errors:\n> \n> * we are interested in stderr, so make it 2>\n> * the -C switch doesn't apply to redirection. (obviously!)\n> So if you have run the test suite in a normal setup, you may have\n> a file \"t/actual\" which is empty. This is scary as it managed to break\n> out of the test suite and overwrote a potential file in your git.git.\n\nOh yeah, I didn't even look at the test itself, but I see I now have\nt/actual in my repository. Yikes. That didn't happen before because it\nsimply sucked the entire command into the here document.\n\n> I'll send a patch on top of the one under discussion momentarily.\n\nI'd prefer if they come in a single patch so that we don't break bisect.\n\n-Peff\n"},{"id":"314981","messageId":"20170322214356.5jj7dggtvgilreci@LykOS.localdomain","threadId":"45468","inReplyTo":"20170322211003.b52cql3iwig2xqcd@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors","fromName":"Santiago Torres","fromEmail":"santiago@nyu.edu","sentAt":"2017-03-22T21:43:57Z","receivedAt":"2017-03-22T21:44:14Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"On Wed, Mar 22, 2017 at 05:10:03PM -0400, Jeff King wrote:\n> On Wed, Mar 22, 2017 at 01:08:05PM -0700, Junio C Hamano wrote:\n> \n> > From: Jan Palus \n> > \n> > These all came as part of an earlier st/verify-tag topic that was\n> > merged to 2.12.\n> > \n> > Signed-off-by: Junio C Hamano \n> > ---\n> > \n> > * This should be applied on top of 4fea72f4 (\"t/t7004-tag: Add\n> > --format specifier tests\", 2017-01-17)\n> > \n> > t/t7004-tag.sh | 8 ++++----\n> > t/t7030-verify-tag.sh | 8 ++++----\n> > 2 files changed, 8 insertions(+), 8 deletions(-)\n> \n\nOn my side, these patches make tests fail. I'm wondering if this is an\nissue with the underlying shell (probably the version)? Let me try to\nfigure out what is exactly going on here.\n\n> Like 2/3, this one also produces test failures for me. It looks like\n> \"verify-tag\" does not show a tag which has been forged. I'm not sure if\n> that's intentional (and the test is wrong) or a bug. \n\nI see that offending code would be [1]. Changing this behavior should be\ntrivial (dropping the continue), although I'm not sure if this is what\nwe want?\n\n> \n> -Peff\n\nThanks,\n-Santiago.\n\n[1] https://github.com/git/git/blob/master/builtin/verify-tag.c#L6://github.com/git/git/blob/master/builtin/verify-tag.c#L67\n"},{"id":"314985","messageId":"20170322214948.12015-1-sbeller@google.com","threadId":"45468","inReplyTo":"20170322213953.oddbgw5nfrvmzjgm@sigill.intra.peff.net","subject":"[PATCH] t7406: correct test case for submodule-update initial population","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-22T21:49:48Z","receivedAt":"2017-03-22T21:50:09Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"There are three issues with the test:\n\n* The syntax of the here-doc was wrong, such that the entire test was\n sucked into the here-doc, which is why the test succeeded successfully.\n\n* The variable $submodulesha1 was not expanded as it was inside a single\n quoted string. Use double quote to expand the variable.\n\n* The redirection from the git command to the output file for comparison\n was wrong as the -C operator from git doesn't apply to the redirect path.\n Also we're interested in stderr of that command.\n\nNoticed-by: Jan Palus \nSigned-off-by: Junio C Hamano \nSigned-off-by: Stefan Beller \n---\n\nThis is just one patch (for bisectability)\nit applies on e7b37caf4fe.\n\nThanks,\nStefan\n\n t/t7406-submodule-update.sh | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex 8c086a4..c327eb6 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -425,11 +425,11 @@ test_expect_success 'submodule update - command in .git/config catches failure -\n '\n \n test_expect_success 'submodule update - command run for initial population of submodule' '\n-\tcat <<-\\ EOF >expect\n-\tExecution of '\\''false $submodulesha1'\\'' failed in submodule path '\\''submodule'\\''\n-\tEOF &&\n+\tcat >expect <<-\\EOF &&\n+\tExecution of '\\'\"false $submodulesha1\"\\'' failed in submodule path '\\''submodule'\\''\n+\tEOF\n \trm -rf super/submodule &&\n-\ttest_must_fail git -C super submodule update >../actual &&\n+\ttest_must_fail git -C super submodule update 2>actual &&\n \ttest_cmp expect actual &&\n \tgit -C super submodule update --checkout\n '\n-- \n2.10.2.50.g9d09a6e.dirty\n\n"},{"id":"314987","messageId":"20170322215951.f4xbxehzvsd64c4y@sigill.intra.peff.net","threadId":"45468","inReplyTo":"20170322214948.12015-1-sbeller@google.com","subject":"Re: [PATCH] t7406: correct test case for submodule-update initial population","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-22T21:59:51Z","receivedAt":"2017-03-22T22:00:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 22, 2017 at 02:49:48PM -0700, Stefan Beller wrote:\n\n> * The syntax of the here-doc was wrong, such that the entire test was\n> sucked into the here-doc, which is why the test succeeded successfully.\n\nAs opposed to succeeding unsuccessfully? :)\n\n> * The variable $submodulesha1 was not expanded as it was inside a single\n> quoted string. Use double quote to expand the variable.\n\nHmm. Sort of. It was inside a non-interpolating here-doc inside a\nsingle-quoted string which was being eval'd. The second half is fine\n(the eval adds an extra layer of evaluation).\n\nYour fix:\n\n> +\tcat >expect <<-\\EOF &&\n> +\tExecution of '\\'\"false $submodulesha1\"\\'' failed in submodule path '\\''submodule'\\''\n> +\tEOF\n\n_does_ work, but it does so because it's evaluating $submodulesha1 in\nthe shell snippet and handing the result off to test_expect_success to\neval. So it would have problems if:\n\n - that variable contained \"\\nEOF\\n\" itself ;)\n\n - the variable was modified inside the shell snippet.\n\nNeither of those is true, but I think:\n\n cat >expect <<-EOF &&\n Execution of '\\''false $submodulesha1'\\'' failed in ...\n EOF\n\nis safer and less surprising. The single-quote handling is unfortunate and\nugly, but necessary to get them into the shell snippet in the first\nplace. I notice the others tests in this script set up the expect file\noutside of a block. You could also do something like:\n\n sq=\\'\n test_expect_success '...' '\n\tcat >expect <<-EOF\n\tExecution of ${sq}false $submodulesha1${sq} ...\n '\n\nbut I'm not sure if that is any more readable.\n\n-Peff\n"},{"id":"314989","messageId":"20170322220404.ierzftpw3liugguo@sigill.intra.peff.net","threadId":"45468","inReplyTo":"20170322214356.5jj7dggtvgilreci@LykOS.localdomain","subject":"Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-22T22:04:05Z","receivedAt":"2017-03-22T22:04:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 22, 2017 at 05:43:57PM -0400, Santiago Torres wrote:\n\n> > Like 2/3, this one also produces test failures for me. It looks like\n> > \"verify-tag\" does not show a tag which has been forged. I'm not sure if\n> > that's intentional (and the test is wrong) or a bug. \n> \n> I see that offending code would be [1]. Changing this behavior should be\n> trivial (dropping the continue), although I'm not sure if this is what\n> we want?\n\nI could see arguments for either behavior. The fact that v2.12 was\nreleased with the skip-gpg-failures behavior makes me inclined to just\nkeep that and fix the test. But I don't feel strongly.\n\nIf we do change it, I think builtin/tag.c:verify_tag() would need a\nsimilar fix (to avoid the early return).\n\n-Peff\n"},{"id":"314990","messageId":"xmqq8tnxhssv.fsf@gitster.mtv.corp.google.com","threadId":"45468","inReplyTo":"20170322211003.b52cql3iwig2xqcd@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-22T22:04:32Z","receivedAt":"2017-03-22T22:04:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Mar 22, 2017 at 01:08:05PM -0700, Junio C Hamano wrote:\n>\n>> From: Jan Palus \n>> \n>> These all came as part of an earlier st/verify-tag topic that was\n>> merged to 2.12.\n>> \n>> Signed-off-by: Junio C Hamano \n>> ---\n>> \n>> * This should be applied on top of 4fea72f4 (\"t/t7004-tag: Add\n>> --format specifier tests\", 2017-01-17)\n>> \n>> t/t7004-tag.sh | 8 ++++----\n>> t/t7030-verify-tag.sh | 8 ++++----\n>> 2 files changed, 8 insertions(+), 8 deletions(-)\n>\n> Like 2/3, this one also produces test failures for me. It looks like\n> \"verify-tag\" does not show a tag which has been forged. I'm not sure if\n> that's intentional (and the test is wrong) or a bug. +cc Santiago\n\nIt appears that the test expected a broken one to be shown, and my\nreading of its log message is that the change expected --format= to\nbe used with %G? so that scripts can tell between pass and fail? \n\nSo if I have to judge, the code becoming silent for a tag that does\nnot pass verification is not doing what the commit wanted it to do.\n\n"},{"id":"314991","messageId":"CAGZ79kbFjCQB_k8E1dMVop_EGF6TpnY04U1NV60VmSs1zHOnZw@mail.gmail.com","threadId":"45468","inReplyTo":"20170322215951.f4xbxehzvsd64c4y@sigill.intra.peff.net","subject":"Re: [PATCH] t7406: correct test case for submodule-update initial population","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-22T22:07:45Z","receivedAt":"2017-03-22T22:07:51Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Mar 22, 2017 at 2:59 PM, Jeff King wrote:\n> On Wed, Mar 22, 2017 at 02:49:48PM -0700, Stefan Beller wrote:\n>\n>> * The syntax of the here-doc was wrong, such that the entire test was\n>> sucked into the here-doc, which is why the test succeeded successfully.\n>\n> As opposed to succeeding unsuccessfully? :)\n>\n>> * The variable $submodulesha1 was not expanded as it was inside a single\n>> quoted string. Use double quote to expand the variable.\n>\n> Hmm. Sort of. It was inside a non-interpolating here-doc inside a\n> single-quoted string which was being eval'd. The second half is fine\n> (the eval adds an extra layer of evaluation).\n>\n> Your fix:\n>\n>> + cat >expect <<-\\EOF &&\n>> + Execution of '\\'\"false $submodulesha1\"\\'' failed in submodule path '\\''submodule'\\''\n>> + EOF\n>\n> _does_ work, but it does so because it's evaluating $submodulesha1 in\n> the shell snippet and handing the result off to test_expect_success to\n> eval. So it would have problems if:\n>\n> - that variable contained \"\\nEOF\\n\" itself ;)\n>\n> - the variable was modified inside the shell snippet.\n>\n> Neither of those is true, but I think:\n>\n> cat >expect <<-EOF &&\n> Execution of '\\''false $submodulesha1'\\'' failed in ...\n> EOF\n>\n> is safer and less surprising. The single-quote handling is unfortunate and\n> ugly, but necessary to get them into the shell snippet in the first\n> place. I notice the others tests in this script set up the expect file\n> outside of a block. You could also do something like:\n>\n> sq=\\'\n> test_expect_success '...' '\n> cat >expect <<-EOF\n> Execution of ${sq}false $submodulesha1${sq} ...\n> '\n>\n> but I'm not sure if that is any more readable.\n>\n\nIf I recall correctly, I made a big fuss about single quotes used correctly when\nwriting that patch (which is why I may have lost track of the actual work there)\nto be told the one and only blessed way to use single quotes in our test suite.\n\nYour proposal to use ${sq} sounds good to me, though we did not\nfollow through with it for some other reason. I can reroll with that, though.\n\nThanks,\nStefan\n"},{"id":"314993","messageId":"20170322220934.ru7wgujbpg5lq2w6@sigill.intra.peff.net","threadId":"45468","inReplyTo":"CAGZ79kbFjCQB_k8E1dMVop_EGF6TpnY04U1NV60VmSs1zHOnZw@mail.gmail.com","subject":"Re: [PATCH] t7406: correct test case for submodule-update initial population","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-22T22:09:35Z","receivedAt":"2017-03-22T22:09:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 22, 2017 at 03:07:45PM -0700, Stefan Beller wrote:\n\n> > Neither of those is true, but I think:\n> >\n> > cat >expect <<-EOF &&\n> > Execution of '\\''false $submodulesha1'\\'' failed in ...\n> > EOF\n> >\n> > is safer and less surprising. The single-quote handling is unfortunate and\n> > ugly, but necessary to get them into the shell snippet in the first\n> > place. I notice the others tests in this script set up the expect file\n> > outside of a block. You could also do something like:\n> >\n> > sq=\\'\n> > test_expect_success '...' '\n> > cat >expect <<-EOF\n> > Execution of ${sq}false $submodulesha1${sq} ...\n> > '\n> >\n> > but I'm not sure if that is any more readable.\n> >\n> \n> If I recall correctly, I made a big fuss about single quotes used correctly when\n> writing that patch (which is why I may have lost track of the actual work there)\n> to be told the one and only blessed way to use single quotes in our test suite.\n> \n> Your proposal to use ${sq} sounds good to me, though we did not\n> follow through with it for some other reason. I can reroll with that, though.\n\nI'm not sure if it's a proposal. I ended it with \"I'm not sure if that\nis any more readable\". :)\n\nI mainly care about making sure the interpolation happens inside the\ntest block (i.e., at the top of the quoted section above).\n\n-Peff\n"},{"id":"314994","messageId":"xmqq37e5hsg8.fsf@gitster.mtv.corp.google.com","threadId":"45468","inReplyTo":"20170322215951.f4xbxehzvsd64c4y@sigill.intra.peff.net","subject":"Re: [PATCH] t7406: correct test case for submodule-update initial population","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-22T22:12:07Z","receivedAt":"2017-03-22T22:13:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Neither of those is true, but I think:\n>\n> cat >expect <<-EOF &&\n> Execution of '\\''false $submodulesha1'\\'' failed in ...\n> EOF\n>\n> is safer and less surprising. The single-quote handling is unfortunate and\n> ugly, but necessary to get them into the shell snippet in the first\n> place. I notice the others tests in this script set up the expect file\n> outside of a block. You could also do something like:\n>\n> sq=\\'\n> test_expect_success '...' '\n> \tcat >expect <<-EOF\n> \tExecution of ${sq}false $submodulesha1${sq} ...\n> '\n>\n> but I'm not sure if that is any more readable.\n\nYup, my eyes have long learned to coast over '\\'' as an idiomatic\nsymbol, but I agree that it is harder to see until you get used to\nit (and I do not think it is particularly useful skill to be able to\nspot '\\'' as a logical unit, either). ${sq} thing may make it easier\nto read but I think the one you did in the first quoted part in this\nreply is good enough.\n\n-- >8 --\nSubject: t7406: correct test case for submodule-update initial population\n\nThere are three issues with the test:\n\n* The syntax of the here-doc was wrong, such that the entire test was\n sucked into the here-doc, which is why the test succeeded successfully.\n\n* The variable $submodulesha1 was not expanded as it was inside a quoted\n here text. We do not want to quote EOF marker for this.\n\n* The redirection from the git command to the output file for comparison\n was wrong as the -C operator from git doesn't apply to the redirect path.\n Also we're interested in stderr of that command.\n\nNoticed-by: Jan Palus \nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n\n t/t7406-submodule-update.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex 8c086a429b..a70fe96ad6 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -425,11 +425,11 @@ test_expect_success 'submodule update - command in .git/config catches failure -\n '\n \n test_expect_success 'submodule update - command run for initial population of submodule' '\n-\tcat <<-\\ EOF >expect\n+\tcat >expect <<-EOF &&\n \tExecution of '\\''false $submodulesha1'\\'' failed in submodule path '\\''submodule'\\''\n-\tEOF &&\n+\tEOF\n \trm -rf super/submodule &&\n-\ttest_must_fail git -C super submodule update >../actual &&\n+\ttest_must_fail git -C super submodule update 2>actual &&\n \ttest_cmp expect actual &&\n \tgit -C super submodule update --checkout\n '\n\n"},{"id":"314996","messageId":"20170322221414.GE26108@aiede.mtv.corp.google.com","threadId":"45468","inReplyTo":"CAGZ79kbFjCQB_k8E1dMVop_EGF6TpnY04U1NV60VmSs1zHOnZw@mail.gmail.com","subject":"Re: [PATCH] t7406: correct test case for submodule-update initial population","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-03-22T22:14:14Z","receivedAt":"2017-03-22T22:14:22Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n> On Wed, Mar 22, 2017 at 2:59 PM, Jeff King wrote:\n\n>> sq=\\'\n>> test_expect_success '...' '\n>> cat >expect <<-EOF\n>> Execution of ${sq}false $submodulesha1${sq} ...\n>> '\n>>\n>> but I'm not sure if that is any more readable.\n>\n> If I recall correctly, I made a big fuss about single quotes used correctly when\n> writing that patch (which is why I may have lost track of the actual work there)\n> to be told the one and only blessed way to use single quotes in our test suite.\n>\n> Your proposal to use ${sq} sounds good to me, though we did not\n> follow through with it for some other reason. I can reroll with that, though.\n\nI don't know what you're referring to by only and blessed way, but it\nmight be the following. If you want to use single-quotes on a command\nline using $sq, you would have to use eval:\n\n\teval \"some_command ${sq}single-quoted argument${sq}\"\n\nwhich is generally more complicated than doing the '\\'' dance:\n\n\tsome_command '\\''single-quoted argument'\\''\n\nBut the example in this thread is different. In the here-document you\nare in an interpolated context so the ${sq} method or '\\'' would work\nequally well.\n\nHope that helps,\nJonathan\n"},{"id":"314997","messageId":"20170322221556.j7uj4vvgbcubcr3b@LykOS.localdomain","threadId":"45468","inReplyTo":"xmqq8tnxhssv.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors","fromName":"Santiago Torres","fromEmail":"santiago@nyu.edu","sentAt":"2017-03-22T22:15:57Z","receivedAt":"2017-03-22T22:16:05Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"On Wed, Mar 22, 2017 at 03:04:32PM -0700, Junio C Hamano wrote:\n> Jeff King writes:\n> \n> > On Wed, Mar 22, 2017 at 01:08:05PM -0700, Junio C Hamano wrote:\n> >\n> >> From: Jan Palus \n> >> \n> >> These all came as part of an earlier st/verify-tag topic that was\n> >> merged to 2.12.\n> >> \n> >> Signed-off-by: Junio C Hamano \n> >> ---\n> >> \n> >> * This should be applied on top of 4fea72f4 (\"t/t7004-tag: Add\n> >> --format specifier tests\", 2017-01-17)\n> >> \n> >> t/t7004-tag.sh | 8 ++++----\n> >> t/t7030-verify-tag.sh | 8 ++++----\n> >> 2 files changed, 8 insertions(+), 8 deletions(-)\n> >\n> > Like 2/3, this one also produces test failures for me. It looks like\n> > \"verify-tag\" does not show a tag which has been forged. I'm not sure if\n> > that's intentional (and the test is wrong) or a bug. +cc Santiago\n> \n> It appears that the test expected a broken one to be shown, and my\n> reading of its log message is that the change expected --format= to\n> be used with %G? so that scripts can tell between pass and fail? \n> \n> So if I have to judge, the code becoming silent for a tag that does\n> not pass verification is not doing what the commit wanted it to do.\n> \n\nYes, considering the test name is:\n\n \"verifying a forged tag with --format fail and format accordingly\" \n\nIt feels as if the behavior of verify-tag/tag -v is not the one\nintended. I could add two patches on top of those two commits.\n\nWould this be enough?\n-Santiago.\n"},{"id":"315005","messageId":"20170322222230.yqqv6x4gokvb4jbz@sigill.intra.peff.net","threadId":"45468","inReplyTo":"20170322221556.j7uj4vvgbcubcr3b@LykOS.localdomain","subject":"Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-22T22:22:30Z","receivedAt":"2017-03-22T22:22:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 22, 2017 at 06:15:57PM -0400, Santiago Torres wrote:\n\n> > > Like 2/3, this one also produces test failures for me. It looks like\n> > > \"verify-tag\" does not show a tag which has been forged. I'm not sure if\n> > > that's intentional (and the test is wrong) or a bug. +cc Santiago\n> > \n> > It appears that the test expected a broken one to be shown, and my\n> > reading of its log message is that the change expected --format= to\n> > be used with %G? so that scripts can tell between pass and fail? \n> > \n> > So if I have to judge, the code becoming silent for a tag that does\n> > not pass verification is not doing what the commit wanted it to do.\n> > \n> \n> Yes, considering the test name is:\n> \n> \"verifying a forged tag with --format fail and format accordingly\" \n> \n> It feels as if the behavior of verify-tag/tag -v is not the one\n> intended. I could add two patches on top of those two commits.\n\nI worked up the patch to do that (see below), but I got stumped trying\nto write the commit message. Perhaps that is what the test intended, but\nI don't think tag's --format understands \"%G\" codes at all. So you\ncannot tell from the output if a tag was valid or not; you have to check\nthe exit code separately.\n\nAnd if you do something like:\n\n git tag -v --format='%(tag)' foo bar |\n while read tag\n do\n ...\n done\n\nyou cannot tell at all which ones are bogus. Whereas with the current\nbehavior, the bogus ones are quietly omitted. Which can also be\nconfusing, but I'd think would generally err on the side of caution.\n\n-Peff\n\n---\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex fbb85ba3d..37e768db4 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -107,19 +107,19 @@ static int verify_tag(const char *name, const char *ref,\n \t\t const unsigned char *sha1, const void *cb_data)\n {\n \tint flags;\n+\tint ret;\n \tconst char *fmt_pretty = cb_data;\n \tflags = GPG_VERIFY_VERBOSE;\n \n \tif (fmt_pretty)\n \t\tflags = GPG_VERIFY_OMIT_STATUS;\n \n-\tif (gpg_verify_tag(sha1, name, flags))\n-\t\treturn -1;\n+\tret = gpg_verify_tag(sha1, name, flags);\n \n \tif (fmt_pretty)\n \t\tpretty_print_ref(name, sha1, fmt_pretty);\n \n-\treturn 0;\n+\treturn ret;\n }\n \n static int do_sign(struct strbuf *buffer)\ndiff --git a/builtin/verify-tag.c b/builtin/verify-tag.c\nindex 5199553d9..cb1024ef4 100644\n--- a/builtin/verify-tag.c\n+++ b/builtin/verify-tag.c\n@@ -62,10 +62,8 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)\n \t\t\tcontinue;\n \t\t}\n \n-\t\tif (gpg_verify_tag(sha1, name, flags)) {\n+\t\tif (gpg_verify_tag(sha1, name, flags))\n \t\t\thad_error = 1;\n-\t\t\tcontinue;\n-\t\t}\n \n \t\tif (fmt_pretty)\n \t\t\tpretty_print_ref(name, sha1, fmt_pretty);\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex b53a2e5e4..633b08956 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -848,17 +848,17 @@ test_expect_success GPG 'verifying a forged tag should fail' '\n '\n \n test_expect_success 'verifying a proper tag with --format pass and format accordingly' '\n-\tcat >expect <<-\\EOF\n+\tcat >expect <<-\\EOF &&\n \ttagname : signed-tag\n-\tEOF &&\n+\tEOF\n \tgit tag -v --format=\"tagname : %(tag)\" \"signed-tag\" >actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success 'verifying a forged tag with --format fail and format accordingly' '\n-\tcat >expect <<-\\EOF\n+\tcat >expect <<-\\EOF &&\n \ttagname : forged-tag\n-\tEOF &&\n+\tEOF\n \ttest_must_fail git tag -v --format=\"tagname : %(tag)\" \"forged-tag\" >actual &&\n \ttest_cmp expect actual\n '\ndiff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh\nindex d62ccbb98..ce37fd986 100755\n--- a/t/t7030-verify-tag.sh\n+++ b/t/t7030-verify-tag.sh\n@@ -126,17 +126,17 @@ test_expect_success GPG 'verify multiple tags' '\n '\n \n test_expect_success 'verifying tag with --format' '\n-\tcat >expect <<-\\EOF\n+\tcat >expect <<-\\EOF &&\n \ttagname : fourth-signed\n-\tEOF &&\n+\tEOF\n \tgit verify-tag --format=\"tagname : %(tag)\" \"fourth-signed\" >actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success 'verifying a forged tag with --format fail and format accordingly' '\n-\tcat >expect <<-\\EOF\n+\tcat >expect <<-\\EOF &&\n \ttagname : 7th forged-signed\n-\tEOF &&\n+\tEOF\n \ttest_must_fail git verify-tag --format=\"tagname : %(tag)\" $(cat forged1.tag) >actual-forged &&\n \ttest_cmp expect actual-forged\n '\n"},{"id":"315006","messageId":"20170322222404.jl3smvai2a3r6tzc@sigill.intra.peff.net","threadId":"45468","inReplyTo":"xmqq37e5hsg8.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] t7406: correct test case for submodule-update initial population","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-22T22:24:04Z","receivedAt":"2017-03-22T22:24:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 22, 2017 at 03:12:07PM -0700, Junio C Hamano wrote:\n\n> > sq=\\'\n> > test_expect_success '...' '\n> > \tcat >expect <<-EOF\n> > \tExecution of ${sq}false $submodulesha1${sq} ...\n> > '\n> >\n> > but I'm not sure if that is any more readable.\n> \n> Yup, my eyes have long learned to coast over '\\'' as an idiomatic\n> symbol, but I agree that it is harder to see until you get used to\n> it (and I do not think it is particularly useful skill to be able to\n> spot '\\'' as a logical unit, either). ${sq} thing may make it easier\n> to read but I think the one you did in the first quoted part in this\n> reply is good enough.\n\nSounds good.\n\n> -- >8 --\n> Subject: t7406: correct test case for submodule-update initial population\n> \n> There are three issues with the test:\n> \n> * The syntax of the here-doc was wrong, such that the entire test was\n> sucked into the here-doc, which is why the test succeeded successfully.\n\nThis version looks fine except for the repetitious repetition. :)\n\n-Peff\n"},{"id":"315008","messageId":"CAGZ79ka_kErObJAuJrvFxnEMsESCr_2jc+RQ1AMuWx_urWs1mQ@mail.gmail.com","threadId":"45468","inReplyTo":"20170322222404.jl3smvai2a3r6tzc@sigill.intra.peff.net","subject":"Re: [PATCH] t7406: correct test case for submodule-update initial population","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-22T22:28:07Z","receivedAt":"2017-03-22T22:28:13Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Mar 22, 2017 at 3:24 PM, Jeff King wrote:\n> On Wed, Mar 22, 2017 at 03:12:07PM -0700, Junio C Hamano wrote:\n>\n>> > sq=\\'\n>> > test_expect_success '...' '\n>> > cat >expect <<-EOF\n>> > Execution of ${sq}false $submodulesha1${sq} ...\n>> > '\n>> >\n>> > but I'm not sure if that is any more readable.\n>>\n>> Yup, my eyes have long learned to coast over '\\'' as an idiomatic\n>> symbol, but I agree that it is harder to see until you get used to\n>> it (and I do not think it is particularly useful skill to be able to\n>> spot '\\'' as a logical unit, either). ${sq} thing may make it easier\n>> to read but I think the one you did in the first quoted part in this\n>> reply is good enough.\n>\n> Sounds good.\n>\n>> -- >8 --\n>> Subject: t7406: correct test case for submodule-update initial population\n>>\n>> There are three issues with the test:\n>>\n>> * The syntax of the here-doc was wrong, such that the entire test was\n>> sucked into the here-doc, which is why the test succeeded successfully.\n>\n> This version looks fine except for the repetitious repetition. :)\n\nWhen I wrote it, It sounded more convincing, but we can drop the\n\"successfully\".\n\nThanks,\nStefan\n"},{"id":"315011","messageId":"20170322223441.w32y464jqbnxnzna@LykOS.localdomain","threadId":"45468","inReplyTo":"20170322222230.yqqv6x4gokvb4jbz@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors","fromName":"Santiago Torres","fromEmail":"santiago@nyu.edu","sentAt":"2017-03-22T22:34:43Z","receivedAt":"2017-03-22T22:34:50Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"> I worked up the patch to do that (see below), but I got stumped trying\n> to write the commit message. Perhaps that is what the test intended, but\n> I don't think tag's --format understands \"%G\" codes at all. So you\n> cannot tell from the output if a tag was valid or not; you have to check\n> the exit code separately.\n> \n> And if you do something like:\n> \n> git tag -v --format='%(tag)' foo bar |\n> while read tag\n> do\n> ...\n> done\n> \n> you cannot tell at all which ones are bogus. Whereas with the current\n> behavior, the bogus ones are quietly omitted. Which can also be\n> confusing, but I'd think would generally err on the side of caution.\n\nIn that case, something like this would be closer to the desired\nbehavior?\n\nI'm also unsure on what would be the right thing to put on the commit\nmessage.\n\n-Santiago.\n\n---\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex b4698ab5f..70f6d4646 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -896,19 +896,18 @@ test_expect_success GPG 'verifying a forged tag should fail' '\n '\n \n test_expect_success 'verifying a proper tag with --format pass and format accordingly' '\n-\tcat >expect <<-\\EOF\n+\tcat >expect <<-\\EOF &&\n \ttagname : signed-tag\n-\tEOF &&\n+\tEOF\n \tgit tag -v --format=\"tagname : %(tag)\" \"signed-tag\" >actual &&\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'verifying a forged tag with --format fail and format accordingly' '\n-\tcat >expect <<-\\EOF\n+test_expect_success 'verifying a forged tag with --format should fail silently' '\n+\tcat >expect <<-\\EOF &&\n \ttagname : forged-tag\n-\tEOF &&\n-\ttest_must_fail git tag -v --format=\"tagname : %(tag)\" \"forged-tag\" >actual &&\n-\ttest_cmp expect actual\n+\tEOF\n+\ttest_must_fail git tag -v --format=\"tagname : %(tag)\" \"forged-tag\"\n '\n \n # blank and empty messages for signed tags:\ndiff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh\nindex d62ccbb98..ba0aafa1f 100755\n--- a/t/t7030-verify-tag.sh\n+++ b/t/t7030-verify-tag.sh\n@@ -126,19 +126,18 @@ test_expect_success GPG 'verify multiple tags' '\n '\n \n test_expect_success 'verifying tag with --format' '\n-\tcat >expect <<-\\EOF\n+\tcat >expect <<-\\EOF &&\n \ttagname : fourth-signed\n-\tEOF &&\n+\tEOF\n \tgit verify-tag --format=\"tagname : %(tag)\" \"fourth-signed\" >actual &&\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'verifying a forged tag with --format fail and format accordingly' '\n-\tcat >expect <<-\\EOF\n+test_expect_success 'verifying a forged tag with --format should fail silently' '\n+\tcat >expect <<-\\EOF &&\n \ttagname : 7th forged-signed\n-\tEOF &&\n-\ttest_must_fail git verify-tag --format=\"tagname : %(tag)\" $(cat forged1.tag) >actual-forged &&\n-\ttest_cmp expect actual-forged\n+\tEOF\n+\ttest_must_fail git verify-tag --format=\"tagname : %(tag)\" $(cat forged1.tag)\n '\n \n test_done\n"},{"id":"315013","messageId":"xmqqlgrxgcn0.fsf@gitster.mtv.corp.google.com","threadId":"45468","inReplyTo":"20170322222230.yqqv6x4gokvb4jbz@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-22T22:38:59Z","receivedAt":"2017-03-22T22:39:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I worked up the patch to do that (see below), but I got stumped trying\n> to write the commit message. Perhaps that is what the test intended, but\n> I don't think tag's --format understands \"%G\" codes at all.\n> So you cannot tell from the output if a tag was valid or not; you have to check\n> the exit code separately.\n\nThanks for digging; that was an unexpected show-stopper to me X-<.\n\n> you cannot tell at all which ones are bogus. Whereas with the current\n> behavior, the bogus ones are quietly omitted. Which can also be\n> confusing, but I'd think would generally err on the side of caution.\n\nOK.\n\n"},{"id":"315015","messageId":"20170322224052.y4d7olfbootoftyq@kalarepa","threadId":"45468","inReplyTo":"20170322200805.23837-1-gitster@pobox.com","subject":"Re: [PATCH 0/3] fix \"here-doc\" syntax errors","fromName":"Jan Palus","fromEmail":"jan.palus@gmail.com","sentAt":"2017-03-22T22:40:52Z","receivedAt":"2017-03-22T22:41:27Z","isPatch":true,"sender":{"key":"jan.palus@gmail.com","avatar":null},"body":"On 22.03.2017 13:08, Junio C Hamano wrote:\n> Please respond to the list, saying it is OK to add your \"sign-off\"\n> (see Documentation/SubmittingPatches) to these patches.\n\nPlease feel free to do so and thanks for handling the issue.\n\n\nRegards\nJan\n"},{"id":"315016","messageId":"20170322224124.u3eax4ui3y4saxks@sigill.intra.peff.net","threadId":"45468","inReplyTo":"20170322223441.w32y464jqbnxnzna@LykOS.localdomain","subject":"Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-22T22:41:24Z","receivedAt":"2017-03-22T22:41:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 22, 2017 at 06:34:43PM -0400, Santiago Torres wrote:\n\n> > I worked up the patch to do that (see below), but I got stumped trying\n> > to write the commit message. Perhaps that is what the test intended, but\n> > I don't think tag's --format understands \"%G\" codes at all. So you\n> > cannot tell from the output if a tag was valid or not; you have to check\n> > the exit code separately.\n> > \n> > And if you do something like:\n> > \n> > git tag -v --format='%(tag)' foo bar |\n> > while read tag\n> > do\n> > ...\n> > done\n> > \n> > you cannot tell at all which ones are bogus. Whereas with the current\n> > behavior, the bogus ones are quietly omitted. Which can also be\n> > confusing, but I'd think would generally err on the side of caution.\n> \n> In that case, something like this would be closer to the desired\n> behavior?\n\nYes, though you can spell:\n\n cat >expect <<-\\EOF\n EOF\n\nas just:\n\n >expect\n\n> I'm also unsure on what would be the right thing to put on the commit\n> message.\n\nI think the argument is:\n\n 1. It's safer not to expound on tags that have failed verification (so\n that the caller cannot accidentally use them). Especially since the\n --format cannot tell anything about the GPG status.\n\n That means that\n\n tag=$(git verify-tag --format='%(tag)' foo)\n\n can use a non-blank $tag without having to wonder whether it is\n valid or not.\n\nand\n\n 2. That's what we've done since the feature was released.\n\nThe only thing that would give me pause is if were to later add\n%G-like formatters, and then:\n\n xargs git verify-tag --format='%(gpg:status) %(tag)' |\n while read status tag\n do\n ...\n done\n\nwould become useful, but we'd be tied to the behavior that we omit the\ntag when the gpg verification failed (for backwards compatibility).\nOTOH, we could perhaps make the rule \"ignored unless %(gpg) formatters\nare used\". Which would be backwards-compatible and safe for old formats,\nand work correctly for new ones.\n\n-Peff\n"},{"id":"315018","messageId":"xmqqh92lgc82.fsf@gitster.mtv.corp.google.com","threadId":"45468","inReplyTo":"20170322224124.u3eax4ui3y4saxks@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-22T22:47:57Z","receivedAt":"2017-03-22T22:48:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> OTOH, we could perhaps make the rule \"ignored unless %(gpg) formatters\n> are used\". Which would be backwards-compatible and safe for old formats,\n> and work correctly for new ones.\n\nYup, that is a very sensible escape hatch that we can use later.\n\nThanks.\n"},{"id":"315019","messageId":"20170322225108.wub4bmr63hk2sp33@LykOS.localdomain","threadId":"45468","inReplyTo":"20170322224124.u3eax4ui3y4saxks@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors","fromName":"Santiago Torres","fromEmail":"santiago@nyu.edu","sentAt":"2017-03-22T22:51:08Z","receivedAt":"2017-03-22T22:51:15Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"On Wed, Mar 22, 2017 at 06:41:24PM -0400, Jeff King wrote:\n> > In that case, something like this would be closer to the desired\n> > behavior?\n> \n> Yes, though you can spell:\n> \n> cat >expect <<-\\EOF\n> EOF\n> \n> as just:\n> \n> >expect\n\nAh, that sounds like a better way to fix this with a smaller diff.\n> \n> > I'm also unsure on what would be the right thing to put on the commit\n> > message.\n> \n> I think the argument is:\n> \n> 1. It's safer not to expound on tags that have failed verification (so\n> that the caller cannot accidentally use them). Especially since the\n> --format cannot tell anything about the GPG status.\n> \n> That means that\n> \n> tag=$(git verify-tag --format='%(tag)' foo)\n> \n> can use a non-blank $tag without having to wonder whether it is\n> valid or not.\n> \n> and\n> \n> 2. That's what we've done since the feature was released.\n> \n> The only thing that would give me pause is if were to later add\n> %G-like formatters, and then:\n> \n> xargs git verify-tag --format='%(gpg:status) %(tag)' |\n> while read status tag\n> do\n> ...\n> done\n> \n> would become useful, but we'd be tied to the behavior that we omit the\n> tag when the gpg verification failed (for backwards compatibility).\n> OTOH, we could perhaps make the rule \"ignored unless %(gpg) formatters\n> are used\". Which would be backwards-compatible and safe for old formats,\n> and work correctly for new ones.\n\nThis sounds like a helpful addition to implement. We could update/add\ntests for compliance on this once the feature is addded and fix the\nambiguous behavior in the tests now.\n\nThanks,\n-Santiago.\n\n---\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex b4698ab5f..0581053a0 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -896,17 +896,15 @@ test_expect_success GPG 'verifying a forged tag should fail' '\n '\n \n test_expect_success 'verifying a proper tag with --format pass and format accordingly' '\n-\tcat >expect <<-\\EOF\n+\tcat >expect <<-\\EOF &&\n \ttagname : signed-tag\n-\tEOF &&\n+\tEOF\n \tgit tag -v --format=\"tagname : %(tag)\" \"signed-tag\" >actual &&\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'verifying a forged tag with --format fail and format accordingly' '\n-\tcat >expect <<-\\EOF\n-\ttagname : forged-tag\n-\tEOF &&\n+test_expect_success 'verifying a forged tag with --format should fail silently' '\n+\t>expect &&\n \ttest_must_fail git tag -v --format=\"tagname : %(tag)\" \"forged-tag\" >actual &&\n \ttest_cmp expect actual\n '\ndiff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh\nindex d62ccbb98..173a88e89 100755\n--- a/t/t7030-verify-tag.sh\n+++ b/t/t7030-verify-tag.sh\n@@ -126,17 +126,15 @@ test_expect_success GPG 'verify multiple tags' '\n '\n \n test_expect_success 'verifying tag with --format' '\n-\tcat >expect <<-\\EOF\n+\tcat >expect <<-\\EOF &&\n \ttagname : fourth-signed\n-\tEOF &&\n+\tEOF\n \tgit verify-tag --format=\"tagname : %(tag)\" \"fourth-signed\" >actual &&\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'verifying a forged tag with --format fail and format accordingly' '\n-\tcat >expect <<-\\EOF\n-\ttagname : 7th forged-signed\n-\tEOF &&\n+test_expect_success 'verifying a forged tag with --format should fail silently' '\n+\t>expect &&\n \ttest_must_fail git verify-tag --format=\"tagname : %(tag)\" $(cat forged1.tag) >actual-forged &&\n \ttest_cmp expect actual-forged\n '\n"},{"id":"315035","messageId":"xmqq7f3ghhc7.fsf_-_@gitster.mtv.corp.google.com","threadId":"45468","inReplyTo":"20170322200805.23837-1-gitster@pobox.com","subject":"[PATCH] tests: lint for run-away here-doc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-23T02:12:08Z","receivedAt":"2017-03-23T02:12:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"We found a few run-away here documents that are started with an\nend-of-here-doc marker that is incorrectly spelled, e.g.\n\n\tgit some command >actual &&\n\tcat <expect\n\t...\n\tEOF &&\n\ttest_cmp expect actual\n\nwhich ends up slurping the entire remainder of the script as if it\nwere the data. Often the command that gets misused like this exits\nwithout failure (e.g. \"cat\" in the above example), which makes the\ncommand appear to work, without eve executing the remainder of the\ntest.\n\nUse a trick similar to the one used to catch the &&-chain breakage\nto detect this case.\n\nSigned-off-by: Junio C Hamano \n---\n\n * This catches all the cases detected in the recent discussion, I think.\n\n t/test-lib.sh | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 86d77c16dd..97bdc91f54 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -627,6 +627,10 @@ test_run_ () {\n \t\ttest_eval_ \"(exit 117) && $1\"\n \t\tif test \"$?\" != 117; then\n \t\t\terror \"bug in the test script: broken &&-chain: $1\"\n+\t\telif ! OK=$(test_eval_ \"false && $1${LF}${LF}echo OK\" 2>/dev/null) ||\n+\t\t test OK != \"$OK\"\n+\t\tthen\n+\t\t\terror \"bug in the test script: possibly unterminated HERE-DOC\"\n \t\tfi\n \t\ttrace=$trace_tmp\n \tfi\n"},{"id":"315041","messageId":"xmqqshm4fszt.fsf_-_@gitster.mtv.corp.google.com","threadId":"45468","inReplyTo":"xmqq7f3ghhc7.fsf_-_@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] tests: lint for run-away here-doc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-23T05:43:18Z","receivedAt":"2017-03-23T07:39:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"We found a few run-away here documents that are started with an\nend-of-here-doc marker that is incorrectly spelled, e.g.\n\n\tgit some command >actual &&\n\tcat <expect\n\t...\n\tEOF &&\n\ttest_cmp expect actual\n\nwhich ends up slurping the entire remainder of the script as if it\nwere the data. Often the command that gets misused like this exits\nwithout failure (e.g. \"cat\" in the above example), which makes the\ncommand appear to work, without eve executing the remainder of the\ntest.\n\nPiggy-back on the test that catches &&-chain breakage to detect this\ncase as well.\n\nSigned-off-by: Junio C Hamano \n---\n\n * The previous one was simply buggy; I forgot that there was an\n interesting redirection going on inside test_eval_. Sorry for\n the noise.\n\n Also we could do this in the same test_eval_ without adding\n another one, which is how this version does it.\n\n t/test-lib.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 86d77c16dd..d5f2b70bce 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -624,9 +624,9 @@ test_run_ () {\n \t\ttrace=\n \t\t# 117 is magic because it is unlikely to match the exit\n \t\t# code of other programs\n-\t\ttest_eval_ \"(exit 117) && $1\"\n-\t\tif test \"$?\" != 117; then\n-\t\t\terror \"bug in the test script: broken &&-chain: $1\"\n+\t\tif test \"OK-117\" != \"$(test_eval_ \"(exit 117) && $1${LF}${LF}echo OK-\\$?\" 3>&1)\"\n+\t\tthen\n+\t\t\terror \"bug in the test script: broken &&-chain or run-away HERE-DOC: $1\"\n \t\tfi\n \t\ttrace=$trace_tmp\n \tfi\n\n\n"},{"id":"315194","messageId":"xmqqfui3ac2f.fsf@gitster.mtv.corp.google.com","threadId":"45468","inReplyTo":"20170322225108.wub4bmr63hk2sp33@LykOS.localdomain","subject":"Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-23T22:00:08Z","receivedAt":"2017-03-23T22:00:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Santiago Torres writes:\n\n> This sounds like a helpful addition to implement. We could update/add\n> tests for compliance on this once the feature is addded and fix the\n> ambiguous behavior in the tests now.\n\nOK, so has everybody agreed what the next step would be? Is the\npatch below a good first step (I still need to get it signed off)?\n\n-- >8 --\nSubject: t7004, t7030: fix here-doc syntax errors\nFrom: Santiago Torres \n\nJan Palus noticed that some here-doc are spelled incorrectly,\nresulting the entire remainder of the test as if it were data\nslurped into the \"expect\" file, e.g. in this sequence\n\n\tcat >expect <actual &&\n\ttest_cmp expect actual\n\nthe last command of the test is \"cat\" that sends everything to\n'expect' and succeeds.\n\nFixing these issues in t7004 and t7030 reveals that \"git tag -v\"\nand \"git verify-tag\" with their --format option do not work as the\ntest was expecting originally. Instead of showing both valid tags\nand tags with incorrect signatures on their output, tags that do not\npass verification are omitted from the output.\n\nArguably, that is a safer behaviour, and because the format\nspecifiers like %(tag) do not have a way to show if the signature\nverifies correctly, the command with the --format option cannot be\nused to get a list of tags annotated with their signature validity\nanyway.\n\nFor now, let's fix the here-doc syntax and update the expectation to\nmatch the reality. Maybe later when we extend the --format language\navailable to \"git tag -v\" and \"git verify-tag\" to include things\nlike \"%(gpg:status)\", we may want to change the behaviour so that\npiping a list of tag names into\n\n xargs git verify-tag --format='%(gpg:status) %(tag)'\n\nbecomes a good way to produce such a list, but that is a separate\ntopic.\n\nNoticed-by: Jan Palus \nHelped-by: Jeff King \n---\n t/t7004-tag.sh | 10 ++++------\n t/t7030-verify-tag.sh | 10 ++++------\n 2 files changed, 8 insertions(+), 12 deletions(-)\n\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex b4698ab5f5..0581053a06 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -896,17 +896,15 @@ test_expect_success GPG 'verifying a forged tag should fail' '\n '\n \n test_expect_success 'verifying a proper tag with --format pass and format accordingly' '\n-\tcat >expect <<-\\EOF\n+\tcat >expect <<-\\EOF &&\n \ttagname : signed-tag\n-\tEOF &&\n+\tEOF\n \tgit tag -v --format=\"tagname : %(tag)\" \"signed-tag\" >actual &&\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'verifying a forged tag with --format fail and format accordingly' '\n-\tcat >expect <<-\\EOF\n-\ttagname : forged-tag\n-\tEOF &&\n+test_expect_success 'verifying a forged tag with --format should fail silently' '\n+\t>expect &&\n \ttest_must_fail git tag -v --format=\"tagname : %(tag)\" \"forged-tag\" >actual &&\n \ttest_cmp expect actual\n '\ndiff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh\nindex d62ccbb98e..79864a3411 100755\n--- a/t/t7030-verify-tag.sh\n+++ b/t/t7030-verify-tag.sh\n@@ -126,17 +126,15 @@ test_expect_success GPG 'verify multiple tags' '\n '\n \n test_expect_success 'verifying tag with --format' '\n-\tcat >expect <<-\\EOF\n+\tcat >expect <<-\\EOF &&\n \ttagname : fourth-signed\n-\tEOF &&\n+\tEOF\n \tgit verify-tag --format=\"tagname : %(tag)\" \"fourth-signed\" >actual &&\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'verifying a forged tag with --format fail and format accordingly' '\n-\tcat >expect <<-\\EOF\n-\ttagname : 7th forged-signed\n-\tEOF &&\n+test_expect_success 'verifying a forged tag with --format should fail silently' '\n+\t>expect &&\n \ttest_must_fail git verify-tag --format=\"tagname : %(tag)\" $(cat forged1.tag) >actual-forged &&\n \ttest_cmp expect actual-forged\n '\n\n\n"},{"id":"315198","messageId":"20170323222846.twxktig335o4r572@LykOS.localdomain","threadId":"45468","inReplyTo":"xmqqfui3ac2f.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors","fromName":"Santiago Torres","fromEmail":"santiago@nyu.edu","sentAt":"2017-03-23T22:28:47Z","receivedAt":"2017-03-23T22:28:56Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"On Thu, Mar 23, 2017 at 03:00:08PM -0700, Junio C Hamano wrote:\n> Santiago Torres writes:\n \n> OK, so has everybody agreed what the next step would be? \n\nI believe it is, although I imagine getting a confirmation from Peff\nwould be adequate.\n\n> Is the patch below a good first step (I still need to get it signed\n> off)?\n\nI'm adding a signoff to the patch below.\n\nThanks,\n-Santiago\n\n-- >8 --\nSubject: t7004, t7030: fix here-doc syntax errors\nFrom: Santiago Torres \n\nJan Palus noticed that some here-doc are spelled incorrectly,\nresulting the entire remainder of the test as if it were data\nslurped into the \"expect\" file, e.g. in this sequence\n\n\tcat >expect <actual &&\n\ttest_cmp expect actual\n\nthe last command of the test is \"cat\" that sends everything to\n'expect' and succeeds.\n\nFixing these issues in t7004 and t7030 reveals that \"git tag -v\"\nand \"git verify-tag\" with their --format option do not work as the\ntest was expecting originally. Instead of showing both valid tags\nand tags with incorrect signatures on their output, tags that do not\npass verification are omitted from the output.\n\nArguably, that is a safer behaviour, and because the format\nspecifiers like %(tag) do not have a way to show if the signature\nverifies correctly, the command with the --format option cannot be\nused to get a list of tags annotated with their signature validity\nanyway.\n\nFor now, let's fix the here-doc syntax and update the expectation to\nmatch the reality. Maybe later when we extend the --format language\navailable to \"git tag -v\" and \"git verify-tag\" to include things\nlike \"%(gpg:status)\", we may want to change the behaviour so that\npiping a list of tag names into\n\n xargs git verify-tag --format='%(gpg:status) %(tag)'\n\nbecomes a good way to produce such a list, but that is a separate\ntopic.\n\nSigned-off-by: Santiago Torres \nNoticed-by: Jan Palus \nHelped-by: Jeff King \n---\n t/t7004-tag.sh | 10 ++++------\n t/t7030-verify-tag.sh | 10 ++++------\n 2 files changed, 8 insertions(+), 12 deletions(-)\n\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex b4698ab5f5..0581053a06 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -896,17 +896,15 @@ test_expect_success GPG 'verifying a forged tag should fail' '\n '\n \n test_expect_success 'verifying a proper tag with --format pass and format accordingly' '\n-\tcat >expect <<-\\EOF\n+\tcat >expect <<-\\EOF &&\n \ttagname : signed-tag\n-\tEOF &&\n+\tEOF\n \tgit tag -v --format=\"tagname : %(tag)\" \"signed-tag\" >actual &&\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'verifying a forged tag with --format fail and format accordingly' '\n-\tcat >expect <<-\\EOF\n-\ttagname : forged-tag\n-\tEOF &&\n+test_expect_success 'verifying a forged tag with --format should fail silently' '\n+\t>expect &&\n \ttest_must_fail git tag -v --format=\"tagname : %(tag)\" \"forged-tag\" >actual &&\n \ttest_cmp expect actual\n '\ndiff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh\nindex d62ccbb98e..79864a3411 100755\n--- a/t/t7030-verify-tag.sh\n+++ b/t/t7030-verify-tag.sh\n@@ -126,17 +126,15 @@ test_expect_success GPG 'verify multiple tags' '\n '\n \n test_expect_success 'verifying tag with --format' '\n-\tcat >expect <<-\\EOF\n+\tcat >expect <<-\\EOF &&\n \ttagname : fourth-signed\n-\tEOF &&\n+\tEOF\n \tgit verify-tag --format=\"tagname : %(tag)\" \"fourth-signed\" >actual &&\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'verifying a forged tag with --format fail and format accordingly' '\n-\tcat >expect <<-\\EOF\n-\ttagname : 7th forged-signed\n-\tEOF &&\n+test_expect_success 'verifying a forged tag with --format should fail silently' '\n+\t>expect &&\n \ttest_must_fail git verify-tag --format=\"tagname : %(tag)\" $(cat forged1.tag) >actual-forged &&\n \ttest_cmp expect actual-forged\n '\n\n\n"},{"id":"315213","messageId":"20170323234922.ot2vqblcnljacdtn@sigill.intra.peff.net","threadId":"45468","inReplyTo":"xmqqfui3ac2f.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-23T23:49:22Z","receivedAt":"2017-03-23T23:49:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 23, 2017 at 03:00:08PM -0700, Junio C Hamano wrote:\n\n> Santiago Torres writes:\n> \n> > This sounds like a helpful addition to implement. We could update/add\n> > tests for compliance on this once the feature is addded and fix the\n> > ambiguous behavior in the tests now.\n> \n> OK, so has everybody agreed what the next step would be? Is the\n> patch below a good first step (I still need to get it signed off)?\n\nYeah, I think this is the right fix.\n\n> -- >8 --\n> Subject: t7004, t7030: fix here-doc syntax errors\n> From: Santiago Torres \n> \n> Jan Palus noticed that some here-doc are spelled incorrectly,\n> resulting the entire remainder of the test as if it were data\n> slurped into the \"expect\" file, e.g. in this sequence\n\nI had trouble parsing this. Perhaps:\n\n resulting in the entire remainder of the test snippet being slurped\n into the \"expect\" file as if it were data\n\n-Peff\n"},{"id":"315221","messageId":"20170324012912.GH20794@aiede.mtv.corp.google.com","threadId":"45468","inReplyTo":"xmqqshm4fszt.fsf_-_@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] tests: lint for run-away here-doc","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-03-24T01:29:12Z","receivedAt":"2017-03-24T01:29:22Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJunio C Hamano wrote:\n\n> We found a few run-away here documents that are started with an\n> end-of-here-doc marker that is incorrectly spelled, e.g.\n>\n> \tgit some command >actual &&\n> \tcat <expect\n> \t...\n> \tEOF &&\n> \ttest_cmp expect actual\n>\n> which ends up slurping the entire remainder of the script as if it\n> were the data. Often the command that gets misused like this exits\n> without failure (e.g. \"cat\" in the above example), which makes the\n> command appear to work, without eve executing the remainder of the\n\ns/eve/ever/\n\n> test.\n>\n> Piggy-back on the test that catches &&-chain breakage to detect this\n> case as well.\n>\n> Signed-off-by: Junio C Hamano \n> ---\n\nI also wonder if we can use an \"sh -n\" style syntax check some day,\nbut that's likely harder.\n\n[...]\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -624,9 +624,9 @@ test_run_ () {\n> \t\ttrace=\n> \t\t# 117 is magic because it is unlikely to match the exit\n> \t\t# code of other programs\n> -\t\ttest_eval_ \"(exit 117) && $1\"\n> -\t\tif test \"$?\" != 117; then\n> -\t\t\terror \"bug in the test script: broken &&-chain: $1\"\n> +\t\tif test \"OK-117\" != \"$(test_eval_ \"(exit 117) && $1${LF}${LF}echo OK-\\$?\" 3>&1)\"\n> +\t\tthen\n> +\t\t\terror \"bug in the test script: broken &&-chain or run-away HERE-DOC: $1\"\n> \t\tfi\n\nNeat. Why the double-LF?\n\nIn some shells, the 3>&1 will last past the function call.\nFortunately, the $() substitution creates a subshell so this doesn't\naffect anything later on.\n\ntest_eval_inner_ contains a warning not to append anything after the\ncommands to be evaluated, since whatever you append would pollute -x\ntracing output. Fortunately, in this context we have already set\ntrace= so the warning does not apply.\n\nReviewed-by: Jonathan Nieder \n\nThanks.\n"},{"id":"315222","messageId":"xmqq37e39yv7.fsf@gitster.mtv.corp.google.com","threadId":"45468","inReplyTo":"20170324012912.GH20794@aiede.mtv.corp.google.com","subject":"Re: [PATCH v2] tests: lint for run-away here-doc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-24T02:45:16Z","receivedAt":"2017-03-24T02:45:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n>> without failure (e.g. \"cat\" in the above example), which makes the\n>> command appear to work, without eve executing the remainder of the\n>\n> s/eve/ever/\n\nOops.\n\n>> +\t\tif test \"OK-117\" != \"$(test_eval_ \"(exit 117) && $1${LF}${LF}echo OK-\\$?\" 3>&1)\"\n>> +\t\tthen\n>> +\t\t\terror \"bug in the test script: broken &&-chain or run-away HERE-DOC: $1\"\n>> \t\tfi\n>\n> Neat. Why the double-LF?\n\nThe main part of \"Neat\" was your invention ;-).\n\nImagining how the string passed to eval looked, having two LFs was\nthe easiest way to ensure that there is a blank line before the new\n\"echo\" (not just the last line in $1 and \"echo\" are on different\nlines), which was more visually pleasing. There was no any real\nfunctional requirement.\n\n> In some shells, the 3>&1 will last past the function call.\n> Fortunately, the $() substitution creates a subshell so this doesn't\n> affect anything later on.\n\nYes, a subshell solves quite lot of problems (while possibly\nintroducing others, though ;-).\n\n> test_eval_inner_ contains a warning not to append anything after the\n> commands to be evaluated, since whatever you append would pollute -x\n> tracing output. Fortunately, in this context we have already set\n> trace= so the warning does not apply.\n>\n>\n> Reviewed-by: Jonathan Nieder \n\nThanks.\n"},{"id":"315223","messageId":"20170324035954.aejqfadvxms2qfsf@sigill.intra.peff.net","threadId":"45468","inReplyTo":"xmqqshm4fszt.fsf_-_@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] tests: lint for run-away here-doc","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-24T03:59:54Z","receivedAt":"2017-03-24T04:00:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 22, 2017 at 10:43:18PM -0700, Junio C Hamano wrote:\n\n> -\t\ttest_eval_ \"(exit 117) && $1\"\n> -\t\tif test \"$?\" != 117; then\n> -\t\t\terror \"bug in the test script: broken &&-chain: $1\"\n> +\t\tif test \"OK-117\" != \"$(test_eval_ \"(exit 117) && $1${LF}${LF}echo OK-\\$?\" 3>&1)\"\n> +\t\tthen\n> +\t\t\terror \"bug in the test script: broken &&-chain or run-away HERE-DOC: $1\"\n\nThis looks good. I had pondered how we might do this but was worried\nthat it would have to end up actually executing the test contents. But\nthis leverages the fact that the problem is syntactic and that the shell\nwill parse the complete &&-chain before executing any of it. Clever.\n\n-Peff\n"},{"id":"315271","messageId":"xmqqlgru8vyt.fsf@gitster.mtv.corp.google.com","threadId":"45468","inReplyTo":"20170323234922.ot2vqblcnljacdtn@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-24T16:45:30Z","receivedAt":"2017-03-24T16:45:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Mar 23, 2017 at 03:00:08PM -0700, Junio C Hamano wrote:\n>\n>> Santiago Torres writes:\n>> \n>> > This sounds like a helpful addition to implement. We could update/add\n>> > tests for compliance on this once the feature is addded and fix the\n>> > ambiguous behavior in the tests now.\n>> \n>> OK, so has everybody agreed what the next step would be? Is the\n>> patch below a good first step (I still need to get it signed off)?\n>\n> Yeah, I think this is the right fix.\n>\n>> -- >8 --\n>> Subject: t7004, t7030: fix here-doc syntax errors\n>> From: Santiago Torres \n>> \n>> Jan Palus noticed that some here-doc are spelled incorrectly,\n>> resulting the entire remainder of the test as if it were data\n>> slurped into the \"expect\" file, e.g. in this sequence\n>\n> I had trouble parsing this. Perhaps:\n>\n> resulting in the entire remainder of the test snippet being slurped\n> into the \"expect\" file as if it were data\n\nThanks. Will rephrase.\n\nI actually think this uncovers another class of breakage. t7030\ntests should be protected with GPG prereq and 'fourth-signed' that\nis made only with the prereq in the first test will not be found.\nt7004 probably has the same issue.\n\n\n\n"},{"id":"315272","messageId":"20170324164943.7vvtcj5sqadj625o@sigill.intra.peff.net","threadId":"45468","inReplyTo":"xmqqlgru8vyt.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-24T16:49:43Z","receivedAt":"2017-03-24T16:50:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 24, 2017 at 09:45:30AM -0700, Junio C Hamano wrote:\n\n> I actually think this uncovers another class of breakage. t7030\n> tests should be protected with GPG prereq and 'fourth-signed' that\n> is made only with the prereq in the first test will not be found.\n\nIt seems like t7030 should just skip_all when the GPG prereq is not\nmet (it's not wrong to mark each test that's added, but it would have\nmade this particular mistake harder).\n\n> t7004 probably has the same issue.\n\nThese ones should be marked individually, though.\n\n-Peff\n"},{"id":"315290","messageId":"xmqqvaqy7dqs.fsf@gitster.mtv.corp.google.com","threadId":"45468","inReplyTo":"20170324164943.7vvtcj5sqadj625o@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-24T18:04:27Z","receivedAt":"2017-03-24T18:05:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> It seems like t7030 should just skip_all when the GPG prereq is not\n> met (it's not wrong to mark each test that's added, but it would have\n> made this particular mistake harder).\n\nI'd leave that to be done by others after the dust settles ;-). \n\nHere is what I have right now (proposed log message has updates to\nmatch rather obvious changes to the tests).\n\n-- >8 --\nFrom: Santiago Torres \nDate: Thu, 23 Mar 2017 18:28:47 -0400\nSubject: [PATCH] t7004, t7030: fix here-doc syntax errors\n\nJan Palus noticed that some here-doc are spelled incorrectly,\nresulting the entire remainder of the test snippet being slurped\ninto the \"expect\" file as if it were data, e.g. in this sequence\n\n\tcat >expect <actual &&\n\ttest_cmp expect actual\n\nthe last command of the test is \"cat\" that sends everything to\n'expect' and succeeds.\n\nFixing these issues in t7004 and t7030 reveals that \"git tag -v\"\nand \"git verify-tag\" with their --format option do not work as the\ntest was expecting originally. Instead of showing both valid tags\nand tags with incorrect signatures on their output, tags that do not\npass verification are omitted from the output. Another breakage that\nis uncovered is that these tests must be restricted to environment\nwhere gpg is available.\n\nArguably, that is a safer behaviour, and because the format\nspecifiers like %(tag) do not have a way to show if the signature\nverifies correctly, the command with the --format option cannot be\nused to get a list of tags annotated with their signature validity\nanyway.\n\nFor now, let's fix the here-doc syntax, update the expectation to\nmatch the reality, and update the test prerequisite.\n\nMaybe later when we extend the --format language available to \"git\ntag -v\" and \"git verify-tag\" to include things like \"%(gpg:status)\",\nwe may want to change the behaviour so that piping a list of tag\nnames into\n\n xargs git verify-tag --format='%(gpg:status) %(tag)'\n\nbecomes a good way to produce such a list, but that is a separate\ntopic.\n\nNoticed-by: Jan Palus \nHelped-by: Jeff King \nSigned-off-by: Santiago Torres \nSigned-off-by: Junio C Hamano \n---\n t/t7004-tag.sh | 12 +++++-------\n t/t7030-verify-tag.sh | 12 +++++-------\n 2 files changed, 10 insertions(+), 14 deletions(-)\n\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex b53a2e5e41..f67bbb8abc 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -847,18 +847,16 @@ test_expect_success GPG 'verifying a forged tag should fail' '\n \ttest_must_fail git tag -v forged-tag\n '\n \n-test_expect_success 'verifying a proper tag with --format pass and format accordingly' '\n-\tcat >expect <<-\\EOF\n+test_expect_success GPG 'verifying a proper tag with --format pass and format accordingly' '\n+\tcat >expect <<-\\EOF &&\n \ttagname : signed-tag\n-\tEOF &&\n+\tEOF\n \tgit tag -v --format=\"tagname : %(tag)\" \"signed-tag\" >actual &&\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'verifying a forged tag with --format fail and format accordingly' '\n-\tcat >expect <<-\\EOF\n-\ttagname : forged-tag\n-\tEOF &&\n+test_expect_success GPG 'verifying a forged tag with --format should fail silently' '\n+\t>expect &&\n \ttest_must_fail git tag -v --format=\"tagname : %(tag)\" \"forged-tag\" >actual &&\n \ttest_cmp expect actual\n '\ndiff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh\nindex d62ccbb98e..b4b49eeb08 100755\n--- a/t/t7030-verify-tag.sh\n+++ b/t/t7030-verify-tag.sh\n@@ -125,18 +125,16 @@ test_expect_success GPG 'verify multiple tags' '\n \ttest_cmp expect.stderr actual.stderr\n '\n \n-test_expect_success 'verifying tag with --format' '\n-\tcat >expect <<-\\EOF\n+test_expect_success GPG 'verifying tag with --format' '\n+\tcat >expect <<-\\EOF &&\n \ttagname : fourth-signed\n-\tEOF &&\n+\tEOF\n \tgit verify-tag --format=\"tagname : %(tag)\" \"fourth-signed\" >actual &&\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'verifying a forged tag with --format fail and format accordingly' '\n-\tcat >expect <<-\\EOF\n-\ttagname : 7th forged-signed\n-\tEOF &&\n+test_expect_success GPG 'verifying a forged tag with --format should fail silently' '\n+\t>expect &&\n \ttest_must_fail git verify-tag --format=\"tagname : %(tag)\" $(cat forged1.tag) >actual-forged &&\n \ttest_cmp expect actual-forged\n '\n-- \n2.12.1-432-gf364f02724\n\n"},{"id":"315291","messageId":"20170324180025.nlxcrgj4c7dg2sxj@sigill.intra.peff.net","threadId":"45468","inReplyTo":"20170324164943.7vvtcj5sqadj625o@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-24T18:00:26Z","receivedAt":"2017-03-24T18:10:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 24, 2017 at 12:49:43PM -0400, Jeff King wrote:\n\n> On Fri, Mar 24, 2017 at 09:45:30AM -0700, Junio C Hamano wrote:\n> \n> > I actually think this uncovers another class of breakage. t7030\n> > tests should be protected with GPG prereq and 'fourth-signed' that\n> > is made only with the prereq in the first test will not be found.\n> \n> It seems like t7030 should just skip_all when the GPG prereq is not\n> met (it's not wrong to mark each test that's added, but it would have\n> made this particular mistake harder).\n> \n> > t7004 probably has the same issue.\n> \n> These ones should be marked individually, though.\n\nI started to prepare a patch for t7030, but given that we want to do\nthis all in one patch anyway (for bisectability), I think it probably\nmakes sense to just add the missing GPG prereqs as part of the patch you\nposted. If somebody wants to convert t7030 to skip_all on top that's\nfine, but it's not that big a deal.\n\n-Peff\n"},{"id":"315292","messageId":"20170324181613.beivhhmfug324dc4@sigill.intra.peff.net","threadId":"45468","inReplyTo":"xmqqvaqy7dqs.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-24T18:16:13Z","receivedAt":"2017-03-24T18:16:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 24, 2017 at 11:04:27AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > It seems like t7030 should just skip_all when the GPG prereq is not\n> > met (it's not wrong to mark each test that's added, but it would have\n> > made this particular mistake harder).\n> \n> I'd leave that to be done by others after the dust settles ;-).\n> \n> Here is what I have right now (proposed log message has updates to\n> match rather obvious changes to the tests).\n\nThanks, this looks good to me.\n\n-Peff\n"}]}