t6392 broken on pu (Mac OS X)

t6392 broken on pu (Mac OS X)

[Torsten Bögershausen]

These tests fail here under Mac OS,
they pass under Linux:
commit ff3d9b660a4b6e9d3eeb664ce1febe717adff737
I haven't had a chance to dig further.


expecting success:
	git for-each-ref --format="%(if)%(authorname)%(then)%(authorname):
%(refname)%(end)" >actual &&
	cat >expect <<-\EOF &&
	A U Thor: refs/heads/master
	A U Thor: refs/heads/side
	A U Thor: refs/odd/spot



	A U Thor: refs/tags/foo1.10
	A U Thor: refs/tags/foo1.3
	A U Thor: refs/tags/foo1.6
	A U Thor: refs/tags/four
	A U Thor: refs/tags/one

	A U Thor: refs/tags/three
	A U Thor: refs/tags/two
	EOF
	test_cmp expect actual

--- expect	2016-05-07 16:08:32.000000000 +0000
+++ actual	2016-05-07 16:08:32.000000000 +0000
@@ -3,12 +3,10 @@
 A U Thor: refs/odd/spot


-
 A U Thor: refs/tags/foo1.10
 A U Thor: refs/tags/foo1.3
 A U Thor: refs/tags/foo1.6
 A U Thor: refs/tags/four
 A U Thor: refs/tags/one
-
 A U Thor: refs/tags/three
 A U Thor: refs/tags/two
not ok 34 - check %(if)...%(then)...%(end) atoms
#	
#		git for-each-ref --format="%(if)%(authorname)%(then)%(authorname):
%(refname)%(end)" >actual &&
#		cat >expect <<-\EOF &&
#		A U Thor: refs/heads/master
#		A U Thor: refs/heads/side
#		A U Thor: refs/odd/spot
#	
#	
#	
#		A U Thor: refs/tags/foo1.10
#		A U Thor: refs/tags/foo1.3
#		A U Thor: refs/tags/foo1.6
#		A U Thor: refs/tags/four
#		A U Thor: refs/tags/one
#	
#		A U Thor: refs/tags/three
#		A U Thor: refs/tags/two
#		EOF
#		test_cmp expect actual
#	

expecting success:
	git for-each-ref --format="%(if)%(authorname)%(then)%(authorname)%(else)No
author%(end): %(refname)" >actual &&
	cat >expect <<-\EOF &&
	A U Thor: refs/heads/master
	A U Thor: refs/heads/side
	A U Thor: refs/odd/spot
	No author: refs/tags/annotated-tag
	No author: refs/tags/doubly-annotated-tag
	No author: refs/tags/doubly-signed-tag
	A U Thor: refs/tags/foo1.10
	A U Thor: refs/tags/foo1.3
	A U Thor: refs/tags/foo1.6
	A U Thor: refs/tags/four
	A U Thor: refs/tags/one
	No author: refs/tags/signed-tag
	A U Thor: refs/tags/three
	A U Thor: refs/tags/two
	EOF
	test_cmp expect actual

--- expect	2016-05-07 16:08:32.000000000 +0000
+++ actual	2016-05-07 16:08:32.000000000 +0000
@@ -3,12 +3,10 @@
 A U Thor: refs/odd/spot
 No author: refs/tags/annotated-tag
 No author: refs/tags/doubly-annotated-tag
-No author: refs/tags/doubly-signed-tag
 A U Thor: refs/tags/foo1.10
 A U Thor: refs/tags/foo1.3
 A U Thor: refs/tags/foo1.6
 A U Thor: refs/tags/four
 A U Thor: refs/tags/one
-No author: refs/tags/signed-tag
 A U Thor: refs/tags/three
 A U Thor: refs/tags/two
not ok 35 - check %(if)...%(then)...%(else)...%(end) atoms
#	
#		git for-each-ref --format="%(if)%(authorname)%(then)%(authorname)%(else)No
author%(end): %(refname)" >actual &&
#		cat >expect <<-\EOF &&
#		A U Thor: refs/heads/master
#		A U Thor: refs/heads/side
#		A U Thor: refs/odd/spot
#		No author: refs/tags/annotated-tag
#		No author: refs/tags/doubly-annotated-tag
#		No author: refs/tags/doubly-signed-tag
#		A U Thor: refs/tags/foo1.10
#		A U Thor: refs/tags/foo1.3
#		A U Thor: refs/tags/foo1.6
#		A U Thor: refs/tags/four
#		A U Thor: refs/tags/one
#		No author: refs/tags/signed-tag
#		A U Thor: refs/tags/three
#		A U Thor: refs/tags/two
#		EOF
#		test_cmp expect actual
#	

expecting success:
	git for-each-ref --format="%(refname:short): %(if)%(HEAD)%(then)Head
ref%(else)Not Head ref%(end)" >actual &&
	cat >expect <<-\EOF &&
	master: Head ref
	side: Not Head ref
	odd/spot: Not Head ref
	annotated-tag: Not Head ref
	doubly-annotated-tag: Not Head ref
	doubly-signed-tag: Not Head ref
	foo1.10: Not Head ref
	foo1.3: Not Head ref
	foo1.6: Not Head ref
	four: Not Head ref
	one: Not Head ref
	signed-tag: Not Head ref
	three: Not Head ref
	two: Not Head ref
	EOF
	test_cmp expect actual

--- expect	2016-05-07 16:08:32.000000000 +0000
+++ actual	2016-05-07 16:08:32.000000000 +0000
@@ -3,12 +3,10 @@
 odd/spot: Not Head ref
 annotated-tag: Not Head ref
 doubly-annotated-tag: Not Head ref
-doubly-signed-tag: Not Head ref
 foo1.10: Not Head ref
 foo1.3: Not Head ref
 foo1.6: Not Head ref
 four: Not Head ref
 one: Not Head ref
-signed-tag: Not Head ref
 three: Not Head ref
 two: Not Head ref
not ok 36 - ignore spaces in %(if) atom usage
#	
#		git for-each-ref --format="%(refname:short): %(if)%(HEAD)%(then)Head
ref%(else)Not Head ref%(end)" >actual &&
#		cat >expect <<-\EOF &&
#		master: Head ref
#		side: Not Head ref
#		odd/spot: Not Head ref
#		annotated-tag: Not Head ref
#		doubly-annotated-tag: Not Head ref
#		doubly-signed-tag: Not Head ref
#		foo1.10: Not Head ref
#		foo1.3: Not Head ref
#		foo1.6: Not Head ref
#		four: Not Head ref
#		one: Not Head ref
#		signed-tag: Not Head ref
#		three: Not Head ref
#		two: Not Head ref
#		EOF
#		test_cmp expect actual
#	

Re: t6392 broken on pu (Mac OS X)

[Jeff King]

On Sat, May 07, 2016 at 06:15:19PM +0200, Torsten Bögershausen wrote:

> These tests fail here under Mac OS,
> they pass under Linux:
> commit ff3d9b660a4b6e9d3eeb664ce1febe717adff737
> I haven't had a chance to dig further.

I assume you mean t6302. It looks like the difference is not Mac OS, but
rather that the GPG prerequisite is not fulfilled, so we are missing a
few of the tags.

Commit 618310a introduced a helper to munge the "expect" output. Using
that fixes some of the cases, but not test 34. That one is expecting
blank lines for tags, so test_prepare_expect doesn't know which lines
are related to GPG.

We could fix it by tweaking the test like this:

diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index 7420e48..04042e1 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -343,29 +343,27 @@ test_expect_success 'improper usage of %(if), %(then), %(else) and %(end) atoms'
 '
 
 test_expect_success 'check %(if)...%(then)...%(end) atoms' '
-	git for-each-ref --format="%(if)%(authorname)%(then)%(authorname): %(refname)%(end)" >actual &&
-	cat >expect <<-\EOF &&
-	A U Thor: refs/heads/master
-	A U Thor: refs/heads/side
-	A U Thor: refs/odd/spot
-
-
-
-	A U Thor: refs/tags/foo1.10
-	A U Thor: refs/tags/foo1.3
-	A U Thor: refs/tags/foo1.6
-	A U Thor: refs/tags/four
-	A U Thor: refs/tags/one
-
-	A U Thor: refs/tags/three
-	A U Thor: refs/tags/two
+	git for-each-ref --format="%(refname):%(if)%(authorname)%(then) author=%(authorname)%(end)" >actual &&
+	test_prepare_expect >expect <<-\EOF &&
+	refs/heads/master: author=A U Thor
+	refs/heads/side: author=A U Thor
+	refs/odd/spot: author=A U Thor
+	refs/tags/annotated-tag:
+	refs/tags/doubly-annotated-tag:
+	refs/tags/foo1.10: author=A U Thor
+	refs/tags/foo1.3: author=A U Thor
+	refs/tags/foo1.6: author=A U Thor
+	refs/tags/four: author=A U Thor
+	refs/tags/one: author=A U Thor
+	refs/tags/three: author=A U Thor
+	refs/tags/two: author=A U Thor
 	EOF
 	test_cmp expect actual
 '
 
 test_expect_success 'check %(if)...%(then)...%(else)...%(end) atoms' '
 	git for-each-ref --format="%(if)%(authorname)%(then)%(authorname)%(else)No author%(end): %(refname)" >actual &&
-	cat >expect <<-\EOF &&
+	test_prepare_expect >expect <<-\EOF &&
 	A U Thor: refs/heads/master
 	A U Thor: refs/heads/side
 	A U Thor: refs/odd/spot
@@ -385,7 +383,7 @@ test_expect_success 'check %(if)...%(then)...%(else)...%(end) atoms' '
 '
 test_expect_success 'ignore spaces in %(if) atom usage' '
 	git for-each-ref --format="%(refname:short): %(if)%(HEAD)%(then)Head ref%(else)Not Head ref%(end)" >actual &&
-	cat >expect <<-\EOF &&
+	test_prepare_expect >expect <<-\EOF &&
 	master: Head ref
 	side: Not Head ref
 	odd/spot: Not Head ref


Though we'd perhaps want to tweak the subsequent tests to use the same
format, just to make things easier to read later.

However, I wonder if we could improve on the strategy in 618310a, and
simply create non-signed versions of the "signed" tags when GPG is not
available. That would make tests looking at the whole ref namespace
more consistent. And any tests which wanted to look specifically at the
signed attributes should be protected with the GPG prereq anyway (it
doesn't look like there are any currently, though).

I.e., something like:

diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index 7420e48..a3df472 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -6,12 +6,8 @@ test_description='test for-each-refs usage of ref-filter APIs'
 . "$TEST_DIRECTORY"/lib-gpg.sh
 
 test_prepare_expect () {
-	if test_have_prereq GPG
-	then
-		cat
-	else
-		sed '/signed/d'
-	fi
+	# XXX this could now go away entirely, and just use cat in each test
+	cat
 }
 
 test_expect_success 'setup some history and refs' '
@@ -24,9 +20,12 @@ test_expect_success 'setup some history and refs' '
 	git tag -m "Annonated doubly" doubly-annotated-tag annotated-tag &&
 	if test_have_prereq GPG
 	then
-		git tag -s -m "A signed tag" signed-tag &&
-		git tag -s -m "Signed doubly" doubly-signed-tag signed-tag
+		sign=-s
+	else
+		sign=
 	fi &&
+	git tag $sign -m "A signed tag" signed-tag &&
+	git tag $sign -m "Signed doubly" doubly-signed-tag signed-tag &&
 	git checkout master &&
 	git update-ref refs/odd/spot master
 '

-Peff

Re: t6392 broken on pu (Mac OS X)

[Eric Sunshine]

On Mon, May 9, 2016 at 12:07 PM, Jeff King <peff@peff.net> wrote:
> On Sat, May 07, 2016 at 06:15:19PM +0200, Torsten Bögershausen wrote:
>> These tests fail here under Mac OS,
>> they pass under Linux:
>> commit ff3d9b660a4b6e9d3eeb664ce1febe717adff737
>> I haven't had a chance to dig further.
>
> I assume you mean t6302. It looks like the difference is not Mac OS, but
> rather that the GPG prerequisite is not fulfilled, so we are missing a
> few of the tags.
>
> Commit 618310a introduced a helper to munge the "expect" output. Using
> that fixes some of the cases, but not test 34. That one is expecting
> blank lines for tags, so test_prepare_expect doesn't know which lines
> are related to GPG.
>
> We could fix it by tweaking the test like this:
> [...snip...]
> However, I wonder if we could improve on the strategy in 618310a, and
> simply create non-signed versions of the "signed" tags when GPG is not
> available. That would make tests looking at the whole ref namespace
> more consistent. And any tests which wanted to look specifically at the
> signed attributes should be protected with the GPG prereq anyway (it
> doesn't look like there are any currently, though).
>
> I.e., something like:
> [...snip...]
>  test_expect_success 'setup some history and refs' '
> @@ -24,9 +20,12 @@ test_expect_success 'setup some history and refs' '
>         git tag -m "Annonated doubly" doubly-annotated-tag annotated-tag &&
>         if test_have_prereq GPG
>         then
> -               git tag -s -m "A signed tag" signed-tag &&
> -               git tag -s -m "Signed doubly" doubly-signed-tag signed-tag
> +               sign=-s
> +       else
> +               sign=
>         fi &&
> +       git tag $sign -m "A signed tag" signed-tag &&
> +       git tag $sign -m "Signed doubly" doubly-signed-tag signed-tag &&
>         git checkout master &&
>         git update-ref refs/odd/spot master
>  '

The latter seems very preferable, though perhaps it could be made more
concise like this?

    sign=
    test_have_prereq GPG && sign=-s

(But that's a minor issue.)

[PATCH] t6302: simplify non-gpg cases

[Jeff King]

On Mon, May 09, 2016 at 12:30:43PM -0400, Eric Sunshine wrote:

> The latter seems very preferable, though perhaps it could be made more
> concise like this?
> 
>     sign=
>     test_have_prereq GPG && sign=-s
> 
> (But that's a minor issue.)

I agree that is nicer, but I wanted to keep the definition inside the
test_expect_success close to its point of use. And that means we to deal
with the existing &&-chain (you can get around it with a {} block, but
at that point you might as well if/then).

Since you as the author of 618310a seem to agree with this direction,
here it is as a real patch.

-- >8 --
Subject: [PATCH] t6302: simplify non-gpg cases

When commit 618310a taught t6302 to run without the GPG
prerequisite, it did so by conditionally creating the signed
tags only when gpg is available. As a result, further tests
need to take this into account, which they can do with the
test_prepare_expect helper. This is a minor hassle, though,
as the helper cannot easily cover all cases (it just matches
"signed" in the output, so all output must include the
actual refname).

Instead, let's take a different approach. We'll always
create the tags, and only conditionally sign them. This does
mean our tag-names are a minor lie, but it lets the tests
which do not care about signing easily behave the same in
all settings. We'll include a comment to document our lie
and avoid confusing further test-writers.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t6302-for-each-ref-filter.sh | 45 +++++++++++++++++++++---------------------
 1 file changed, 22 insertions(+), 23 deletions(-)

diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index 70afb44..3225a0b 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -5,15 +5,6 @@ test_description='test for-each-refs usage of ref-filter APIs'
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-gpg.sh
 
-test_prepare_expect () {
-	if test_have_prereq GPG
-	then
-		cat
-	else
-		sed '/signed/d'
-	fi
-}
-
 test_expect_success 'setup some history and refs' '
 	test_commit one &&
 	test_commit two &&
@@ -22,11 +13,19 @@ test_expect_success 'setup some history and refs' '
 	test_commit four &&
 	git tag -m "An annotated tag" annotated-tag &&
 	git tag -m "Annonated doubly" doubly-annotated-tag annotated-tag &&
+
+	# Note that these "signed" tags might not actually be signed.
+	# Tests which care about the distinction should be marked
+	# with the GPG prereq.
 	if test_have_prereq GPG
 	then
-		git tag -s -m "A signed tag" signed-tag &&
-		git tag -s -m "Signed doubly" doubly-signed-tag signed-tag
+		sign=-s
+	else
+		sign=
 	fi &&
+	git tag $sign -m "A signed tag" signed-tag &&
+	git tag $sign -m "Signed doubly" doubly-signed-tag signed-tag &&
+
 	git checkout master &&
 	git update-ref refs/odd/spot master
 '
@@ -42,7 +41,7 @@ test_expect_success 'filtering with --points-at' '
 '
 
 test_expect_success 'check signed tags with --points-at' '
-	test_prepare_expect <<-\EOF | sed -e "s/Z$//" >expect &&
+	cat <<-\EOF | sed -e "s/Z$//" >expect &&
 	refs/heads/side Z
 	refs/tags/annotated-tag four
 	refs/tags/four Z
@@ -65,7 +64,7 @@ test_expect_success 'filtering with --merged' '
 '
 
 test_expect_success 'filtering with --no-merged' '
-	test_prepare_expect >expect <<-\EOF &&
+	cat >expect <<-\EOF &&
 	refs/heads/side
 	refs/tags/annotated-tag
 	refs/tags/doubly-annotated-tag
@@ -78,7 +77,7 @@ test_expect_success 'filtering with --no-merged' '
 '
 
 test_expect_success 'filtering with --contains' '
-	test_prepare_expect >expect <<-\EOF &&
+	cat >expect <<-\EOF &&
 	refs/heads/master
 	refs/heads/side
 	refs/odd/spot
@@ -99,7 +98,7 @@ test_expect_success '%(color) must fail' '
 '
 
 test_expect_success 'left alignment is default' '
-	test_prepare_expect >expect <<-\EOF &&
+	cat >expect <<-\EOF &&
 	refname is refs/heads/master  |refs/heads/master
 	refname is refs/heads/side    |refs/heads/side
 	refname is refs/odd/spot      |refs/odd/spot
@@ -117,7 +116,7 @@ test_expect_success 'left alignment is default' '
 '
 
 test_expect_success 'middle alignment' '
-	test_prepare_expect >expect <<-\EOF &&
+	cat >expect <<-\EOF &&
 	| refname is refs/heads/master |refs/heads/master
 	|  refname is refs/heads/side  |refs/heads/side
 	|   refname is refs/odd/spot   |refs/odd/spot
@@ -135,7 +134,7 @@ test_expect_success 'middle alignment' '
 '
 
 test_expect_success 'right alignment' '
-	test_prepare_expect >expect <<-\EOF &&
+	cat >expect <<-\EOF &&
 	|  refname is refs/heads/master|refs/heads/master
 	|    refname is refs/heads/side|refs/heads/side
 	|      refname is refs/odd/spot|refs/odd/spot
@@ -152,7 +151,7 @@ test_expect_success 'right alignment' '
 	test_cmp expect actual
 '
 
-test_prepare_expect >expect <<-\EOF
+cat >expect <<-\EOF
 |       refname is refs/heads/master       |refs/heads/master
 |        refname is refs/heads/side        |refs/heads/side
 |         refname is refs/odd/spot         |refs/odd/spot
@@ -199,7 +198,7 @@ EOF
 # Individual atoms inside %(align:...) and %(end) must not be quoted.
 
 test_expect_success 'alignment with format quote' "
-	test_prepare_expect >expect <<-\EOF &&
+	cat >expect <<-\EOF &&
 	|'      '\''master| A U Thor'\''      '|
 	|'       '\''side| A U Thor'\''       '|
 	|'     '\''odd/spot| A U Thor'\''     '|
@@ -217,7 +216,7 @@ test_expect_success 'alignment with format quote' "
 "
 
 test_expect_success 'nested alignment with quote formatting' "
-	test_prepare_expect >expect <<-\EOF &&
+	cat >expect <<-\EOF &&
 	|'         master               '|
 	|'           side               '|
 	|'       odd/spot               '|
@@ -235,7 +234,7 @@ test_expect_success 'nested alignment with quote formatting' "
 "
 
 test_expect_success 'check `%(contents:lines=1)`' '
-	test_prepare_expect >expect <<-\EOF &&
+	cat >expect <<-\EOF &&
 	master |three
 	side |four
 	odd/spot |three
@@ -253,7 +252,7 @@ test_expect_success 'check `%(contents:lines=1)`' '
 '
 
 test_expect_success 'check `%(contents:lines=0)`' '
-	test_prepare_expect >expect <<-\EOF &&
+	cat >expect <<-\EOF &&
 	master |
 	side |
 	odd/spot |
@@ -271,7 +270,7 @@ test_expect_success 'check `%(contents:lines=0)`' '
 '
 
 test_expect_success 'check `%(contents:lines=99999)`' '
-	test_prepare_expect >expect <<-\EOF &&
+	cat >expect <<-\EOF &&
 	master |three
 	side |four
 	odd/spot |three
-- 
2.8.2.643.g361a07a

Re: [PATCH] t6302: simplify non-gpg cases

[Eric Sunshine]

On Mon, May 9, 2016 at 12:49 PM, Jeff King <peff@peff.net> wrote:
> On Mon, May 09, 2016 at 12:30:43PM -0400, Eric Sunshine wrote:
> Since you as the author of 618310a seem to agree with this direction,
> here it is as a real patch.

Thanks for working on this.

> Subject: [PATCH] t6302: simplify non-gpg cases
>
> When commit 618310a taught t6302 to run without the GPG

618310a (t6302: skip only signed tags rather than all tests when GPG
is missing, 2016-03-06)

> prerequisite, it did so by conditionally creating the signed
> tags only when gpg is available. As a result, further tests
> need to take this into account, which they can do with the
> test_prepare_expect helper. This is a minor hassle, though,
> as the helper cannot easily cover all cases (it just matches
> "signed" in the output, so all output must include the
> actual refname).

Should we cite bc9acea (ref-filter: implement %(if), %(then), and
%(else) atoms, 2016-04-25) here as an example of a commit for which
this was problematic (and which indeed broke the tests when GPG is
unavailable)?

> Instead, let's take a different approach. We'll always
> create the tags, and only conditionally sign them. This does
> mean our tag-names are a minor lie, but it lets the tests
> which do not care about signing easily behave the same in
> all settings. We'll include a comment to document our lie
> and avoid confusing further test-writers.
>
> Signed-off-by: Jeff King <peff@peff.net>

Looks good. With or without the minor change below, this patch is:

    Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>

> ---
> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
>  test_expect_success 'check signed tags with --points-at' '
> -       test_prepare_expect <<-\EOF | sed -e "s/Z$//" >expect &&
> +       cat <<-\EOF | sed -e "s/Z$//" >expect &&

To make this as close to a reversion as possible, this could be
restored to the original (sans 'cat'):

    sed -e "s/Z$//" >expect <<-\EOF &&

>         refs/heads/side Z
>         refs/tags/annotated-tag four
>         refs/tags/four Z

Re: [PATCH] t6302: simplify non-gpg cases

[Eric Sunshine]

On Mon, May 9, 2016 at 4:24 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> On Mon, May 9, 2016 at 11:17 PM, Eric Sunshine <sunshine@sunshineco.com>
> wrote:
>> Should we cite bc9acea (ref-filter: implement %(if), %(then), and
>> %(else) atoms, 2016-04-25) here as an example of a commit for which
>> this was problematic (and which indeed broke the tests when GPG is
>> unavailable)?
>
> But it's still in pu and I'll be re-rolling it, would that be acceptable?

Ah right, therefore, no reason to cite that particular commit. Peff's
description is fine as-is.

[PATCH v2] t6302: simplify non-gpg cases

[Jeff King]

[+cc Junio as this should be the final version]

On Mon, May 09, 2016 at 01:47:35PM -0400, Eric Sunshine wrote:

> > Subject: [PATCH] t6302: simplify non-gpg cases
> >
> > When commit 618310a taught t6302 to run without the GPG
> 
> 618310a (t6302: skip only signed tags rather than all tests when GPG
> is missing, 2016-03-06)

I sometimes intentionally avoid using that longer form when the title of
the commit does not convey what I want to communicate, and I have to
summarize the change in my own words anyway (in this case the
interesting thing is not _what_ it did, but _how_ it chose to do it). So
I find including the original subject line just bloats the sentence and
makes the point harder to find.

But I'm curious whether other people run into that problem, or if
readers would prefer an unconditional full-citation. If I were writing
a book, I would probably footnote a case like this (to give extra
context if somebody wants it, but not break the flow of the text). But
"618310a" is already a footnote of sorts. So I dunno.

> Should we cite bc9acea (ref-filter: implement %(if), %(then), and
> %(else) atoms, 2016-04-25) here as an example of a commit for which
> this was problematic (and which indeed broke the tests when GPG is
> unavailable)?

Nope, as Karthik mentioned, we don't know the sha1 of that commit yet.
:(

> Looks good. With or without the minor change below, this patch is:
> 
>     Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>

Thanks.

> > -       test_prepare_expect <<-\EOF | sed -e "s/Z$//" >expect &&
> > +       cat <<-\EOF | sed -e "s/Z$//" >expect &&
> 
> To make this as close to a reversion as possible, this could be
> restored to the original (sans 'cat'):
> 
>     sed -e "s/Z$//" >expect <<-\EOF &&

Thanks, I did the reversion with s/test_prepare_expect/cat/ rather than
reverting 618310a, but I agree dropping this useless-use-of-cat is worth
doing. Here's v2 with that change and your reviewed-by.

-- >8 --
Subject: t6302: simplify non-gpg cases

When commit 618310a taught t6302 to run without the GPG
prerequisite, it did so by conditionally creating the signed
tags only when gpg is available. As a result, further tests
need to take this into account, which they can do with the
test_prepare_expect helper. This is a minor hassle, though,
as the helper cannot easily cover all cases (it just matches
"signed" in the output, so all output must include the
actual refname).

Instead, let's take a different approach. We'll always
create the tags, and only conditionally sign them. This does
mean our tag-names are a minor lie, but it lets the tests
which do not care about signing easily behave the same in
all settings. We'll include a comment to document our lie
and avoid confusing further test-writers.

Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 t/t6302-for-each-ref-filter.sh | 45 +++++++++++++++++++++---------------------
 1 file changed, 22 insertions(+), 23 deletions(-)

diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index 70afb44..d0ab09f 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -5,15 +5,6 @@ test_description='test for-each-refs usage of ref-filter APIs'
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-gpg.sh
 
-test_prepare_expect () {
-	if test_have_prereq GPG
-	then
-		cat
-	else
-		sed '/signed/d'
-	fi
-}
-
 test_expect_success 'setup some history and refs' '
 	test_commit one &&
 	test_commit two &&
@@ -22,11 +13,19 @@ test_expect_success 'setup some history and refs' '
 	test_commit four &&
 	git tag -m "An annotated tag" annotated-tag &&
 	git tag -m "Annonated doubly" doubly-annotated-tag annotated-tag &&
+
+	# Note that these "signed" tags might not actually be signed.
+	# Tests which care about the distinction should be marked
+	# with the GPG prereq.
 	if test_have_prereq GPG
 	then
-		git tag -s -m "A signed tag" signed-tag &&
-		git tag -s -m "Signed doubly" doubly-signed-tag signed-tag
+		sign=-s
+	else
+		sign=
 	fi &&
+	git tag $sign -m "A signed tag" signed-tag &&
+	git tag $sign -m "Signed doubly" doubly-signed-tag signed-tag &&
+
 	git checkout master &&
 	git update-ref refs/odd/spot master
 '
@@ -42,7 +41,7 @@ test_expect_success 'filtering with --points-at' '
 '
 
 test_expect_success 'check signed tags with --points-at' '
-	test_prepare_expect <<-\EOF | sed -e "s/Z$//" >expect &&
+	sed -e "s/Z$//" >expect <<-\EOF &&
 	refs/heads/side Z
 	refs/tags/annotated-tag four
 	refs/tags/four Z
@@ -65,7 +64,7 @@ test_expect_success 'filtering with --merged' '
 '
 
 test_expect_success 'filtering with --no-merged' '
-	test_prepare_expect >expect <<-\EOF &&
+	cat >expect <<-\EOF &&
 	refs/heads/side
 	refs/tags/annotated-tag
 	refs/tags/doubly-annotated-tag
@@ -78,7 +77,7 @@ test_expect_success 'filtering with --no-merged' '
 '
 
 test_expect_success 'filtering with --contains' '
-	test_prepare_expect >expect <<-\EOF &&
+	cat >expect <<-\EOF &&
 	refs/heads/master
 	refs/heads/side
 	refs/odd/spot
@@ -99,7 +98,7 @@ test_expect_success '%(color) must fail' '
 '
 
 test_expect_success 'left alignment is default' '
-	test_prepare_expect >expect <<-\EOF &&
+	cat >expect <<-\EOF &&
 	refname is refs/heads/master  |refs/heads/master
 	refname is refs/heads/side    |refs/heads/side
 	refname is refs/odd/spot      |refs/odd/spot
@@ -117,7 +116,7 @@ test_expect_success 'left alignment is default' '
 '
 
 test_expect_success 'middle alignment' '
-	test_prepare_expect >expect <<-\EOF &&
+	cat >expect <<-\EOF &&
 	| refname is refs/heads/master |refs/heads/master
 	|  refname is refs/heads/side  |refs/heads/side
 	|   refname is refs/odd/spot   |refs/odd/spot
@@ -135,7 +134,7 @@ test_expect_success 'middle alignment' '
 '
 
 test_expect_success 'right alignment' '
-	test_prepare_expect >expect <<-\EOF &&
+	cat >expect <<-\EOF &&
 	|  refname is refs/heads/master|refs/heads/master
 	|    refname is refs/heads/side|refs/heads/side
 	|      refname is refs/odd/spot|refs/odd/spot
@@ -152,7 +151,7 @@ test_expect_success 'right alignment' '
 	test_cmp expect actual
 '
 
-test_prepare_expect >expect <<-\EOF
+cat >expect <<-\EOF
 |       refname is refs/heads/master       |refs/heads/master
 |        refname is refs/heads/side        |refs/heads/side
 |         refname is refs/odd/spot         |refs/odd/spot
@@ -199,7 +198,7 @@ EOF
 # Individual atoms inside %(align:...) and %(end) must not be quoted.
 
 test_expect_success 'alignment with format quote' "
-	test_prepare_expect >expect <<-\EOF &&
+	cat >expect <<-\EOF &&
 	|'      '\''master| A U Thor'\''      '|
 	|'       '\''side| A U Thor'\''       '|
 	|'     '\''odd/spot| A U Thor'\''     '|
@@ -217,7 +216,7 @@ test_expect_success 'alignment with format quote' "
 "
 
 test_expect_success 'nested alignment with quote formatting' "
-	test_prepare_expect >expect <<-\EOF &&
+	cat >expect <<-\EOF &&
 	|'         master               '|
 	|'           side               '|
 	|'       odd/spot               '|
@@ -235,7 +234,7 @@ test_expect_success 'nested alignment with quote formatting' "
 "
 
 test_expect_success 'check `%(contents:lines=1)`' '
-	test_prepare_expect >expect <<-\EOF &&
+	cat >expect <<-\EOF &&
 	master |three
 	side |four
 	odd/spot |three
@@ -253,7 +252,7 @@ test_expect_success 'check `%(contents:lines=1)`' '
 '
 
 test_expect_success 'check `%(contents:lines=0)`' '
-	test_prepare_expect >expect <<-\EOF &&
+	cat >expect <<-\EOF &&
 	master |
 	side |
 	odd/spot |
@@ -271,7 +270,7 @@ test_expect_success 'check `%(contents:lines=0)`' '
 '
 
 test_expect_success 'check `%(contents:lines=99999)`' '
-	test_prepare_expect >expect <<-\EOF &&
+	cat >expect <<-\EOF &&
 	master |three
 	side |four
 	odd/spot |three
-- 
2.8.2.643.g361a07a

Re: [PATCH v2] t6302: simplify non-gpg cases

[Eric Sunshine]

On Mon, May 9, 2016 at 10:40 PM, Jeff King <peff@peff.net> wrote:
> On Mon, May 09, 2016 at 01:47:35PM -0400, Eric Sunshine wrote:
>> > -       test_prepare_expect <<-\EOF | sed -e "s/Z$//" >expect &&
>> > +       cat <<-\EOF | sed -e "s/Z$//" >expect &&
>>
>> To make this as close to a reversion as possible, this could be
>> restored to the original (sans 'cat'):
>>
>>     sed -e "s/Z$//" >expect <<-\EOF &&
>
> Thanks, I did the reversion with s/test_prepare_expect/cat/ rather than
> reverting 618310a, but I agree dropping this useless-use-of-cat is worth
> doing. Here's v2 with that change and your reviewed-by.

Looks good, thanks.

Re: [PATCH v2] t6302: simplify non-gpg cases

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> [+cc Junio as this should be the final version]

Thanks, I think I queued with "do not cat a single file to a pipe"
tweak already.

>> > When commit 618310a taught t6302 to run without the GPG
>> 
>> 618310a (t6302: skip only signed tags rather than all tests when GPG
>> is missing, 2016-03-06)
>
> I sometimes intentionally avoid using that longer form when the title of
> the commit does not convey what I want to communicate, and I have to
> summarize the change in my own words anyway (in this case the
> interesting thing is not _what_ it did, but _how_ it chose to do it). So
> I find including the original subject line just bloats the sentence and
> makes the point harder to find.
>
> But I'm curious whether other people run into that problem, or if
> readers would prefer an unconditional full-citation.

Personally, I find your version better in this case, simply because,
as you said, the focus is different, and because the readers
familiar with the recent history can still tell from your
description which commit you are talking about without resorting to
"git show 618310a".  The only thing we are losing is the datestamp,
which is more relevant when referring to a commit in more distant
past.  But in general, not everybody writes a good log message like
you do, so if they try to imitate what you did above, the end result
is likely to end up being a cryptic mess that does not help identify
which commit they are talking about. For that reason, I am a bit
hesitant to say everybody should omit the original when they (think
they) do their own rephrasing.

> Thanks, I did the reversion with s/test_prepare_expect/cat/ rather than
> reverting 618310a, but I agree dropping this useless-use-of-cat is worth
> doing. Here's v2 with that change and your reviewed-by.