EOF test fixes (t5615/t7004)

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.

diff -ur git-2.12.0.orig/t/t7004-tag.sh git-2.12.0/t/t7004-tag.sh

t/t7004-tag.sh +2 -2

@@ -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
 '

From: Jan Palus <[email protected]>

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 <[email protected]>

---

* 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(+)

t/t5615-alternate-env.sh +0 -0

index 26ebb0375d..d2d883f3a1 100755

t/t7406-submodule-update.sh test_expect_success 'submodule update - command in .git/config catches failure - · line 447

Quoted 13 lines · Junio C Hamano

t/t7406-submodule-update.sh +2 -2

index 347857fa7c..a20df9420a 100755
@@ -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

Quoted 2 lines

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.

* 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.

Quoted 2 lines · Stefan Beller

* 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? :)

Quoted 2 lines

* 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:

t/t7406-submodule-update.sh test_expect_success 'submodule update - command in .git/config catches failure - · line 430

Quoted 3 lines

t/t7406-submodule-update.sh +3 -0

+     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.

Quoted 11 lines

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:

Quoted 8 lines

  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.

Quoted 6 lines · Jeff King

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

Quoted 1 line

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.

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.

Quoted 6 lines

sq=\'
test_expect_success '...' '

	cat >expect <<-EOF
	Execution of ${sq}false $submodulesha1${sq} ...

'

Quoted 1 line

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.

Quoted 6 lines · Junio C Hamano

Quoted 6 lines

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

Quoted 8 lines

Quoted 1 line

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.

Quoted 3 lines

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

Quoted 4 lines

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. :)

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

From: Jan Palus <[email protected]>

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

Signed-off-by Junio C Hamano <[email protected]>

---

* 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(-)

Quoted 8 lines · Junio C Hamano

From: Jan Palus <[email protected]>

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

Signed-off-by Junio C Hamano <[email protected]>

---

Quoted 6 lines

* 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.

-Peff

Quoted 3 lines

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?

Quoted 11 lines · Junio C Hamano

From: Jan Palus <[email protected]>

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

Signed-off-by Junio C Hamano <[email protected]>

---

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

Quoted 3 lines

 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

Quoted 13 lines · Jeff King

Quoted 11 lines · Junio C Hamano

From: Jan Palus <[email protected]>

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

Signed-off-by Junio C Hamano <[email protected]>

---

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

Quoted 7 lines

Quoted 3 lines

 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.

Quoted 11 lines

Quoted 3 lines

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.

Quoted 14 lines

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

Quoted 3 lines

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?

expect

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

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.

Quoted 2 lines

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

Yes, though you can spell:

cat >expect <<-\EOF
EOF

as just:

Quoted 1 line

expect

Quoted 3 lines

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.

Santiago Torres <[email protected]> writes:

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)?

Quoted 3 lines · Santiago Torres

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 <[email protected]>

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

Quoted 8 lines · Junio C Hamano

Quoted 3 lines · Santiago Torres

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.

Quoted 4 lines

-- >8 --
Subject: t7004, t7030: fix here-doc syntax errors
From: Santiago Torres <[email protected]>

Quoted 3 lines

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

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.

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).

Quoted 3 lines · Jeff King

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).

Quoted 3 lines · Junio C Hamano

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).

Quoted 1 line

t7004 probably has the same issue.

These ones should be marked individually, though.

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.

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.

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

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 <[email protected]>

---

Quoted 2 lines

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/

t/test-lib.sh test_run_ () { · line 630

Quoted 4 lines

t/test-lib.sh +3 -0

+		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 <[email protected]>