* EOF test fixes (t5615/t7004) @ 2017-03-22 17:35 Jan Palus 2017-03-22 18:28 ` EOF test fixes (t7030/t7406) Jan Palus ` (3 more replies) 0 siblings, 4 replies; 46+ messages in thread From: Jan Palus @ 2017-03-22 17:35 UTC (permalink / raw) To: git [-- 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 ' ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: EOF test fixes (t7030/t7406) 2017-03-22 17:35 EOF test fixes (t5615/t7004) Jan Palus @ 2017-03-22 18:28 ` Jan Palus 2017-03-22 18:47 ` EOF test fixes (t5615/t7004) Stefan Beller ` (2 subsequent siblings) 3 siblings, 0 replies; 46+ messages in thread From: Jan Palus @ 2017-03-22 18:28 UTC (permalink / raw) To: git [-- 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 && ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: EOF test fixes (t5615/t7004) 2017-03-22 17:35 EOF test fixes (t5615/t7004) Jan Palus 2017-03-22 18:28 ` EOF test fixes (t7030/t7406) Jan Palus @ 2017-03-22 18:47 ` Stefan Beller 2017-03-22 19:43 ` Junio C Hamano 2017-03-22 20:08 ` [PATCH 0/3] fix "here-doc" syntax errors Junio C Hamano 3 siblings, 0 replies; 46+ messages in thread From: Stefan Beller @ 2017-03-22 18:47 UTC (permalink / raw) To: Jan Palus; +Cc: git@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: EOF test fixes (t5615/t7004) 2017-03-22 17:35 EOF test fixes (t5615/t7004) Jan Palus 2017-03-22 18:28 ` EOF test fixes (t7030/t7406) Jan Palus 2017-03-22 18:47 ` EOF test fixes (t5615/t7004) Stefan Beller @ 2017-03-22 19:43 ` Junio C Hamano 2017-03-22 20:08 ` [PATCH 0/3] fix "here-doc" syntax errors Junio C Hamano 3 siblings, 0 replies; 46+ messages in thread From: Junio C Hamano @ 2017-03-22 19:43 UTC (permalink / raw) To: Jan Palus; +Cc: git 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. ^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH 0/3] fix "here-doc" syntax errors 2017-03-22 17:35 EOF test fixes (t5615/t7004) Jan Palus ` (2 preceding siblings ...) 2017-03-22 19:43 ` Junio C Hamano @ 2017-03-22 20:08 ` Junio C Hamano 2017-03-22 20:08 ` [PATCH 1/3] t5615: fix a here-doc syntax error Junio C Hamano ` (4 more replies) 3 siblings, 5 replies; 46+ messages in thread From: Junio C Hamano @ 2017-03-22 20:08 UTC (permalink / raw) To: git; +Cc: Jan Palus 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH 1/3] t5615: fix a here-doc syntax error 2017-03-22 20:08 ` [PATCH 0/3] fix "here-doc" syntax errors Junio C Hamano @ 2017-03-22 20:08 ` Junio C Hamano 2017-03-22 21:02 ` Jeff King 2017-03-22 20:08 ` [PATCH 2/3] t7406: fix here-doc syntax errors Junio C Hamano ` (3 subsequent siblings) 4 siblings, 1 reply; 46+ messages in thread From: Junio C Hamano @ 2017-03-22 20:08 UTC (permalink / raw) To: git; +Cc: Jan Palus 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 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [PATCH 1/3] t5615: fix a here-doc syntax error 2017-03-22 20:08 ` [PATCH 1/3] t5615: fix a here-doc syntax error Junio C Hamano @ 2017-03-22 21:02 ` Jeff King 0 siblings, 0 replies; 46+ messages in thread From: Jeff King @ 2017-03-22 21:02 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jan Palus 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH 2/3] t7406: fix here-doc syntax errors 2017-03-22 20:08 ` [PATCH 0/3] fix "here-doc" syntax errors Junio C Hamano 2017-03-22 20:08 ` [PATCH 1/3] t5615: fix a here-doc syntax error Junio C Hamano @ 2017-03-22 20:08 ` Junio C Hamano 2017-03-22 21:07 ` Jeff King 2017-03-22 20:08 ` [PATCH 3/3] t7004, t7030: " Junio C Hamano ` (2 subsequent siblings) 4 siblings, 1 reply; 46+ messages in thread From: Junio C Hamano @ 2017-03-22 20:08 UTC (permalink / raw) To: git; +Cc: Jan Palus 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 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [PATCH 2/3] t7406: fix here-doc syntax errors 2017-03-22 20:08 ` [PATCH 2/3] t7406: fix here-doc syntax errors Junio C Hamano @ 2017-03-22 21:07 ` Jeff King 2017-03-22 21:32 ` Stefan Beller 2017-03-22 21:34 ` [PATCH 2/3] t7406: fix here-doc syntax errors Junio C Hamano 0 siblings, 2 replies; 46+ messages in thread From: Jeff King @ 2017-03-22 21:07 UTC (permalink / raw) To: Junio C Hamano; +Cc: Stefan Beller, git, Jan Palus 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 2/3] t7406: fix here-doc syntax errors 2017-03-22 21:07 ` Jeff King @ 2017-03-22 21:32 ` Stefan Beller 2017-03-22 21:39 ` Jeff King 2017-03-22 21:34 ` [PATCH 2/3] t7406: fix here-doc syntax errors Junio C Hamano 1 sibling, 1 reply; 46+ messages in thread From: Stefan Beller @ 2017-03-22 21:32 UTC (permalink / raw) To: Jeff King; +Cc: Junio C Hamano, git@vger.kernel.org, Jan Palus 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 2/3] t7406: fix here-doc syntax errors 2017-03-22 21:32 ` Stefan Beller @ 2017-03-22 21:39 ` Jeff King 2017-03-22 21:49 ` [PATCH] t7406: correct test case for submodule-update initial population Stefan Beller 0 siblings, 1 reply; 46+ messages in thread From: Jeff King @ 2017-03-22 21:39 UTC (permalink / raw) To: Stefan Beller; +Cc: Junio C Hamano, git@vger.kernel.org, Jan Palus 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH] t7406: correct test case for submodule-update initial population 2017-03-22 21:39 ` Jeff King @ 2017-03-22 21:49 ` Stefan Beller 2017-03-22 21:59 ` Jeff King 0 siblings, 1 reply; 46+ messages in thread From: Stefan Beller @ 2017-03-22 21:49 UTC (permalink / raw) To: gitster; +Cc: git, peff, jan.palus, 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 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [PATCH] t7406: correct test case for submodule-update initial population 2017-03-22 21:49 ` [PATCH] t7406: correct test case for submodule-update initial population Stefan Beller @ 2017-03-22 21:59 ` Jeff King 2017-03-22 22:07 ` Stefan Beller 2017-03-22 22:12 ` Junio C Hamano 0 siblings, 2 replies; 46+ messages in thread From: Jeff King @ 2017-03-22 21:59 UTC (permalink / raw) To: Stefan Beller; +Cc: gitster, git, jan.palus 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH] t7406: correct test case for submodule-update initial population 2017-03-22 21:59 ` Jeff King @ 2017-03-22 22:07 ` Stefan Beller 2017-03-22 22:09 ` Jeff King 2017-03-22 22:14 ` Jonathan Nieder 2017-03-22 22:12 ` Junio C Hamano 1 sibling, 2 replies; 46+ messages in thread From: Stefan Beller @ 2017-03-22 22:07 UTC (permalink / raw) To: Jeff King; +Cc: Junio C Hamano, git@vger.kernel.org, Jan Palus 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH] t7406: correct test case for submodule-update initial population 2017-03-22 22:07 ` Stefan Beller @ 2017-03-22 22:09 ` Jeff King 2017-03-22 22:14 ` Jonathan Nieder 1 sibling, 0 replies; 46+ messages in thread From: Jeff King @ 2017-03-22 22:09 UTC (permalink / raw) To: Stefan Beller; +Cc: Junio C Hamano, git@vger.kernel.org, Jan Palus 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH] t7406: correct test case for submodule-update initial population 2017-03-22 22:07 ` Stefan Beller 2017-03-22 22:09 ` Jeff King @ 2017-03-22 22:14 ` Jonathan Nieder 1 sibling, 0 replies; 46+ messages in thread From: Jonathan Nieder @ 2017-03-22 22:14 UTC (permalink / raw) To: Stefan Beller; +Cc: Jeff King, Junio C Hamano, git@vger.kernel.org, Jan Palus 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH] t7406: correct test case for submodule-update initial population 2017-03-22 21:59 ` Jeff King 2017-03-22 22:07 ` Stefan Beller @ 2017-03-22 22:12 ` Junio C Hamano 2017-03-22 22:24 ` Jeff King 1 sibling, 1 reply; 46+ messages in thread From: Junio C Hamano @ 2017-03-22 22:12 UTC (permalink / raw) To: Jeff King; +Cc: Stefan Beller, git, jan.palus 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 ' ^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [PATCH] t7406: correct test case for submodule-update initial population 2017-03-22 22:12 ` Junio C Hamano @ 2017-03-22 22:24 ` Jeff King 2017-03-22 22:28 ` Stefan Beller 0 siblings, 1 reply; 46+ messages in thread From: Jeff King @ 2017-03-22 22:24 UTC (permalink / raw) To: Junio C Hamano; +Cc: Stefan Beller, git, jan.palus 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH] t7406: correct test case for submodule-update initial population 2017-03-22 22:24 ` Jeff King @ 2017-03-22 22:28 ` Stefan Beller 0 siblings, 0 replies; 46+ messages in thread From: Stefan Beller @ 2017-03-22 22:28 UTC (permalink / raw) To: Jeff King; +Cc: Junio C Hamano, git@vger.kernel.org, Jan Palus 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 2/3] t7406: fix here-doc syntax errors 2017-03-22 21:07 ` Jeff King 2017-03-22 21:32 ` Stefan Beller @ 2017-03-22 21:34 ` Junio C Hamano 1 sibling, 0 replies; 46+ messages in thread From: Junio C Hamano @ 2017-03-22 21:34 UTC (permalink / raw) To: Jeff King; +Cc: Stefan Beller, git, Jan Palus 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH 3/3] t7004, t7030: fix here-doc syntax errors 2017-03-22 20:08 ` [PATCH 0/3] fix "here-doc" syntax errors Junio C Hamano 2017-03-22 20:08 ` [PATCH 1/3] t5615: fix a here-doc syntax error Junio C Hamano 2017-03-22 20:08 ` [PATCH 2/3] t7406: fix here-doc syntax errors Junio C Hamano @ 2017-03-22 20:08 ` Junio C Hamano 2017-03-22 21:10 ` Jeff King 2017-03-22 22:40 ` [PATCH 0/3] fix "here-doc" " Jan Palus 2017-03-23 2:12 ` [PATCH] tests: lint for run-away here-doc Junio C Hamano 4 siblings, 1 reply; 46+ messages in thread From: Junio C Hamano @ 2017-03-22 20:08 UTC (permalink / raw) To: git; +Cc: Jan Palus 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 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors 2017-03-22 20:08 ` [PATCH 3/3] t7004, t7030: " Junio C Hamano @ 2017-03-22 21:10 ` Jeff King 2017-03-22 21:43 ` Santiago Torres 2017-03-22 22:04 ` Junio C Hamano 0 siblings, 2 replies; 46+ messages in thread From: Jeff King @ 2017-03-22 21:10 UTC (permalink / raw) To: Junio C Hamano; +Cc: Santiago Torres, git, Jan Palus 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors 2017-03-22 21:10 ` Jeff King @ 2017-03-22 21:43 ` Santiago Torres 2017-03-22 22:04 ` Jeff King 2017-03-22 22:04 ` Junio C Hamano 1 sibling, 1 reply; 46+ messages in thread From: Santiago Torres @ 2017-03-22 21:43 UTC (permalink / raw) To: Jeff King; +Cc: Junio C Hamano, git, Jan Palus [-- 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 --] ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors 2017-03-22 21:43 ` Santiago Torres @ 2017-03-22 22:04 ` Jeff King 0 siblings, 0 replies; 46+ messages in thread From: Jeff King @ 2017-03-22 22:04 UTC (permalink / raw) To: Santiago Torres; +Cc: Junio C Hamano, git, Jan Palus 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors 2017-03-22 21:10 ` Jeff King 2017-03-22 21:43 ` Santiago Torres @ 2017-03-22 22:04 ` Junio C Hamano 2017-03-22 22:15 ` Santiago Torres 1 sibling, 1 reply; 46+ messages in thread From: Junio C Hamano @ 2017-03-22 22:04 UTC (permalink / raw) To: Jeff King; +Cc: Santiago Torres, git, Jan Palus 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. ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors 2017-03-22 22:04 ` Junio C Hamano @ 2017-03-22 22:15 ` Santiago Torres 2017-03-22 22:22 ` Jeff King 0 siblings, 1 reply; 46+ messages in thread From: Santiago Torres @ 2017-03-22 22:15 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, git, Jan Palus [-- 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 --] ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors 2017-03-22 22:15 ` Santiago Torres @ 2017-03-22 22:22 ` Jeff King 2017-03-22 22:34 ` Santiago Torres 2017-03-22 22:38 ` Junio C Hamano 0 siblings, 2 replies; 46+ messages in thread From: Jeff King @ 2017-03-22 22:22 UTC (permalink / raw) To: Santiago Torres; +Cc: Junio C Hamano, git, Jan Palus 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 ' ^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors 2017-03-22 22:22 ` Jeff King @ 2017-03-22 22:34 ` Santiago Torres 2017-03-22 22:41 ` Jeff King 2017-03-22 22:38 ` Junio C Hamano 1 sibling, 1 reply; 46+ messages in thread From: Santiago Torres @ 2017-03-22 22:34 UTC (permalink / raw) To: Jeff King; +Cc: Junio C Hamano, git, Jan Palus [-- 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 --] ^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors 2017-03-22 22:34 ` Santiago Torres @ 2017-03-22 22:41 ` Jeff King 2017-03-22 22:47 ` Junio C Hamano 2017-03-22 22:51 ` Santiago Torres 0 siblings, 2 replies; 46+ messages in thread From: Jeff King @ 2017-03-22 22:41 UTC (permalink / raw) To: Santiago Torres; +Cc: Junio C Hamano, git, Jan Palus 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors 2017-03-22 22:41 ` Jeff King @ 2017-03-22 22:47 ` Junio C Hamano 2017-03-22 22:51 ` Santiago Torres 1 sibling, 0 replies; 46+ messages in thread From: Junio C Hamano @ 2017-03-22 22:47 UTC (permalink / raw) To: Jeff King; +Cc: Santiago Torres, git, Jan Palus 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. ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors 2017-03-22 22:41 ` Jeff King 2017-03-22 22:47 ` Junio C Hamano @ 2017-03-22 22:51 ` Santiago Torres 2017-03-23 22:00 ` Junio C Hamano 1 sibling, 1 reply; 46+ messages in thread From: Santiago Torres @ 2017-03-22 22:51 UTC (permalink / raw) To: Jeff King; +Cc: Junio C Hamano, git, Jan Palus [-- 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 --] ^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors 2017-03-22 22:51 ` Santiago Torres @ 2017-03-23 22:00 ` Junio C Hamano 2017-03-23 22:28 ` Santiago Torres 2017-03-23 23:49 ` Jeff King 0 siblings, 2 replies; 46+ messages in thread From: Junio C Hamano @ 2017-03-23 22:00 UTC (permalink / raw) To: Santiago Torres; +Cc: Jeff King, git, Jan Palus 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 ' ^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors 2017-03-23 22:00 ` Junio C Hamano @ 2017-03-23 22:28 ` Santiago Torres 2017-03-23 23:49 ` Jeff King 1 sibling, 0 replies; 46+ messages in thread From: Santiago Torres @ 2017-03-23 22:28 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, git, Jan Palus [-- 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 --] ^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors 2017-03-23 22:00 ` Junio C Hamano 2017-03-23 22:28 ` Santiago Torres @ 2017-03-23 23:49 ` Jeff King 2017-03-24 16:45 ` Junio C Hamano 1 sibling, 1 reply; 46+ messages in thread From: Jeff King @ 2017-03-23 23:49 UTC (permalink / raw) To: Junio C Hamano; +Cc: Santiago Torres, git, Jan Palus 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors 2017-03-23 23:49 ` Jeff King @ 2017-03-24 16:45 ` Junio C Hamano 2017-03-24 16:49 ` Jeff King 0 siblings, 1 reply; 46+ messages in thread From: Junio C Hamano @ 2017-03-24 16:45 UTC (permalink / raw) To: Jeff King; +Cc: Santiago Torres, git, Jan Palus 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. ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors 2017-03-24 16:45 ` Junio C Hamano @ 2017-03-24 16:49 ` Jeff King 2017-03-24 18:00 ` Jeff King 2017-03-24 18:04 ` Junio C Hamano 0 siblings, 2 replies; 46+ messages in thread From: Jeff King @ 2017-03-24 16:49 UTC (permalink / raw) To: Junio C Hamano; +Cc: Santiago Torres, git, Jan Palus 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors 2017-03-24 16:49 ` Jeff King @ 2017-03-24 18:00 ` Jeff King 2017-03-24 18:04 ` Junio C Hamano 1 sibling, 0 replies; 46+ messages in thread From: Jeff King @ 2017-03-24 18:00 UTC (permalink / raw) To: Junio C Hamano; +Cc: Santiago Torres, git, Jan Palus 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors 2017-03-24 16:49 ` Jeff King 2017-03-24 18:00 ` Jeff King @ 2017-03-24 18:04 ` Junio C Hamano 2017-03-24 18:16 ` Jeff King 1 sibling, 1 reply; 46+ messages in thread From: Junio C Hamano @ 2017-03-24 18:04 UTC (permalink / raw) To: Jeff King; +Cc: Santiago Torres, git, Jan Palus 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 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors 2017-03-24 18:04 ` Junio C Hamano @ 2017-03-24 18:16 ` Jeff King 0 siblings, 0 replies; 46+ messages in thread From: Jeff King @ 2017-03-24 18:16 UTC (permalink / raw) To: Junio C Hamano; +Cc: Santiago Torres, git, Jan Palus 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors 2017-03-22 22:22 ` Jeff King 2017-03-22 22:34 ` Santiago Torres @ 2017-03-22 22:38 ` Junio C Hamano 1 sibling, 0 replies; 46+ messages in thread From: Junio C Hamano @ 2017-03-22 22:38 UTC (permalink / raw) To: Jeff King; +Cc: Santiago Torres, git, Jan Palus 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. ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 0/3] fix "here-doc" syntax errors 2017-03-22 20:08 ` [PATCH 0/3] fix "here-doc" syntax errors Junio C Hamano ` (2 preceding siblings ...) 2017-03-22 20:08 ` [PATCH 3/3] t7004, t7030: " Junio C Hamano @ 2017-03-22 22:40 ` Jan Palus 2017-03-23 2:12 ` [PATCH] tests: lint for run-away here-doc Junio C Hamano 4 siblings, 0 replies; 46+ messages in thread From: Jan Palus @ 2017-03-22 22:40 UTC (permalink / raw) To: git; +Cc: Junio C Hamano 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH] tests: lint for run-away here-doc 2017-03-22 20:08 ` [PATCH 0/3] fix "here-doc" syntax errors Junio C Hamano ` (3 preceding siblings ...) 2017-03-22 22:40 ` [PATCH 0/3] fix "here-doc" " Jan Palus @ 2017-03-23 2:12 ` Junio C Hamano 2017-03-23 5:43 ` [PATCH v2] " Junio C Hamano 4 siblings, 1 reply; 46+ messages in thread From: Junio C Hamano @ 2017-03-23 2:12 UTC (permalink / raw) To: git 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 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [PATCH v2] tests: lint for run-away here-doc 2017-03-23 2:12 ` [PATCH] tests: lint for run-away here-doc Junio C Hamano @ 2017-03-23 5:43 ` Junio C Hamano 2017-03-24 1:29 ` Jonathan Nieder 2017-03-24 3:59 ` Jeff King 0 siblings, 2 replies; 46+ messages in thread From: Junio C Hamano @ 2017-03-23 5:43 UTC (permalink / raw) To: git 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 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [PATCH v2] tests: lint for run-away here-doc 2017-03-23 5:43 ` [PATCH v2] " Junio C Hamano @ 2017-03-24 1:29 ` Jonathan Nieder 2017-03-24 2:45 ` Junio C Hamano 2017-03-24 3:59 ` Jeff King 1 sibling, 1 reply; 46+ messages in thread From: Jonathan Nieder @ 2017-03-24 1:29 UTC (permalink / raw) To: Junio C Hamano; +Cc: git 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. ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v2] tests: lint for run-away here-doc 2017-03-24 1:29 ` Jonathan Nieder @ 2017-03-24 2:45 ` Junio C Hamano 0 siblings, 0 replies; 46+ messages in thread From: Junio C Hamano @ 2017-03-24 2:45 UTC (permalink / raw) To: Jonathan Nieder; +Cc: git 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. ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v2] tests: lint for run-away here-doc 2017-03-23 5:43 ` [PATCH v2] " Junio C Hamano 2017-03-24 1:29 ` Jonathan Nieder @ 2017-03-24 3:59 ` Jeff King 1 sibling, 0 replies; 46+ messages in thread From: Jeff King @ 2017-03-24 3:59 UTC (permalink / raw) To: Junio C Hamano; +Cc: git 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
end of thread, other threads:[~2017-03-24 18:16 UTC | newest] Thread overview: 46+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-03-22 17:35 EOF test fixes (t5615/t7004) Jan Palus 2017-03-22 18:28 ` EOF test fixes (t7030/t7406) Jan Palus 2017-03-22 18:47 ` EOF test fixes (t5615/t7004) Stefan Beller 2017-03-22 19:43 ` Junio C Hamano 2017-03-22 20:08 ` [PATCH 0/3] fix "here-doc" syntax errors Junio C Hamano 2017-03-22 20:08 ` [PATCH 1/3] t5615: fix a here-doc syntax error Junio C Hamano 2017-03-22 21:02 ` Jeff King 2017-03-22 20:08 ` [PATCH 2/3] t7406: fix here-doc syntax errors Junio C Hamano 2017-03-22 21:07 ` Jeff King 2017-03-22 21:32 ` Stefan Beller 2017-03-22 21:39 ` Jeff King 2017-03-22 21:49 ` [PATCH] t7406: correct test case for submodule-update initial population Stefan Beller 2017-03-22 21:59 ` Jeff King 2017-03-22 22:07 ` Stefan Beller 2017-03-22 22:09 ` Jeff King 2017-03-22 22:14 ` Jonathan Nieder 2017-03-22 22:12 ` Junio C Hamano 2017-03-22 22:24 ` Jeff King 2017-03-22 22:28 ` Stefan Beller 2017-03-22 21:34 ` [PATCH 2/3] t7406: fix here-doc syntax errors Junio C Hamano 2017-03-22 20:08 ` [PATCH 3/3] t7004, t7030: " Junio C Hamano 2017-03-22 21:10 ` Jeff King 2017-03-22 21:43 ` Santiago Torres 2017-03-22 22:04 ` Jeff King 2017-03-22 22:04 ` Junio C Hamano 2017-03-22 22:15 ` Santiago Torres 2017-03-22 22:22 ` Jeff King 2017-03-22 22:34 ` Santiago Torres 2017-03-22 22:41 ` Jeff King 2017-03-22 22:47 ` Junio C Hamano 2017-03-22 22:51 ` Santiago Torres 2017-03-23 22:00 ` Junio C Hamano 2017-03-23 22:28 ` Santiago Torres 2017-03-23 23:49 ` Jeff King 2017-03-24 16:45 ` Junio C Hamano 2017-03-24 16:49 ` Jeff King 2017-03-24 18:00 ` Jeff King 2017-03-24 18:04 ` Junio C Hamano 2017-03-24 18:16 ` Jeff King 2017-03-22 22:38 ` Junio C Hamano 2017-03-22 22:40 ` [PATCH 0/3] fix "here-doc" " Jan Palus 2017-03-23 2:12 ` [PATCH] tests: lint for run-away here-doc Junio C Hamano 2017-03-23 5:43 ` [PATCH v2] " Junio C Hamano 2017-03-24 1:29 ` Jonathan Nieder 2017-03-24 2:45 ` Junio C Hamano 2017-03-24 3:59 ` Jeff King
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git 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 read-only IMAP folder(s) and NNTP newsgroup(s).