EOF test fixes (t5615/t7004)

EOF test fixes (t5615/t7004)

[Jan Palus]

[-- Attachment #1: Type: text/plain, Size: 943 bytes --]

Hi,

attached patch fixes 2 out of 3 tests failing in my env -- missing EOF,
incorrectly placed "&&" after EOF. One test seems to be incorrect as it
fails even after fixing. I suspect the main difference between my env and
others is in shell used -- mksh in my case and probably bash in others.
Looks like bash has a nasty behavior when it encounters syntax error:

$ cat test.sh
cat >/dev/null <<-\EOF
tagname : forged-tag
EOF &&
echo foo

$ bash test.sh && echo success || echo failed
test.sh: line 4: warning: here-document at line 1 delimited by
end-of-file (wanted `EOF')
success

# notice no "foo" printed

$ mksh test.sh && echo success || echo failed    
test.sh[5]: here document 'EOF' unclosed
failed

Test that fails even after fixing EOFs:
t7004-tag.sh:verifying a forged tag with --format fail and format accordingly

Note that I'm not subscribed to mailing list so in case of any questions
please mail me directly.


Regards
Jan

[-- Attachment #2: git-core-tests.patch --]
[-- Type: text/plain, Size: 1197 bytes --]

diff -urN git-2.11.1.orig/t/t5615-alternate-env.sh git-2.11.1/t/t5615-alternate-env.sh
--- git-2.11.1.orig/t/t5615-alternate-env.sh	2017-02-03 02:24:45.115143042 +0100
+++ git-2.11.1/t/t5615-alternate-env.sh	2017-02-03 02:24:58.081809318 +0100
@@ -77,6 +77,7 @@
 	check_obj "$quoted:$unquoted" <<-EOF
 	$one blob
 	$two blob
+	EOF
 '
 
 test_expect_success !MINGW 'broken quoting falls back to interpreting raw' '
diff -ur git-2.12.0.orig/t/t7004-tag.sh git-2.12.0/t/t7004-tag.sh
--- git-2.12.0.orig/t/t7004-tag.sh	2017-02-25 14:10:53.059367179 +0100
+++ git-2.12.0/t/t7004-tag.sh	2017-02-25 14:11:45.105827700 +0100
@@ -880,17 +880,17 @@
 '
 
 test_expect_success 'verifying a proper tag with --format pass and format accordingly' '
-	cat >expect <<-\EOF
+	cat >expect <<-\EOF &&
 	tagname : signed-tag
-	EOF &&
+	EOF
 	git tag -v --format="tagname : %(tag)" "signed-tag" >actual &&
 	test_cmp expect actual
 '
 
 test_expect_success 'verifying a forged tag with --format fail and format accordingly' '
-	cat >expect <<-\EOF
+	cat >expect <<-\EOF &&
 	tagname : forged-tag
-	EOF &&
+	EOF
 	test_must_fail git tag -v --format="tagname : %(tag)" "forged-tag" >actual &&
 	test_cmp expect actual
 '

Re: EOF test fixes (t7030/t7406)

[Jan Palus]

[-- Attachment #1: Type: text/plain, Size: 296 bytes --]

It appears more tests are affected:

$ grep -r '^[[:space:]]*EOF &&' .   
./t7406-submodule-update.sh:	EOF &&
./t7030-verify-tag.sh:	EOF &&
./t7030-verify-tag.sh:	EOF &&
./t7004-tag.sh:	EOF &&
./t7004-tag.sh:	EOF &&

attaching patch for t7406 and t7030 which both fail even after fix is
applied.

[-- Attachment #2: git-EOF-t7030-t7406.patch --]
[-- Type: text/plain, Size: 1454 bytes --]

diff -ur git-2.12.1.orig/t/t7030-verify-tag.sh git-2.12.1/t/t7030-verify-tag.sh
--- git-2.12.1.orig/t/t7030-verify-tag.sh	2017-03-22 19:20:49.614759549 +0100
+++ git-2.12.1/t/t7030-verify-tag.sh	2017-03-22 19:26:27.608511234 +0100
@@ -126,17 +126,17 @@
 '
 
 test_expect_success 'verifying tag with --format' '
-	cat >expect <<-\EOF
+	cat >expect <<-\EOF &&
 	tagname : fourth-signed
-	EOF &&
+	EOF
 	git verify-tag --format="tagname : %(tag)" "fourth-signed" >actual &&
 	test_cmp expect actual
 '
 
 test_expect_success 'verifying a forged tag with --format fail and format accordingly' '
-	cat >expect <<-\EOF
+	cat >expect <<-\EOF &&
 	tagname : 7th forged-signed
-	EOF &&
+	EOF
 	test_must_fail git verify-tag --format="tagname : %(tag)" $(cat forged1.tag) >actual-forged &&
 	test_cmp expect actual-forged
 '
diff -ur git-2.12.1.orig/t/t7406-submodule-update.sh git-2.12.1/t/t7406-submodule-update.sh
--- git-2.12.1.orig/t/t7406-submodule-update.sh	2017-03-22 19:20:49.614759549 +0100
+++ git-2.12.1/t/t7406-submodule-update.sh	2017-03-22 19:25:34.105528379 +0100
@@ -442,9 +442,9 @@
 '
 
 test_expect_success 'submodule update - command run for initial population of submodule' '
-	cat <<-\ EOF >expect
+	cat >expect <<-\EOF &&
 	Execution of '\''false $submodulesha1'\'' failed in submodule path '\''submodule'\''
-	EOF &&
+	EOF
 	rm -rf super/submodule &&
 	test_must_fail git -C super submodule update >../actual &&
 	test_cmp expect actual &&

Re: EOF test fixes (t5615/t7004)

[Stefan Beller]

On Wed, Mar 22, 2017 at 10:35 AM, Jan Palus <jan.palus@gmail.com> wrote:
> Hi,
>
> attached patch fixes 2 out of 3 tests failing in my env -- missing EOF,
> incorrectly placed "&&" after EOF. One test seems to be incorrect as it
> fails even after fixing. I suspect the main difference between my env and
> others is in shell used -- mksh in my case and probably bash in others.
> Looks like bash has a nasty behavior when it encounters syntax error:
>
> $ cat test.sh
> cat >/dev/null <<-\EOF
> tagname : forged-tag
> EOF &&
> echo foo
>
> $ bash test.sh && echo success || echo failed
> test.sh: line 4: warning: here-document at line 1 delimited by
> end-of-file (wanted `EOF')
> success
>
> # notice no "foo" printed
>
> $ mksh test.sh && echo success || echo failed
> test.sh[5]: here document 'EOF' unclosed
> failed
>
> Test that fails even after fixing EOFs:
> t7004-tag.sh:verifying a forged tag with --format fail and format accordingly
>
> Note that I'm not subscribed to mailing list so in case of any questions
> please mail me directly.

Thanks for catching these bugs!

Please have a look at Documentation/SubmittingPatches.
(the most important thing is the "Sign-off-by" line indicating you
are legally permitted to send such a patch;
for one-off patches the format can be negotiated, but it is easier
for maintainers to take the proper format.)

This email conveys the actual problem very well, so only a little
change is needed to make it a proper commit message.
(c.f. git clone git://git.kernel.org/pub/scm/git/git.git/ &&
git -C git log)

Thanks,
Stefan

Re: EOF test fixes (t5615/t7004)

[Junio C Hamano]

Jan Palus <jan.palus@gmail.com> writes:

> diff -ur git-2.12.0.orig/t/t7004-tag.sh git-2.12.0/t/t7004-tag.sh
> --- git-2.12.0.orig/t/t7004-tag.sh	2017-02-25 14:10:53.059367179 +0100
> +++ git-2.12.0/t/t7004-tag.sh	2017-02-25 14:11:45.105827700 +0100
> @@ -880,17 +880,17 @@
>  '
>  
>  test_expect_success 'verifying a proper tag with --format pass and format accordingly' '
> -	cat >expect <<-\EOF
> +	cat >expect <<-\EOF &&
>  	tagname : signed-tag
> -	EOF &&
> +	EOF
>  	git tag -v --format="tagname : %(tag)" "signed-tag" >actual &&
>  	test_cmp expect actual
>  '
>  
>  test_expect_success 'verifying a forged tag with --format fail and format accordingly' '
> -	cat >expect <<-\EOF
> +	cat >expect <<-\EOF &&
>  	tagname : forged-tag
> -	EOF &&
> +	EOF
>  	test_must_fail git tag -v --format="tagname : %(tag)" "forged-tag" >actual &&
>  	test_cmp expect actual
>  '

Ouch.  These shouldn't even have passed our review.

Thanks for spotting.

[PATCH 0/3] fix "here-doc" syntax errors

[Junio C Hamano]

Because I'd prefer to be able to queue fixes on individual topics
that introduced the breakages, I splitted the fixes in your two
messages into three patches.

Please respond to the list, saying it is OK to add your "sign-off"
(see Documentation/SubmittingPatches) to these patches.

Thanks.

Jan Palus (3):
  t5615: fix a here-doc syntax error
  t7406: fix here-doc syntax errors
  t7004, t7030: fix here-doc syntax errors

 t/t5615-alternate-env.sh    | 1 +
 t/t7004-tag.sh              | 8 ++++----
 t/t7030-verify-tag.sh       | 8 ++++----
 t/t7406-submodule-update.sh | 4 ++--
 4 files changed, 11 insertions(+), 10 deletions(-)

-- 
2.12.1-430-gafd6726309

[PATCH 1/3] t5615: fix a here-doc syntax error

[Junio C Hamano]

From: Jan Palus <jan.palus@gmail.com>

This came as part of jk/quote-env-path-list-component and was merged
to 2.11.1 and later.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * This should be applied on top of 5e74824f ("t5615-alternate-env:
 double-quotes in file names do not work on Windows", 2016-12-21)

 t/t5615-alternate-env.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/t5615-alternate-env.sh b/t/t5615-alternate-env.sh
index 26ebb0375d..d2d883f3a1 100755
--- a/t/t5615-alternate-env.sh
+++ b/t/t5615-alternate-env.sh
@@ -77,6 +77,7 @@ test_expect_success 'mix of quoted and unquoted alternates' '
 	check_obj "$quoted:$unquoted" <<-EOF
 	$one blob
 	$two blob
+	EOF
 '
 
 test_expect_success !MINGW 'broken quoting falls back to interpreting raw' '
-- 
2.12.1-430-gafd6726309

Re: [PATCH 1/3] t5615: fix a here-doc syntax error

[Jeff King]

On Wed, Mar 22, 2017 at 01:08:03PM -0700, Junio C Hamano wrote:

> From: Jan Palus <jan.palus@gmail.com>
> 
> This came as part of jk/quote-env-path-list-component and was merged
> to 2.11.1 and later.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> 
>  * This should be applied on top of 5e74824f ("t5615-alternate-env:
>  double-quotes in file names do not work on Windows", 2016-12-21)
> 
>  t/t5615-alternate-env.sh | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/t/t5615-alternate-env.sh b/t/t5615-alternate-env.sh
> index 26ebb0375d..d2d883f3a1 100755
> --- a/t/t5615-alternate-env.sh
> +++ b/t/t5615-alternate-env.sh
> @@ -77,6 +77,7 @@ test_expect_success 'mix of quoted and unquoted alternates' '
>  	check_obj "$quoted:$unquoted" <<-EOF
>  	$one blob
>  	$two blob
> +	EOF
>  '

Thanks, this one is my fault, and the solution is obviously correct.

-Peff

[PATCH 2/3] t7406: fix here-doc syntax errors

[Junio C Hamano]

From: Jan Palus <jan.palus@gmail.com>

The came in an earlier sb/submodule-update-initial-runs-custom-script
topic that was merged to 2.12.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * This should be applied on top of e7b37caf ("submodule update: run
   custom update script for initial populating as well", 2017-01-25)

 t/t7406-submodule-update.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 347857fa7c..a20df9420a 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -442,9 +442,9 @@ test_expect_success 'submodule update - command in .git/config catches failure -
 '
 
 test_expect_success 'submodule update - command run for initial population of submodule' '
-	cat <<-\ EOF >expect
+	cat >expect <<-\EOF &&
 	Execution of '\''false $submodulesha1'\'' failed in submodule path '\''submodule'\''
-	EOF &&
+	EOF
 	rm -rf super/submodule &&
 	test_must_fail git -C super submodule update >../actual &&
 	test_cmp expect actual &&
-- 
2.12.1-430-gafd6726309

Re: [PATCH 2/3] t7406: fix here-doc syntax errors

[Jeff King]

On Wed, Mar 22, 2017 at 01:08:04PM -0700, Junio C Hamano wrote:

> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
> index 347857fa7c..a20df9420a 100755
> --- a/t/t7406-submodule-update.sh
> +++ b/t/t7406-submodule-update.sh
> @@ -442,9 +442,9 @@ test_expect_success 'submodule update - command in .git/config catches failure -
>  '
>  
>  test_expect_success 'submodule update - command run for initial population of submodule' '
> -	cat <<-\ EOF >expect
> +	cat >expect <<-\EOF &&
>  	Execution of '\''false $submodulesha1'\'' failed in submodule path '\''submodule'\''
> -	EOF &&
> +	EOF

After applying this, I get a failure:

  --- expect	2017-03-22 21:01:53.350721155 +0000
  +++ actual	2017-03-22 21:01:53.346721155 +0000
  @@ -1 +1 @@
  -Execution of 'false $submodulesha1' failed in submodule path 'submodule'
  +Execution of 'false 4301fd3e4110d3b6212c19aed3094150392545b9' failed in submodule path '../submodule'

At the very least, we need to drop the "\" from EOF to expand
$submodulesha1. But the submodule path seems wrong, too. I'm not sure if
the expectation is wrong, or if there's a bug. +cc Stefan

-Peff

Re: [PATCH 2/3] t7406: fix here-doc syntax errors

[Stefan Beller]

On Wed, Mar 22, 2017 at 2:07 PM, Jeff King <peff@peff.net> wrote:
> On Wed, Mar 22, 2017 at 01:08:04PM -0700, Junio C Hamano wrote:
>
>> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
>> index 347857fa7c..a20df9420a 100755
>> --- a/t/t7406-submodule-update.sh
>> +++ b/t/t7406-submodule-update.sh
>> @@ -442,9 +442,9 @@ test_expect_success 'submodule update - command in .git/config catches failure -
>>  '
>>
>>  test_expect_success 'submodule update - command run for initial population of submodule' '
>> -     cat <<-\ EOF >expect
>> +     cat >expect <<-\EOF &&
>>       Execution of '\''false $submodulesha1'\'' failed in submodule path '\''submodule'\''
>> -     EOF &&
>> +     EOF
>
> After applying this, I get a failure:
>
>   --- expect    2017-03-22 21:01:53.350721155 +0000
>   +++ actual    2017-03-22 21:01:53.346721155 +0000
>   @@ -1 +1 @@
>   -Execution of 'false $submodulesha1' failed in submodule path 'submodule'
>   +Execution of 'false 4301fd3e4110d3b6212c19aed3094150392545b9' failed in submodule path '../submodule'
>
> At the very least, we need to drop the "\" from EOF to expand
> $submodulesha1.

yes.

> But the submodule path seems wrong, too. I'm not sure if
> the expectation is wrong, or if there's a bug. +cc Stefan

Yes the actual output is wrong, too.
But that is not because Git is wrong, but the test suite is.

    test_must_fail git -C super submodule update >../actual &&

contains 2 errors:

* we are interested in stderr, so make it 2>
* the -C switch doesn't apply to redirection. (obviously!)
  So if you have run the test suite in a normal setup, you may have
  a file "t/actual" which is empty. This is scary as it managed to break
  out of the test suite and overwrote a potential file in your git.git.

I'll send a patch on top of the one under discussion momentarily.

Thanks,
Stefan

Re: [PATCH 2/3] t7406: fix here-doc syntax errors

[Jeff King]

On Wed, Mar 22, 2017 at 02:32:22PM -0700, Stefan Beller wrote:

> > But the submodule path seems wrong, too. I'm not sure if
> > the expectation is wrong, or if there's a bug. +cc Stefan
> 
> Yes the actual output is wrong, too.
> But that is not because Git is wrong, but the test suite is.
> 
>     test_must_fail git -C super submodule update >../actual &&
> 
> contains 2 errors:
> 
> * we are interested in stderr, so make it 2>
> * the -C switch doesn't apply to redirection. (obviously!)
>   So if you have run the test suite in a normal setup, you may have
>   a file "t/actual" which is empty. This is scary as it managed to break
>   out of the test suite and overwrote a potential file in your git.git.

Oh yeah, I didn't even look at the test itself, but I see I now have
t/actual in my repository. Yikes. That didn't happen before because it
simply sucked the entire command into the here document.

> I'll send a patch on top of the one under discussion momentarily.

I'd prefer if they come in a single patch so that we don't break bisect.

-Peff

[PATCH] t7406: correct test case for submodule-update initial population

[Stefan Beller]

There are three issues with the test:

* The syntax of the here-doc was wrong, such that the entire test was
  sucked into the here-doc, which is why the test succeeded successfully.

* The variable $submodulesha1 was not expanded as it was inside a single
  quoted string. Use double quote to expand the variable.

* The redirection from the git command to the output file for comparison
  was wrong as the -C operator from git doesn't apply to the redirect path.
  Also we're interested in stderr of that command.

Noticed-by: Jan Palus <jan.palus@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---

This is just one patch (for bisectability)
it applies on e7b37caf4fe.

Thanks,
Stefan

 t/t7406-submodule-update.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 8c086a4..c327eb6 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -425,11 +425,11 @@ test_expect_success 'submodule update - command in .git/config catches failure -
 '
 
 test_expect_success 'submodule update - command run for initial population of submodule' '
-	cat <<-\ EOF >expect
-	Execution of '\''false $submodulesha1'\'' failed in submodule path '\''submodule'\''
-	EOF &&
+	cat >expect <<-\EOF &&
+	Execution of '\'"false $submodulesha1"\'' failed in submodule path '\''submodule'\''
+	EOF
 	rm -rf super/submodule &&
-	test_must_fail git -C super submodule update >../actual &&
+	test_must_fail git -C super submodule update 2>actual &&
 	test_cmp expect actual &&
 	git -C super submodule update --checkout
 '
-- 
2.10.2.50.g9d09a6e.dirty

Re: [PATCH] t7406: correct test case for submodule-update initial population

[Jeff King]

On Wed, Mar 22, 2017 at 02:49:48PM -0700, Stefan Beller wrote:

> * The syntax of the here-doc was wrong, such that the entire test was
>   sucked into the here-doc, which is why the test succeeded successfully.

As opposed to succeeding unsuccessfully? :)

> * The variable $submodulesha1 was not expanded as it was inside a single
>   quoted string. Use double quote to expand the variable.

Hmm. Sort of. It was inside a non-interpolating here-doc inside a
single-quoted string which was being eval'd. The second half is fine
(the eval adds an extra layer of evaluation).

Your fix:

> +	cat >expect <<-\EOF &&
> +	Execution of '\'"false $submodulesha1"\'' failed in submodule path '\''submodule'\''
> +	EOF

_does_ work, but it does so because it's evaluating $submodulesha1 in
the shell snippet and handing the result off to test_expect_success to
eval. So it would have problems if:

  - that variable contained "\nEOF\n" itself ;)

  - the variable was modified inside the shell snippet.

Neither of those is true, but I think:

  cat >expect <<-EOF &&
  Execution of '\''false $submodulesha1'\'' failed in ...
  EOF

is safer and less surprising. The single-quote handling is unfortunate and
ugly, but necessary to get them into the shell snippet in the first
place. I notice the others tests in this script set up the expect file
outside of a block. You could also do something like:

  sq=\'
  test_expect_success '...' '
	cat >expect <<-EOF
	Execution of ${sq}false $submodulesha1${sq} ...
  '

but I'm not sure if that is any more readable.

-Peff

Re: [PATCH] t7406: correct test case for submodule-update initial population

[Stefan Beller]

On Wed, Mar 22, 2017 at 2:59 PM, Jeff King <peff@peff.net> wrote:
> On Wed, Mar 22, 2017 at 02:49:48PM -0700, Stefan Beller wrote:
>
>> * The syntax of the here-doc was wrong, such that the entire test was
>>   sucked into the here-doc, which is why the test succeeded successfully.
>
> As opposed to succeeding unsuccessfully? :)
>
>> * The variable $submodulesha1 was not expanded as it was inside a single
>>   quoted string. Use double quote to expand the variable.
>
> Hmm. Sort of. It was inside a non-interpolating here-doc inside a
> single-quoted string which was being eval'd. The second half is fine
> (the eval adds an extra layer of evaluation).
>
> Your fix:
>
>> +     cat >expect <<-\EOF &&
>> +     Execution of '\'"false $submodulesha1"\'' failed in submodule path '\''submodule'\''
>> +     EOF
>
> _does_ work, but it does so because it's evaluating $submodulesha1 in
> the shell snippet and handing the result off to test_expect_success to
> eval. So it would have problems if:
>
>   - that variable contained "\nEOF\n" itself ;)
>
>   - the variable was modified inside the shell snippet.
>
> Neither of those is true, but I think:
>
>   cat >expect <<-EOF &&
>   Execution of '\''false $submodulesha1'\'' failed in ...
>   EOF
>
> is safer and less surprising. The single-quote handling is unfortunate and
> ugly, but necessary to get them into the shell snippet in the first
> place. I notice the others tests in this script set up the expect file
> outside of a block. You could also do something like:
>
>   sq=\'
>   test_expect_success '...' '
>         cat >expect <<-EOF
>         Execution of ${sq}false $submodulesha1${sq} ...
>   '
>
> but I'm not sure if that is any more readable.
>

If I recall correctly, I made a big fuss about single quotes used correctly when
writing that patch (which is why I may have lost track of the actual work there)
to be told the one and only blessed way to use single quotes in our test suite.

Your proposal to use ${sq} sounds good to me, though we did not
follow through with it for some other reason. I can reroll with that, though.

Thanks,
Stefan

Re: [PATCH] t7406: correct test case for submodule-update initial population

[Jeff King]

On Wed, Mar 22, 2017 at 03:07:45PM -0700, Stefan Beller wrote:

> > Neither of those is true, but I think:
> >
> >   cat >expect <<-EOF &&
> >   Execution of '\''false $submodulesha1'\'' failed in ...
> >   EOF
> >
> > is safer and less surprising. The single-quote handling is unfortunate and
> > ugly, but necessary to get them into the shell snippet in the first
> > place. I notice the others tests in this script set up the expect file
> > outside of a block. You could also do something like:
> >
> >   sq=\'
> >   test_expect_success '...' '
> >         cat >expect <<-EOF
> >         Execution of ${sq}false $submodulesha1${sq} ...
> >   '
> >
> > but I'm not sure if that is any more readable.
> >
> 
> If I recall correctly, I made a big fuss about single quotes used correctly when
> writing that patch (which is why I may have lost track of the actual work there)
> to be told the one and only blessed way to use single quotes in our test suite.
> 
> Your proposal to use ${sq} sounds good to me, though we did not
> follow through with it for some other reason. I can reroll with that, though.

I'm not sure if it's a proposal. I ended it with "I'm not sure if that
is any more readable". :)

I mainly care about making sure the interpolation happens inside the
test block (i.e., at the top of the quoted section above).

-Peff

Re: [PATCH] t7406: correct test case for submodule-update initial population

[Jonathan Nieder]

Stefan Beller wrote:
> On Wed, Mar 22, 2017 at 2:59 PM, Jeff King <peff@peff.net> wrote:

>>   sq=\'
>>   test_expect_success '...' '
>>         cat >expect <<-EOF
>>         Execution of ${sq}false $submodulesha1${sq} ...
>>   '
>>
>> but I'm not sure if that is any more readable.
>
> If I recall correctly, I made a big fuss about single quotes used correctly when
> writing that patch (which is why I may have lost track of the actual work there)
> to be told the one and only blessed way to use single quotes in our test suite.
>
> Your proposal to use ${sq} sounds good to me, though we did not
> follow through with it for some other reason. I can reroll with that, though.

I don't know what you're referring to by only and blessed way, but it
might be the following.  If you want to use single-quotes on a command
line using $sq, you would have to use eval:

	eval "some_command ${sq}single-quoted argument${sq}"

which is generally more complicated than doing the '\'' dance:

	some_command '\''single-quoted argument'\''

But the example in this thread is different. In the here-document you
are in an interpolated context so the ${sq} method or '\'' would work
equally well.

Hope that helps,
Jonathan

Re: [PATCH] t7406: correct test case for submodule-update initial population

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Neither of those is true, but I think:
>
>   cat >expect <<-EOF &&
>   Execution of '\''false $submodulesha1'\'' failed in ...
>   EOF
>
> is safer and less surprising. The single-quote handling is unfortunate and
> ugly, but necessary to get them into the shell snippet in the first
> place. I notice the others tests in this script set up the expect file
> outside of a block. You could also do something like:
>
>   sq=\'
>   test_expect_success '...' '
> 	cat >expect <<-EOF
> 	Execution of ${sq}false $submodulesha1${sq} ...
>   '
>
> but I'm not sure if that is any more readable.

Yup, my eyes have long learned to coast over '\'' as an idiomatic
symbol, but I agree that it is harder to see until you get used to
it (and I do not think it is particularly useful skill to be able to
spot '\'' as a logical unit, either).  ${sq} thing may make it easier
to read but I think the one you did in the first quoted part in this
reply is good enough.

-- >8 --
Subject: t7406: correct test case for submodule-update initial population

There are three issues with the test:

* The syntax of the here-doc was wrong, such that the entire test was
  sucked into the here-doc, which is why the test succeeded successfully.

* The variable $submodulesha1 was not expanded as it was inside a quoted
  here text.  We do not want to quote EOF marker for this.

* The redirection from the git command to the output file for comparison
  was wrong as the -C operator from git doesn't apply to the redirect path.
  Also we're interested in stderr of that command.

Noticed-by: Jan Palus <jan.palus@gmail.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 t/t7406-submodule-update.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 8c086a429b..a70fe96ad6 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -425,11 +425,11 @@ test_expect_success 'submodule update - command in .git/config catches failure -
 '
 
 test_expect_success 'submodule update - command run for initial population of submodule' '
-	cat <<-\ EOF >expect
+	cat >expect <<-EOF &&
 	Execution of '\''false $submodulesha1'\'' failed in submodule path '\''submodule'\''
-	EOF &&
+	EOF
 	rm -rf super/submodule &&
-	test_must_fail git -C super submodule update >../actual &&
+	test_must_fail git -C super submodule update 2>actual &&
 	test_cmp expect actual &&
 	git -C super submodule update --checkout
 '

Re: [PATCH] t7406: correct test case for submodule-update initial population

[Jeff King]

On Wed, Mar 22, 2017 at 03:12:07PM -0700, Junio C Hamano wrote:

> >   sq=\'
> >   test_expect_success '...' '
> > 	cat >expect <<-EOF
> > 	Execution of ${sq}false $submodulesha1${sq} ...
> >   '
> >
> > but I'm not sure if that is any more readable.
> 
> Yup, my eyes have long learned to coast over '\'' as an idiomatic
> symbol, but I agree that it is harder to see until you get used to
> it (and I do not think it is particularly useful skill to be able to
> spot '\'' as a logical unit, either).  ${sq} thing may make it easier
> to read but I think the one you did in the first quoted part in this
> reply is good enough.

Sounds good.

> -- >8 --
> Subject: t7406: correct test case for submodule-update initial population
> 
> There are three issues with the test:
> 
> * The syntax of the here-doc was wrong, such that the entire test was
>   sucked into the here-doc, which is why the test succeeded successfully.

This version looks fine except for the repetitious repetition. :)

-Peff

Re: [PATCH] t7406: correct test case for submodule-update initial population

[Stefan Beller]

On Wed, Mar 22, 2017 at 3:24 PM, Jeff King <peff@peff.net> wrote:
> On Wed, Mar 22, 2017 at 03:12:07PM -0700, Junio C Hamano wrote:
>
>> >   sq=\'
>> >   test_expect_success '...' '
>> >     cat >expect <<-EOF
>> >     Execution of ${sq}false $submodulesha1${sq} ...
>> >   '
>> >
>> > but I'm not sure if that is any more readable.
>>
>> Yup, my eyes have long learned to coast over '\'' as an idiomatic
>> symbol, but I agree that it is harder to see until you get used to
>> it (and I do not think it is particularly useful skill to be able to
>> spot '\'' as a logical unit, either).  ${sq} thing may make it easier
>> to read but I think the one you did in the first quoted part in this
>> reply is good enough.
>
> Sounds good.
>
>> -- >8 --
>> Subject: t7406: correct test case for submodule-update initial population
>>
>> There are three issues with the test:
>>
>> * The syntax of the here-doc was wrong, such that the entire test was
>>   sucked into the here-doc, which is why the test succeeded successfully.
>
> This version looks fine except for the repetitious repetition. :)

When I wrote it, It sounded more convincing, but we can drop the
"successfully".

Thanks,
Stefan

Re: [PATCH 2/3] t7406: fix here-doc syntax errors

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> After applying this, I get a failure:
>
>   --- expect	2017-03-22 21:01:53.350721155 +0000
>   +++ actual	2017-03-22 21:01:53.346721155 +0000
>   @@ -1 +1 @@
>   -Execution of 'false $submodulesha1' failed in submodule path 'submodule'
>   +Execution of 'false 4301fd3e4110d3b6212c19aed3094150392545b9' failed in submodule path '../submodule'
>
> At the very least, we need to drop the "\" from EOF to expand
> $submodulesha1.

Right.

> But the submodule path seems wrong, too. I'm not sure if
> the expectation is wrong, or if there's a bug. +cc Stefan

[PATCH 3/3] t7004, t7030: fix here-doc syntax errors

[Junio C Hamano]

From: Jan Palus <jan.palus@gmail.com>

These all came as part of an earlier st/verify-tag topic that was
merged to 2.12.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * This should be applied on top of 4fea72f4 ("t/t7004-tag: Add
   --format specifier tests", 2017-01-17)

 t/t7004-tag.sh        | 8 ++++----
 t/t7030-verify-tag.sh | 8 ++++----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index b4698ab5f5..efc6dde7b8 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -896,17 +896,17 @@ test_expect_success GPG 'verifying a forged tag should fail' '
 '
 
 test_expect_success 'verifying a proper tag with --format pass and format accordingly' '
-	cat >expect <<-\EOF
+	cat >expect <<-\EOF &&
 	tagname : signed-tag
-	EOF &&
+	EOF
 	git tag -v --format="tagname : %(tag)" "signed-tag" >actual &&
 	test_cmp expect actual
 '
 
 test_expect_success 'verifying a forged tag with --format fail and format accordingly' '
-	cat >expect <<-\EOF
+	cat >expect <<-\EOF &&
 	tagname : forged-tag
-	EOF &&
+	EOF
 	test_must_fail git tag -v --format="tagname : %(tag)" "forged-tag" >actual &&
 	test_cmp expect actual
 '
diff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh
index d62ccbb98e..ce37fd9864 100755
--- a/t/t7030-verify-tag.sh
+++ b/t/t7030-verify-tag.sh
@@ -126,17 +126,17 @@ test_expect_success GPG 'verify multiple tags' '
 '
 
 test_expect_success 'verifying tag with --format' '
-	cat >expect <<-\EOF
+	cat >expect <<-\EOF &&
 	tagname : fourth-signed
-	EOF &&
+	EOF
 	git verify-tag --format="tagname : %(tag)" "fourth-signed" >actual &&
 	test_cmp expect actual
 '
 
 test_expect_success 'verifying a forged tag with --format fail and format accordingly' '
-	cat >expect <<-\EOF
+	cat >expect <<-\EOF &&
 	tagname : 7th forged-signed
-	EOF &&
+	EOF
 	test_must_fail git verify-tag --format="tagname : %(tag)" $(cat forged1.tag) >actual-forged &&
 	test_cmp expect actual-forged
 '
-- 
2.12.1-430-gafd6726309

Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors

[Jeff King]

On Wed, Mar 22, 2017 at 01:08:05PM -0700, Junio C Hamano wrote:

> From: Jan Palus <jan.palus@gmail.com>
> 
> These all came as part of an earlier st/verify-tag topic that was
> merged to 2.12.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> 
>  * This should be applied on top of 4fea72f4 ("t/t7004-tag: Add
>    --format specifier tests", 2017-01-17)
> 
>  t/t7004-tag.sh        | 8 ++++----
>  t/t7030-verify-tag.sh | 8 ++++----
>  2 files changed, 8 insertions(+), 8 deletions(-)

Like 2/3, this one also produces test failures for me. It looks like
"verify-tag" does not show a tag which has been forged. I'm not sure if
that's intentional (and the test is wrong) or a bug.  +cc Santiago

-Peff

Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors

[Santiago Torres]

[-- Attachment #1: Type: text/plain, Size: 1329 bytes --]

On Wed, Mar 22, 2017 at 05:10:03PM -0400, Jeff King wrote:
> On Wed, Mar 22, 2017 at 01:08:05PM -0700, Junio C Hamano wrote:
> 
> > From: Jan Palus <jan.palus@gmail.com>
> > 
> > These all came as part of an earlier st/verify-tag topic that was
> > merged to 2.12.
> > 
> > Signed-off-by: Junio C Hamano <gitster@pobox.com>
> > ---
> > 
> >  * This should be applied on top of 4fea72f4 ("t/t7004-tag: Add
> >    --format specifier tests", 2017-01-17)
> > 
> >  t/t7004-tag.sh        | 8 ++++----
> >  t/t7030-verify-tag.sh | 8 ++++----
> >  2 files changed, 8 insertions(+), 8 deletions(-)
> 

On my side, these patches make tests fail. I'm wondering if this is an
issue with the underlying shell (probably the version)? Let me try to
figure out what is exactly going on here.

> Like 2/3, this one also produces test failures for me. It looks like
> "verify-tag" does not show a tag which has been forged. I'm not sure if
> that's intentional (and the test is wrong) or a bug. 

I see that offending code would be [1]. Changing this behavior should be
trivial (dropping the continue), although I'm not sure if this is what
we want?

> 
> -Peff

Thanks,
-Santiago.

[1] https://github.com/git/git/blob/master/builtin/verify-tag.c#L6://github.com/git/git/blob/master/builtin/verify-tag.c#L67

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors

[Jeff King]

On Wed, Mar 22, 2017 at 05:43:57PM -0400, Santiago Torres wrote:

> > Like 2/3, this one also produces test failures for me. It looks like
> > "verify-tag" does not show a tag which has been forged. I'm not sure if
> > that's intentional (and the test is wrong) or a bug. 
> 
> I see that offending code would be [1]. Changing this behavior should be
> trivial (dropping the continue), although I'm not sure if this is what
> we want?

I could see arguments for either behavior. The fact that v2.12 was
released with the skip-gpg-failures behavior makes me inclined to just
keep that and fix the test. But I don't feel strongly.

If we do change it, I think builtin/tag.c:verify_tag() would need a
similar fix (to avoid the early return).

-Peff

Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Mar 22, 2017 at 01:08:05PM -0700, Junio C Hamano wrote:
>
>> From: Jan Palus <jan.palus@gmail.com>
>> 
>> These all came as part of an earlier st/verify-tag topic that was
>> merged to 2.12.
>> 
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>> 
>>  * This should be applied on top of 4fea72f4 ("t/t7004-tag: Add
>>    --format specifier tests", 2017-01-17)
>> 
>>  t/t7004-tag.sh        | 8 ++++----
>>  t/t7030-verify-tag.sh | 8 ++++----
>>  2 files changed, 8 insertions(+), 8 deletions(-)
>
> Like 2/3, this one also produces test failures for me. It looks like
> "verify-tag" does not show a tag which has been forged. I'm not sure if
> that's intentional (and the test is wrong) or a bug.  +cc Santiago

It appears that the test expected a broken one to be shown, and my
reading of its log message is that the change expected --format= to
be used with %G? so that scripts can tell between pass and fail?  

So if I have to judge, the code becoming silent for a tag that does
not pass verification is not doing what the commit wanted it to do.

Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors

[Santiago Torres]

[-- Attachment #1: Type: text/plain, Size: 1544 bytes --]

On Wed, Mar 22, 2017 at 03:04:32PM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
> > On Wed, Mar 22, 2017 at 01:08:05PM -0700, Junio C Hamano wrote:
> >
> >> From: Jan Palus <jan.palus@gmail.com>
> >> 
> >> These all came as part of an earlier st/verify-tag topic that was
> >> merged to 2.12.
> >> 
> >> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> >> ---
> >> 
> >>  * This should be applied on top of 4fea72f4 ("t/t7004-tag: Add
> >>    --format specifier tests", 2017-01-17)
> >> 
> >>  t/t7004-tag.sh        | 8 ++++----
> >>  t/t7030-verify-tag.sh | 8 ++++----
> >>  2 files changed, 8 insertions(+), 8 deletions(-)
> >
> > Like 2/3, this one also produces test failures for me. It looks like
> > "verify-tag" does not show a tag which has been forged. I'm not sure if
> > that's intentional (and the test is wrong) or a bug.  +cc Santiago
> 
> It appears that the test expected a broken one to be shown, and my
> reading of its log message is that the change expected --format= to
> be used with %G? so that scripts can tell between pass and fail?  
> 
> So if I have to judge, the code becoming silent for a tag that does
> not pass verification is not doing what the commit wanted it to do.
> 

Yes, considering the test name is:

    "verifying a forged tag with --format fail and format accordingly" 

It feels as if the behavior of verify-tag/tag -v is not the one
intended. I could add two patches on top of those two commits.

Would this be enough?
-Santiago.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors

[Jeff King]

On Wed, Mar 22, 2017 at 06:15:57PM -0400, Santiago Torres wrote:

> > > Like 2/3, this one also produces test failures for me. It looks like
> > > "verify-tag" does not show a tag which has been forged. I'm not sure if
> > > that's intentional (and the test is wrong) or a bug.  +cc Santiago
> > 
> > It appears that the test expected a broken one to be shown, and my
> > reading of its log message is that the change expected --format= to
> > be used with %G? so that scripts can tell between pass and fail?  
> > 
> > So if I have to judge, the code becoming silent for a tag that does
> > not pass verification is not doing what the commit wanted it to do.
> > 
> 
> Yes, considering the test name is:
> 
>     "verifying a forged tag with --format fail and format accordingly" 
> 
> It feels as if the behavior of verify-tag/tag -v is not the one
> intended. I could add two patches on top of those two commits.

I worked up the patch to do that (see below), but I got stumped trying
to write the commit message. Perhaps that is what the test intended, but
I don't think tag's --format understands "%G" codes at all. So you
cannot tell from the output if a tag was valid or not; you have to check
the exit code separately.

And if you do something like:

  git tag -v --format='%(tag)' foo bar |
  while read tag
  do
     ...
  done

you cannot tell at all which ones are bogus. Whereas with the current
behavior, the bogus ones are quietly omitted. Which can also be
confusing, but I'd think would generally err on the side of caution.

-Peff

---
diff --git a/builtin/tag.c b/builtin/tag.c
index fbb85ba3d..37e768db4 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -107,19 +107,19 @@ static int verify_tag(const char *name, const char *ref,
 		      const unsigned char *sha1, const void *cb_data)
 {
 	int flags;
+	int ret;
 	const char *fmt_pretty = cb_data;
 	flags = GPG_VERIFY_VERBOSE;
 
 	if (fmt_pretty)
 		flags = GPG_VERIFY_OMIT_STATUS;
 
-	if (gpg_verify_tag(sha1, name, flags))
-		return -1;
+	ret = gpg_verify_tag(sha1, name, flags);
 
 	if (fmt_pretty)
 		pretty_print_ref(name, sha1, fmt_pretty);
 
-	return 0;
+	return ret;
 }
 
 static int do_sign(struct strbuf *buffer)
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index 5199553d9..cb1024ef4 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -62,10 +62,8 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
 			continue;
 		}
 
-		if (gpg_verify_tag(sha1, name, flags)) {
+		if (gpg_verify_tag(sha1, name, flags))
 			had_error = 1;
-			continue;
-		}
 
 		if (fmt_pretty)
 			pretty_print_ref(name, sha1, fmt_pretty);
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index b53a2e5e4..633b08956 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -848,17 +848,17 @@ test_expect_success GPG 'verifying a forged tag should fail' '
 '
 
 test_expect_success 'verifying a proper tag with --format pass and format accordingly' '
-	cat >expect <<-\EOF
+	cat >expect <<-\EOF &&
 	tagname : signed-tag
-	EOF &&
+	EOF
 	git tag -v --format="tagname : %(tag)" "signed-tag" >actual &&
 	test_cmp expect actual
 '
 
 test_expect_success 'verifying a forged tag with --format fail and format accordingly' '
-	cat >expect <<-\EOF
+	cat >expect <<-\EOF &&
 	tagname : forged-tag
-	EOF &&
+	EOF
 	test_must_fail git tag -v --format="tagname : %(tag)" "forged-tag" >actual &&
 	test_cmp expect actual
 '
diff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh
index d62ccbb98..ce37fd986 100755
--- a/t/t7030-verify-tag.sh
+++ b/t/t7030-verify-tag.sh
@@ -126,17 +126,17 @@ test_expect_success GPG 'verify multiple tags' '
 '
 
 test_expect_success 'verifying tag with --format' '
-	cat >expect <<-\EOF
+	cat >expect <<-\EOF &&
 	tagname : fourth-signed
-	EOF &&
+	EOF
 	git verify-tag --format="tagname : %(tag)" "fourth-signed" >actual &&
 	test_cmp expect actual
 '
 
 test_expect_success 'verifying a forged tag with --format fail and format accordingly' '
-	cat >expect <<-\EOF
+	cat >expect <<-\EOF &&
 	tagname : 7th forged-signed
-	EOF &&
+	EOF
 	test_must_fail git verify-tag --format="tagname : %(tag)" $(cat forged1.tag) >actual-forged &&
 	test_cmp expect actual-forged
 '

Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors

[Santiago Torres]

[-- Attachment #1: Type: text/plain, Size: 2839 bytes --]

> I worked up the patch to do that (see below), but I got stumped trying
> to write the commit message. Perhaps that is what the test intended, but
> I don't think tag's --format understands "%G" codes at all. So you
> cannot tell from the output if a tag was valid or not; you have to check
> the exit code separately.
> 
> And if you do something like:
> 
>   git tag -v --format='%(tag)' foo bar |
>   while read tag
>   do
>      ...
>   done
> 
> you cannot tell at all which ones are bogus. Whereas with the current
> behavior, the bogus ones are quietly omitted. Which can also be
> confusing, but I'd think would generally err on the side of caution.

In that case, something like this would be closer to the desired
behavior?

I'm also unsure on what would be the right thing to put on the commit
message.

-Santiago.

---
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index b4698ab5f..70f6d4646 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -896,19 +896,18 @@ test_expect_success GPG 'verifying a forged tag should fail' '
 '
 
 test_expect_success 'verifying a proper tag with --format pass and format accordingly' '
-	cat >expect <<-\EOF
+	cat >expect <<-\EOF &&
 	tagname : signed-tag
-	EOF &&
+	EOF
 	git tag -v --format="tagname : %(tag)" "signed-tag" >actual &&
 	test_cmp expect actual
 '
 
-test_expect_success 'verifying a forged tag with --format fail and format accordingly' '
-	cat >expect <<-\EOF
+test_expect_success 'verifying a forged tag with --format should fail silently' '
+	cat >expect <<-\EOF &&
 	tagname : forged-tag
-	EOF &&
-	test_must_fail git tag -v --format="tagname : %(tag)" "forged-tag" >actual &&
-	test_cmp expect actual
+	EOF
+	test_must_fail git tag -v --format="tagname : %(tag)" "forged-tag"
 '
 
 # blank and empty messages for signed tags:
diff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh
index d62ccbb98..ba0aafa1f 100755
--- a/t/t7030-verify-tag.sh
+++ b/t/t7030-verify-tag.sh
@@ -126,19 +126,18 @@ test_expect_success GPG 'verify multiple tags' '
 '
 
 test_expect_success 'verifying tag with --format' '
-	cat >expect <<-\EOF
+	cat >expect <<-\EOF &&
 	tagname : fourth-signed
-	EOF &&
+	EOF
 	git verify-tag --format="tagname : %(tag)" "fourth-signed" >actual &&
 	test_cmp expect actual
 '
 
-test_expect_success 'verifying a forged tag with --format fail and format accordingly' '
-	cat >expect <<-\EOF
+test_expect_success 'verifying a forged tag with --format should fail silently' '
+	cat >expect <<-\EOF &&
 	tagname : 7th forged-signed
-	EOF &&
-	test_must_fail git verify-tag --format="tagname : %(tag)" $(cat forged1.tag) >actual-forged &&
-	test_cmp expect actual-forged
+	EOF
+	test_must_fail git verify-tag --format="tagname : %(tag)" $(cat forged1.tag)
 '
 
 test_done

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors

[Jeff King]

On Wed, Mar 22, 2017 at 06:34:43PM -0400, Santiago Torres wrote:

> > I worked up the patch to do that (see below), but I got stumped trying
> > to write the commit message. Perhaps that is what the test intended, but
> > I don't think tag's --format understands "%G" codes at all. So you
> > cannot tell from the output if a tag was valid or not; you have to check
> > the exit code separately.
> > 
> > And if you do something like:
> > 
> >   git tag -v --format='%(tag)' foo bar |
> >   while read tag
> >   do
> >      ...
> >   done
> > 
> > you cannot tell at all which ones are bogus. Whereas with the current
> > behavior, the bogus ones are quietly omitted. Which can also be
> > confusing, but I'd think would generally err on the side of caution.
> 
> In that case, something like this would be closer to the desired
> behavior?

Yes, though you can spell:

  cat >expect <<-\EOF
  EOF

as just:

  >expect

> I'm also unsure on what would be the right thing to put on the commit
> message.

I think the argument is:

  1. It's safer not to expound on tags that have failed verification (so
     that the caller cannot accidentally use them). Especially since the
     --format cannot tell anything about the GPG status.

     That means that

       tag=$(git verify-tag --format='%(tag)' foo)

     can use a non-blank $tag without having to wonder whether it is
     valid or not.

and

  2. That's what we've done since the feature was released.

The only thing that would give me pause is if were to later add
%G-like formatters, and then:

  xargs git verify-tag --format='%(gpg:status) %(tag)' |
  while read status tag
  do
     ...
  done

would become useful, but we'd be tied to the behavior that we omit the
tag when the gpg verification failed (for backwards compatibility).
OTOH, we could perhaps make the rule "ignored unless %(gpg) formatters
are used". Which would be backwards-compatible and safe for old formats,
and work correctly for new ones.

-Peff

Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> OTOH, we could perhaps make the rule "ignored unless %(gpg) formatters
> are used". Which would be backwards-compatible and safe for old formats,
> and work correctly for new ones.

Yup, that is a very sensible escape hatch that we can use later.

Thanks.

Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors

[Santiago Torres]

[-- Attachment #1: Type: text/plain, Size: 3421 bytes --]

On Wed, Mar 22, 2017 at 06:41:24PM -0400, Jeff King wrote:
> > In that case, something like this would be closer to the desired
> > behavior?
> 
> Yes, though you can spell:
> 
>   cat >expect <<-\EOF
>   EOF
> 
> as just:
> 
>   >expect

Ah, that sounds like a better way to fix this with a smaller diff.
> 
> > I'm also unsure on what would be the right thing to put on the commit
> > message.
> 
> I think the argument is:
> 
>   1. It's safer not to expound on tags that have failed verification (so
>      that the caller cannot accidentally use them). Especially since the
>      --format cannot tell anything about the GPG status.
> 
>      That means that
> 
>        tag=$(git verify-tag --format='%(tag)' foo)
> 
>      can use a non-blank $tag without having to wonder whether it is
>      valid or not.
> 
> and
> 
>   2. That's what we've done since the feature was released.
> 
> The only thing that would give me pause is if were to later add
> %G-like formatters, and then:
> 
>   xargs git verify-tag --format='%(gpg:status) %(tag)' |
>   while read status tag
>   do
>      ...
>   done
> 
> would become useful, but we'd be tied to the behavior that we omit the
> tag when the gpg verification failed (for backwards compatibility).
> OTOH, we could perhaps make the rule "ignored unless %(gpg) formatters
> are used". Which would be backwards-compatible and safe for old formats,
> and work correctly for new ones.

This sounds like a helpful addition to implement. We could update/add
tests for compliance on this once the feature is addded and fix the
ambiguous behavior in the tests now.

Thanks,
-Santiago.

---
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index b4698ab5f..0581053a0 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -896,17 +896,15 @@ test_expect_success GPG 'verifying a forged tag should fail' '
 '
 
 test_expect_success 'verifying a proper tag with --format pass and format accordingly' '
-	cat >expect <<-\EOF
+	cat >expect <<-\EOF &&
 	tagname : signed-tag
-	EOF &&
+	EOF
 	git tag -v --format="tagname : %(tag)" "signed-tag" >actual &&
 	test_cmp expect actual
 '
 
-test_expect_success 'verifying a forged tag with --format fail and format accordingly' '
-	cat >expect <<-\EOF
-	tagname : forged-tag
-	EOF &&
+test_expect_success 'verifying a forged tag with --format should fail silently' '
+	>expect &&
 	test_must_fail git tag -v --format="tagname : %(tag)" "forged-tag" >actual &&
 	test_cmp expect actual
 '
diff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh
index d62ccbb98..173a88e89 100755
--- a/t/t7030-verify-tag.sh
+++ b/t/t7030-verify-tag.sh
@@ -126,17 +126,15 @@ test_expect_success GPG 'verify multiple tags' '
 '
 
 test_expect_success 'verifying tag with --format' '
-	cat >expect <<-\EOF
+	cat >expect <<-\EOF &&
 	tagname : fourth-signed
-	EOF &&
+	EOF
 	git verify-tag --format="tagname : %(tag)" "fourth-signed" >actual &&
 	test_cmp expect actual
 '
 
-test_expect_success 'verifying a forged tag with --format fail and format accordingly' '
-	cat >expect <<-\EOF
-	tagname : 7th forged-signed
-	EOF &&
+test_expect_success 'verifying a forged tag with --format should fail silently' '
+	>expect &&
 	test_must_fail git verify-tag --format="tagname : %(tag)" $(cat forged1.tag) >actual-forged &&
 	test_cmp expect actual-forged
 '

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors

[Junio C Hamano]

Santiago Torres <santiago@nyu.edu> writes:

> This sounds like a helpful addition to implement. We could update/add
> tests for compliance on this once the feature is addded and fix the
> ambiguous behavior in the tests now.

OK, so has everybody agreed what the next step would be?  Is the
patch below a good first step (I still need to get it signed off)?

-- >8 --
Subject: t7004, t7030: fix here-doc syntax errors
From: Santiago Torres <santiago@nyu.edu>

Jan Palus noticed that some here-doc are spelled incorrectly,
resulting the entire remainder of the test as if it were data
slurped into the "expect" file, e.g. in this sequence

	cat >expect <<EOF &&
	... expectation ...
	EOF
	git $cmd_being_tested >actual &&
	test_cmp expect actual

the last command of the test is "cat" that sends everything to
'expect' and succeeds.

Fixing these issues in t7004 and t7030 reveals that "git tag -v"
and "git verify-tag" with their --format option do not work as the
test was expecting originally.  Instead of showing both valid tags
and tags with incorrect signatures on their output, tags that do not
pass verification are omitted from the output.

Arguably, that is a safer behaviour, and because the format
specifiers like %(tag) do not have a way to show if the signature
verifies correctly, the command with the --format option cannot be
used to get a list of tags annotated with their signature validity
anyway.

For now, let's fix the here-doc syntax and update the expectation to
match the reality.  Maybe later when we extend the --format language
available to "git tag -v" and "git verify-tag" to include things
like "%(gpg:status)", we may want to change the behaviour so that
piping a list of tag names into

    xargs git verify-tag --format='%(gpg:status) %(tag)'

becomes a good way to produce such a list, but that is a separate
topic.

Noticed-by: Jan Palus <jan.palus@gmail.com>
Helped-by: Jeff King <peff@peff.net>
---
 t/t7004-tag.sh        | 10 ++++------
 t/t7030-verify-tag.sh | 10 ++++------
 2 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index b4698ab5f5..0581053a06 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -896,17 +896,15 @@ test_expect_success GPG 'verifying a forged tag should fail' '
 '
 
 test_expect_success 'verifying a proper tag with --format pass and format accordingly' '
-	cat >expect <<-\EOF
+	cat >expect <<-\EOF &&
 	tagname : signed-tag
-	EOF &&
+	EOF
 	git tag -v --format="tagname : %(tag)" "signed-tag" >actual &&
 	test_cmp expect actual
 '
 
-test_expect_success 'verifying a forged tag with --format fail and format accordingly' '
-	cat >expect <<-\EOF
-	tagname : forged-tag
-	EOF &&
+test_expect_success 'verifying a forged tag with --format should fail silently' '
+	>expect &&
 	test_must_fail git tag -v --format="tagname : %(tag)" "forged-tag" >actual &&
 	test_cmp expect actual
 '
diff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh
index d62ccbb98e..79864a3411 100755
--- a/t/t7030-verify-tag.sh
+++ b/t/t7030-verify-tag.sh
@@ -126,17 +126,15 @@ test_expect_success GPG 'verify multiple tags' '
 '
 
 test_expect_success 'verifying tag with --format' '
-	cat >expect <<-\EOF
+	cat >expect <<-\EOF &&
 	tagname : fourth-signed
-	EOF &&
+	EOF
 	git verify-tag --format="tagname : %(tag)" "fourth-signed" >actual &&
 	test_cmp expect actual
 '
 
-test_expect_success 'verifying a forged tag with --format fail and format accordingly' '
-	cat >expect <<-\EOF
-	tagname : 7th forged-signed
-	EOF &&
+test_expect_success 'verifying a forged tag with --format should fail silently' '
+	>expect &&
 	test_must_fail git verify-tag --format="tagname : %(tag)" $(cat forged1.tag) >actual-forged &&
 	test_cmp expect actual-forged
 '

Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors

[Santiago Torres]

[-- Attachment #1: Type: text/plain, Size: 3959 bytes --]

On Thu, Mar 23, 2017 at 03:00:08PM -0700, Junio C Hamano wrote:
> Santiago Torres <santiago@nyu.edu> writes:
 
> OK, so has everybody agreed what the next step would be? 

I believe it is, although I imagine getting a confirmation from Peff
would be adequate.

> Is the patch below a good first step (I still need to get it signed
> off)?

I'm adding a signoff to the patch below.

Thanks,
-Santiago

-- >8 --
Subject: t7004, t7030: fix here-doc syntax errors
From: Santiago Torres <santiago@nyu.edu>

Jan Palus noticed that some here-doc are spelled incorrectly,
resulting the entire remainder of the test as if it were data
slurped into the "expect" file, e.g. in this sequence

	cat >expect <<EOF &&
	... expectation ...
	EOF
	git $cmd_being_tested >actual &&
	test_cmp expect actual

the last command of the test is "cat" that sends everything to
'expect' and succeeds.

Fixing these issues in t7004 and t7030 reveals that "git tag -v"
and "git verify-tag" with their --format option do not work as the
test was expecting originally.  Instead of showing both valid tags
and tags with incorrect signatures on their output, tags that do not
pass verification are omitted from the output.

Arguably, that is a safer behaviour, and because the format
specifiers like %(tag) do not have a way to show if the signature
verifies correctly, the command with the --format option cannot be
used to get a list of tags annotated with their signature validity
anyway.

For now, let's fix the here-doc syntax and update the expectation to
match the reality.  Maybe later when we extend the --format language
available to "git tag -v" and "git verify-tag" to include things
like "%(gpg:status)", we may want to change the behaviour so that
piping a list of tag names into

    xargs git verify-tag --format='%(gpg:status) %(tag)'

becomes a good way to produce such a list, but that is a separate
topic.

Signed-off-by: Santiago Torres <santiago@nyu.edu>
Noticed-by: Jan Palus <jan.palus@gmail.com>
Helped-by: Jeff King <peff@peff.net>
---
 t/t7004-tag.sh        | 10 ++++------
 t/t7030-verify-tag.sh | 10 ++++------
 2 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index b4698ab5f5..0581053a06 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -896,17 +896,15 @@ test_expect_success GPG 'verifying a forged tag should fail' '
 '
 
 test_expect_success 'verifying a proper tag with --format pass and format accordingly' '
-	cat >expect <<-\EOF
+	cat >expect <<-\EOF &&
 	tagname : signed-tag
-	EOF &&
+	EOF
 	git tag -v --format="tagname : %(tag)" "signed-tag" >actual &&
 	test_cmp expect actual
 '
 
-test_expect_success 'verifying a forged tag with --format fail and format accordingly' '
-	cat >expect <<-\EOF
-	tagname : forged-tag
-	EOF &&
+test_expect_success 'verifying a forged tag with --format should fail silently' '
+	>expect &&
 	test_must_fail git tag -v --format="tagname : %(tag)" "forged-tag" >actual &&
 	test_cmp expect actual
 '
diff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh
index d62ccbb98e..79864a3411 100755
--- a/t/t7030-verify-tag.sh
+++ b/t/t7030-verify-tag.sh
@@ -126,17 +126,15 @@ test_expect_success GPG 'verify multiple tags' '
 '
 
 test_expect_success 'verifying tag with --format' '
-	cat >expect <<-\EOF
+	cat >expect <<-\EOF &&
 	tagname : fourth-signed
-	EOF &&
+	EOF
 	git verify-tag --format="tagname : %(tag)" "fourth-signed" >actual &&
 	test_cmp expect actual
 '
 
-test_expect_success 'verifying a forged tag with --format fail and format accordingly' '
-	cat >expect <<-\EOF
-	tagname : 7th forged-signed
-	EOF &&
+test_expect_success 'verifying a forged tag with --format should fail silently' '
+	>expect &&
 	test_must_fail git verify-tag --format="tagname : %(tag)" $(cat forged1.tag) >actual-forged &&
 	test_cmp expect actual-forged
 '



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors

[Jeff King]

On Thu, Mar 23, 2017 at 03:00:08PM -0700, Junio C Hamano wrote:

> Santiago Torres <santiago@nyu.edu> writes:
> 
> > This sounds like a helpful addition to implement. We could update/add
> > tests for compliance on this once the feature is addded and fix the
> > ambiguous behavior in the tests now.
> 
> OK, so has everybody agreed what the next step would be?  Is the
> patch below a good first step (I still need to get it signed off)?

Yeah, I think this is the right fix.

> -- >8 --
> Subject: t7004, t7030: fix here-doc syntax errors
> From: Santiago Torres <santiago@nyu.edu>
> 
> Jan Palus noticed that some here-doc are spelled incorrectly,
> resulting the entire remainder of the test as if it were data
> slurped into the "expect" file, e.g. in this sequence

I had trouble parsing this. Perhaps:

  resulting in the entire remainder of the test snippet being slurped
  into the "expect" file as if it were data

-Peff

Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Mar 23, 2017 at 03:00:08PM -0700, Junio C Hamano wrote:
>
>> Santiago Torres <santiago@nyu.edu> writes:
>> 
>> > This sounds like a helpful addition to implement. We could update/add
>> > tests for compliance on this once the feature is addded and fix the
>> > ambiguous behavior in the tests now.
>> 
>> OK, so has everybody agreed what the next step would be?  Is the
>> patch below a good first step (I still need to get it signed off)?
>
> Yeah, I think this is the right fix.
>
>> -- >8 --
>> Subject: t7004, t7030: fix here-doc syntax errors
>> From: Santiago Torres <santiago@nyu.edu>
>> 
>> Jan Palus noticed that some here-doc are spelled incorrectly,
>> resulting the entire remainder of the test as if it were data
>> slurped into the "expect" file, e.g. in this sequence
>
> I had trouble parsing this. Perhaps:
>
>   resulting in the entire remainder of the test snippet being slurped
>   into the "expect" file as if it were data

Thanks.  Will rephrase.

I actually think this uncovers another class of breakage.  t7030
tests should be protected with GPG prereq and 'fourth-signed' that
is made only with the prereq in the first test will not be found.
t7004 probably has the same issue.

Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors

[Jeff King]

On Fri, Mar 24, 2017 at 09:45:30AM -0700, Junio C Hamano wrote:

> I actually think this uncovers another class of breakage.  t7030
> tests should be protected with GPG prereq and 'fourth-signed' that
> is made only with the prereq in the first test will not be found.

It seems like t7030 should just skip_all when the GPG prereq is not
met (it's not wrong to mark each test that's added, but it would have
made this particular mistake harder).

> t7004 probably has the same issue.

These ones should be marked individually, though.

-Peff

Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors

[Jeff King]

On Fri, Mar 24, 2017 at 12:49:43PM -0400, Jeff King wrote:

> On Fri, Mar 24, 2017 at 09:45:30AM -0700, Junio C Hamano wrote:
> 
> > I actually think this uncovers another class of breakage.  t7030
> > tests should be protected with GPG prereq and 'fourth-signed' that
> > is made only with the prereq in the first test will not be found.
> 
> It seems like t7030 should just skip_all when the GPG prereq is not
> met (it's not wrong to mark each test that's added, but it would have
> made this particular mistake harder).
> 
> > t7004 probably has the same issue.
> 
> These ones should be marked individually, though.

I started to prepare a patch for t7030, but given that we want to do
this all in one patch anyway (for bisectability), I think it probably
makes sense to just add the missing GPG prereqs as part of the patch you
posted. If somebody wants to convert t7030 to skip_all on top that's
fine, but it's not that big a deal.

-Peff

Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> It seems like t7030 should just skip_all when the GPG prereq is not
> met (it's not wrong to mark each test that's added, but it would have
> made this particular mistake harder).

I'd leave that to be done by others after the dust settles ;-).  

Here is what I have right now (proposed log message has updates to
match rather obvious changes to the tests).

-- >8 --
From: Santiago Torres <santiago@nyu.edu>
Date: Thu, 23 Mar 2017 18:28:47 -0400
Subject: [PATCH] t7004, t7030: fix here-doc syntax errors

Jan Palus noticed that some here-doc are spelled incorrectly,
resulting the entire remainder of the test snippet being slurped
into the "expect" file as if it were data, e.g. in this sequence

	cat >expect <<EOF &&
	... expectation ...
	EOF
	git $cmd_being_tested >actual &&
	test_cmp expect actual

the last command of the test is "cat" that sends everything to
'expect' and succeeds.

Fixing these issues in t7004 and t7030 reveals that "git tag -v"
and "git verify-tag" with their --format option do not work as the
test was expecting originally.  Instead of showing both valid tags
and tags with incorrect signatures on their output, tags that do not
pass verification are omitted from the output.  Another breakage that
is uncovered is that these tests must be restricted to environment
where gpg is available.

Arguably, that is a safer behaviour, and because the format
specifiers like %(tag) do not have a way to show if the signature
verifies correctly, the command with the --format option cannot be
used to get a list of tags annotated with their signature validity
anyway.

For now, let's fix the here-doc syntax, update the expectation to
match the reality, and update the test prerequisite.

Maybe later when we extend the --format language available to "git
tag -v" and "git verify-tag" to include things like "%(gpg:status)",
we may want to change the behaviour so that piping a list of tag
names into

    xargs git verify-tag --format='%(gpg:status) %(tag)'

becomes a good way to produce such a list, but that is a separate
topic.

Noticed-by: Jan Palus <jan.palus@gmail.com>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Santiago Torres <santiago@nyu.edu>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t7004-tag.sh        | 12 +++++-------
 t/t7030-verify-tag.sh | 12 +++++-------
 2 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index b53a2e5e41..f67bbb8abc 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -847,18 +847,16 @@ test_expect_success GPG 'verifying a forged tag should fail' '
 	test_must_fail git tag -v forged-tag
 '
 
-test_expect_success 'verifying a proper tag with --format pass and format accordingly' '
-	cat >expect <<-\EOF
+test_expect_success GPG 'verifying a proper tag with --format pass and format accordingly' '
+	cat >expect <<-\EOF &&
 	tagname : signed-tag
-	EOF &&
+	EOF
 	git tag -v --format="tagname : %(tag)" "signed-tag" >actual &&
 	test_cmp expect actual
 '
 
-test_expect_success 'verifying a forged tag with --format fail and format accordingly' '
-	cat >expect <<-\EOF
-	tagname : forged-tag
-	EOF &&
+test_expect_success GPG 'verifying a forged tag with --format should fail silently' '
+	>expect &&
 	test_must_fail git tag -v --format="tagname : %(tag)" "forged-tag" >actual &&
 	test_cmp expect actual
 '
diff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh
index d62ccbb98e..b4b49eeb08 100755
--- a/t/t7030-verify-tag.sh
+++ b/t/t7030-verify-tag.sh
@@ -125,18 +125,16 @@ test_expect_success GPG 'verify multiple tags' '
 	test_cmp expect.stderr actual.stderr
 '
 
-test_expect_success 'verifying tag with --format' '
-	cat >expect <<-\EOF
+test_expect_success GPG 'verifying tag with --format' '
+	cat >expect <<-\EOF &&
 	tagname : fourth-signed
-	EOF &&
+	EOF
 	git verify-tag --format="tagname : %(tag)" "fourth-signed" >actual &&
 	test_cmp expect actual
 '
 
-test_expect_success 'verifying a forged tag with --format fail and format accordingly' '
-	cat >expect <<-\EOF
-	tagname : 7th forged-signed
-	EOF &&
+test_expect_success GPG 'verifying a forged tag with --format should fail silently' '
+	>expect &&
 	test_must_fail git verify-tag --format="tagname : %(tag)" $(cat forged1.tag) >actual-forged &&
 	test_cmp expect actual-forged
 '
-- 
2.12.1-432-gf364f02724

Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors

[Jeff King]

On Fri, Mar 24, 2017 at 11:04:27AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > It seems like t7030 should just skip_all when the GPG prereq is not
> > met (it's not wrong to mark each test that's added, but it would have
> > made this particular mistake harder).
> 
> I'd leave that to be done by others after the dust settles ;-).
> 
> Here is what I have right now (proposed log message has updates to
> match rather obvious changes to the tests).

Thanks, this looks good to me.

-Peff

Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I worked up the patch to do that (see below), but I got stumped trying
> to write the commit message. Perhaps that is what the test intended, but
> I don't think tag's --format understands "%G" codes at all.
> So you cannot tell from the output if a tag was valid or not; you have to check
> the exit code separately.

Thanks for digging; that was an unexpected show-stopper to me X-<.

> you cannot tell at all which ones are bogus. Whereas with the current
> behavior, the bogus ones are quietly omitted. Which can also be
> confusing, but I'd think would generally err on the side of caution.

OK.

Re: [PATCH 0/3] fix "here-doc" syntax errors

[Jan Palus]

On 22.03.2017 13:08, Junio C Hamano wrote:
> Please respond to the list, saying it is OK to add your "sign-off"
> (see Documentation/SubmittingPatches) to these patches.

Please feel free to do so and thanks for handling the issue.


Regards
Jan

[PATCH] tests: lint for run-away here-doc

[Junio C Hamano]

We found a few run-away here documents that are started with an
end-of-here-doc marker that is incorrectly spelled, e.g.

	git some command >actual &&
	cat <<EOF >expect
	...
	EOF &&
	test_cmp expect actual

which ends up slurping the entire remainder of the script as if it
were the data.  Often the command that gets misused like this exits
without failure (e.g. "cat" in the above example), which makes the
command appear to work, without eve executing the remainder of the
test.

Use a trick similar to the one used to catch the &&-chain breakage
to detect this case.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * This catches all the cases detected in the recent discussion, I think.

 t/test-lib.sh | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 86d77c16dd..97bdc91f54 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -627,6 +627,10 @@ test_run_ () {
 		test_eval_ "(exit 117) && $1"
 		if test "$?" != 117; then
 			error "bug in the test script: broken &&-chain: $1"
+		elif ! OK=$(test_eval_ "false && $1${LF}${LF}echo OK" 2>/dev/null) ||
+		   test OK != "$OK"
+		then
+			error "bug in the test script: possibly unterminated HERE-DOC"
 		fi
 		trace=$trace_tmp
 	fi

Re: [PATCH v2] tests: lint for run-away here-doc

[Junio C Hamano]

We found a few run-away here documents that are started with an
end-of-here-doc marker that is incorrectly spelled, e.g.

	git some command >actual &&
	cat <<EOF >expect
	...
	EOF &&
	test_cmp expect actual

which ends up slurping the entire remainder of the script as if it
were the data.  Often the command that gets misused like this exits
without failure (e.g. "cat" in the above example), which makes the
command appear to work, without eve executing the remainder of the
test.

Piggy-back on the test that catches &&-chain breakage to detect this
case as well.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * The previous one was simply buggy; I forgot that there was an
   interesting redirection going on inside test_eval_.  Sorry for
   the noise.

   Also we could do this in the same test_eval_ without adding
   another one, which is how this version does it.

 t/test-lib.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 86d77c16dd..d5f2b70bce 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -624,9 +624,9 @@ test_run_ () {
 		trace=
 		# 117 is magic because it is unlikely to match the exit
 		# code of other programs
-		test_eval_ "(exit 117) && $1"
-		if test "$?" != 117; then
-			error "bug in the test script: broken &&-chain: $1"
+		if test "OK-117" != "$(test_eval_ "(exit 117) && $1${LF}${LF}echo OK-\$?" 3>&1)"
+		then
+			error "bug in the test script: broken &&-chain or run-away HERE-DOC: $1"
 		fi
 		trace=$trace_tmp
 	fi

Re: [PATCH v2] tests: lint for run-away here-doc

[Jonathan Nieder]

Hi,

Junio C Hamano wrote:

> We found a few run-away here documents that are started with an
> end-of-here-doc marker that is incorrectly spelled, e.g.
>
> 	git some command >actual &&
> 	cat <<EOF >expect
> 	...
> 	EOF &&
> 	test_cmp expect actual
>
> which ends up slurping the entire remainder of the script as if it
> were the data.  Often the command that gets misused like this exits
> without failure (e.g. "cat" in the above example), which makes the
> command appear to work, without eve executing the remainder of the

s/eve/ever/

> test.
>
> Piggy-back on the test that catches &&-chain breakage to detect this
> case as well.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---

I also wonder if we can use an "sh -n" style syntax check some day,
but that's likely harder.

[...]
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -624,9 +624,9 @@ test_run_ () {
>  		trace=
>  		# 117 is magic because it is unlikely to match the exit
>  		# code of other programs
> -		test_eval_ "(exit 117) && $1"
> -		if test "$?" != 117; then
> -			error "bug in the test script: broken &&-chain: $1"
> +		if test "OK-117" != "$(test_eval_ "(exit 117) && $1${LF}${LF}echo OK-\$?" 3>&1)"
> +		then
> +			error "bug in the test script: broken &&-chain or run-away HERE-DOC: $1"
>  		fi

Neat.  Why the double-LF?

In some shells, the 3>&1 will last past the function call.
Fortunately, the $() substitution creates a subshell so this doesn't
affect anything later on.

test_eval_inner_ contains a warning not to append anything after the
commands to be evaluated, since whatever you append would pollute -x
tracing output.  Fortunately, in this context we have already set
trace= so the warning does not apply.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

Re: [PATCH v2] tests: lint for run-away here-doc

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

>> without failure (e.g. "cat" in the above example), which makes the
>> command appear to work, without eve executing the remainder of the
>
> s/eve/ever/

Oops.

>> +		if test "OK-117" != "$(test_eval_ "(exit 117) && $1${LF}${LF}echo OK-\$?" 3>&1)"
>> +		then
>> +			error "bug in the test script: broken &&-chain or run-away HERE-DOC: $1"
>>  		fi
>
> Neat.  Why the double-LF?

The main part of "Neat" was your invention ;-).

Imagining how the string passed to eval looked, having two LFs was
the easiest way to ensure that there is a blank line before the new
"echo" (not just the last line in $1 and "echo" are on different
lines), which was more visually pleasing.  There was no any real
functional requirement.

> In some shells, the 3>&1 will last past the function call.
> Fortunately, the $() substitution creates a subshell so this doesn't
> affect anything later on.

Yes, a subshell solves quite lot of problems (while possibly
introducing others, though ;-).

> test_eval_inner_ contains a warning not to append anything after the
> commands to be evaluated, since whatever you append would pollute -x
> tracing output.  Fortunately, in this context we have already set
> trace= so the warning does not apply.
>
>
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

Re: [PATCH v2] tests: lint for run-away here-doc

[Jeff King]

On Wed, Mar 22, 2017 at 10:43:18PM -0700, Junio C Hamano wrote:

> -		test_eval_ "(exit 117) && $1"
> -		if test "$?" != 117; then
> -			error "bug in the test script: broken &&-chain: $1"
> +		if test "OK-117" != "$(test_eval_ "(exit 117) && $1${LF}${LF}echo OK-\$?" 3>&1)"
> +		then
> +			error "bug in the test script: broken &&-chain or run-away HERE-DOC: $1"

This looks good. I had pondered how we might do this but was worried
that it would have to end up actually executing the test contents. But
this leverages the fact that the problem is syntactic and that the shell
will parse the complete &&-chain before executing any of it. Clever.

-Peff