* t6392 broken on pu (Mac OS X) @ 2016-05-07 16:15 Torsten Bögershausen 2016-05-09 16:07 ` Jeff King 0 siblings, 1 reply; 9+ messages in thread From: Torsten Bögershausen @ 2016-05-07 16:15 UTC (permalink / raw) To: Karthik Nayak, Git Mailing List 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 # ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: t6392 broken on pu (Mac OS X) 2016-05-07 16:15 t6392 broken on pu (Mac OS X) Torsten Bögershausen @ 2016-05-09 16:07 ` Jeff King 2016-05-09 16:30 ` Eric Sunshine 0 siblings, 1 reply; 9+ messages in thread From: Jeff King @ 2016-05-09 16:07 UTC (permalink / raw) To: Torsten Bögershausen; +Cc: Eric Sunshine, Karthik Nayak, Git Mailing List 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: t6392 broken on pu (Mac OS X) 2016-05-09 16:07 ` Jeff King @ 2016-05-09 16:30 ` Eric Sunshine 2016-05-09 16:49 ` [PATCH] t6302: simplify non-gpg cases Jeff King 0 siblings, 1 reply; 9+ messages in thread From: Eric Sunshine @ 2016-05-09 16:30 UTC (permalink / raw) To: Jeff King; +Cc: Torsten Bögershausen, Karthik Nayak, Git Mailing List 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.) ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] t6302: simplify non-gpg cases 2016-05-09 16:30 ` Eric Sunshine @ 2016-05-09 16:49 ` Jeff King 2016-05-09 17:47 ` Eric Sunshine 0 siblings, 1 reply; 9+ messages in thread From: Jeff King @ 2016-05-09 16:49 UTC (permalink / raw) To: Eric Sunshine; +Cc: Torsten Bögershausen, Karthik Nayak, Git Mailing List 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] t6302: simplify non-gpg cases 2016-05-09 16:49 ` [PATCH] t6302: simplify non-gpg cases Jeff King @ 2016-05-09 17:47 ` Eric Sunshine [not found] ` <CAOLa=ZSZqs=++Hf8CF3pWEnJqmOA9ajuX03hzLMkuQ+ehXXCVQ@mail.gmail.com> 2016-05-10 2:40 ` [PATCH v2] " Jeff King 0 siblings, 2 replies; 9+ messages in thread From: Eric Sunshine @ 2016-05-09 17:47 UTC (permalink / raw) To: Jeff King; +Cc: Torsten Bögershausen, Karthik Nayak, Git Mailing List 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
[parent not found: <CAOLa=ZSZqs=++Hf8CF3pWEnJqmOA9ajuX03hzLMkuQ+ehXXCVQ@mail.gmail.com>]
* Re: [PATCH] t6302: simplify non-gpg cases [not found] ` <CAOLa=ZSZqs=++Hf8CF3pWEnJqmOA9ajuX03hzLMkuQ+ehXXCVQ@mail.gmail.com> @ 2016-05-09 21:37 ` Eric Sunshine 0 siblings, 0 replies; 9+ messages in thread From: Eric Sunshine @ 2016-05-09 21:37 UTC (permalink / raw) To: Karthik Nayak; +Cc: Jeff King, Torsten Bögershausen, Git Mailing List 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] t6302: simplify non-gpg cases 2016-05-09 17:47 ` Eric Sunshine [not found] ` <CAOLa=ZSZqs=++Hf8CF3pWEnJqmOA9ajuX03hzLMkuQ+ehXXCVQ@mail.gmail.com> @ 2016-05-10 2:40 ` Jeff King 2016-05-10 2:49 ` Eric Sunshine 2016-05-10 6:03 ` Junio C Hamano 1 sibling, 2 replies; 9+ messages in thread From: Jeff King @ 2016-05-10 2:40 UTC (permalink / raw) To: Eric Sunshine Cc: Junio C Hamano, Torsten Bögershausen, Karthik Nayak, Git Mailing List [+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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] t6302: simplify non-gpg cases 2016-05-10 2:40 ` [PATCH v2] " Jeff King @ 2016-05-10 2:49 ` Eric Sunshine 2016-05-10 6:03 ` Junio C Hamano 1 sibling, 0 replies; 9+ messages in thread From: Eric Sunshine @ 2016-05-10 2:49 UTC (permalink / raw) To: Jeff King Cc: Junio C Hamano, Torsten Bögershausen, Karthik Nayak, Git Mailing List 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] t6302: simplify non-gpg cases 2016-05-10 2:40 ` [PATCH v2] " Jeff King 2016-05-10 2:49 ` Eric Sunshine @ 2016-05-10 6:03 ` Junio C Hamano 1 sibling, 0 replies; 9+ messages in thread From: Junio C Hamano @ 2016-05-10 6:03 UTC (permalink / raw) To: Jeff King Cc: Eric Sunshine, Torsten Bögershausen, Karthik Nayak, Git Mailing List 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-05-10 6:03 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-05-07 16:15 t6392 broken on pu (Mac OS X) Torsten Bögershausen
2016-05-09 16:07 ` Jeff King
2016-05-09 16:30 ` Eric Sunshine
2016-05-09 16:49 ` [PATCH] t6302: simplify non-gpg cases Jeff King
2016-05-09 17:47 ` Eric Sunshine
[not found] ` <CAOLa=ZSZqs=++Hf8CF3pWEnJqmOA9ajuX03hzLMkuQ+ehXXCVQ@mail.gmail.com>
2016-05-09 21:37 ` Eric Sunshine
2016-05-10 2:40 ` [PATCH v2] " Jeff King
2016-05-10 2:49 ` Eric Sunshine
2016-05-10 6:03 ` Junio C Hamano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).