{"thread":{"id":"48235","subject":"[PATCH] t5404: relax overzealous test","startedAt":"2018-04-06T19:31:53Z","lastAt":"2018-04-09T13:52:56Z","messageCount":7,"participants":["Johannes Schindelin","Jeff King","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"343993","messageId":"958ee6f006aba0c67c8e064d31206e3e68a1cc49.1523043053.git.johannes.schindelin@gmx.de","threadId":"48235","inReplyTo":null,"subject":"[PATCH] t5404: relax overzealous test","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-04-06T19:31:22Z","receivedAt":"2018-04-06T19:31:53Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"In 0b294c0abf0 (make deleting a missing ref more quiet, 2008-07-08), we\nadded a test to verify that deleting an already-deleted ref does not\nshow an error.\n\nOur test simply looks for the substring 'error' in the output of the\n`git push`, which might look innocuous on the face of it.\n\nSuppose, however, that you are a big fan of whales. Or even better: your\nIT administrator has a whale of a time picking cute user names, e.g.\nreferring to you (due to your like of India Pale Ales) as \"one of the\ncuter rorquals\" (see https://en.wikipedia.org/wiki/Rorqual to learn a\nthing or two about rorquals) and hence your home directory becomes\n/home/cuterrorqual. If you now run t5404, it fails! Why? Because the\ntest calls `git push origin :b3` which outputs:\n\n To /home/cuterrorqual/git/t/trash directory.t5404-tracking-branches/.\n - [deleted] b3\n\nNote how there is no error displayed in that output? But of course\n\"error\" is a substring of \"cuterrorqual\". And so that `grep error\noutput` finds something.\n\nThis bug was not, actually, caught having \"error\" as a substring of the\nuser name but while working in a worktree called \"colorize-push-errors\",\nwhose name was part of that output, too, suggesting that not even\ntesting for the *word* `error` via `git grep -w error output` would fix\nthe underlying issue.\n\nThis patch chooses instead to look for the prefix \"error:\" at the\nbeginning of the line, so that there can be no ambiguity that any catch\nwas indeed a message generated by Git's `error_builtin()` function.\n\nSigned-off-by: Johannes Schindelin \n---\nPublished-As: https://github.com/dscho/git/releases/tag/fix-t5404-error-v1\nFetch-It-Via: git fetch https://github.com/dscho/git fix-t5404-error-v1\n t/t5404-tracking-branches.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t5404-tracking-branches.sh b/t/t5404-tracking-branches.sh\nindex 2b8c0bac7db..2762f420bc2 100755\n--- a/t/t5404-tracking-branches.sh\n+++ b/t/t5404-tracking-branches.sh\n@@ -56,7 +56,7 @@ test_expect_success 'deleted branches have their tracking branches removed' '\n test_expect_success 'already deleted tracking branches ignored' '\n \tgit branch -d -r origin/b3 &&\n \tgit push origin :b3 >output 2>&1 &&\n-\t! grep error output\n+\t! grep \"^error: \" output\n '\n \n test_done\n\nbase-commit: 468165c1d8a442994a825f3684528361727cd8c0\n-- \n2.17.0.windows.1.4.g7e4058d72e3\n"},{"id":"343998","messageId":"20180406195331.GC11450@sigill.intra.peff.net","threadId":"48235","inReplyTo":"958ee6f006aba0c67c8e064d31206e3e68a1cc49.1523043053.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH] t5404: relax overzealous test","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-04-06T19:53:31Z","receivedAt":"2018-04-06T19:53:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 06, 2018 at 09:31:22PM +0200, Johannes Schindelin wrote:\n\n> In 0b294c0abf0 (make deleting a missing ref more quiet, 2008-07-08), we\n> added a test to verify that deleting an already-deleted ref does not\n> show an error.\n\nAmazing that it took this long to come up.\n\n> Suppose, however, that you are a big fan of whales. Or even better: your\n> IT administrator has a whale of a time picking cute user names, e.g.\n> referring to you (due to your like of India Pale Ales) as \"one of the\n> cuter rorquals\" (see https://en.wikipedia.org/wiki/Rorqual to learn a\n> thing or two about rorquals) and hence your home directory becomes\n> /home/cuterrorqual. If you now run t5404, it fails! Why? Because the\n> test calls `git push origin :b3` which outputs:\n> \n> To /home/cuterrorqual/git/t/trash directory.t5404-tracking-branches/.\n> - [deleted] b3\n\nThis is from the same Dscho who complains about the length of some of my\ncommit messages, right? ;P\n\n> This patch chooses instead to look for the prefix \"error:\" at the\n> beginning of the line, so that there can be no ambiguity that any catch\n> was indeed a message generated by Git's `error_builtin()` function.\n\nYep, this seems obviously correct.\n\nRight now we do not localize the \"error\" string, but I wonder if this\nought to use test_i18ngrep. With or without that change, the patch looks\ngood to me.\n\n-Peff\n"},{"id":"344001","messageId":"nycvar.QRO.7.76.6.1804062212310.55@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz","threadId":"48235","inReplyTo":"20180406195331.GC11450@sigill.intra.peff.net","subject":"Re: [PATCH] t5404: relax overzealous test","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-04-06T20:13:46Z","receivedAt":"2018-04-06T20:14:19Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Fri, 6 Apr 2018, Jeff King wrote:\n\n> On Fri, Apr 06, 2018 at 09:31:22PM +0200, Johannes Schindelin wrote:\n> \n> > Suppose, however, that you are a big fan of whales. Or even better: your\n> > IT administrator has a whale of a time picking cute user names, e.g.\n> > referring to you (due to your like of India Pale Ales) as \"one of the\n> > cuter rorquals\" (see https://en.wikipedia.org/wiki/Rorqual to learn a\n> > thing or two about rorquals) and hence your home directory becomes\n> > /home/cuterrorqual. If you now run t5404, it fails! Why? Because the\n> > test calls `git push origin :b3` which outputs:\n> > \n> > To /home/cuterrorqual/git/t/trash directory.t5404-tracking-branches/.\n> > - [deleted] b3\n> \n> This is from the same Dscho who complains about the length of some of my\n> commit messages, right? ;P\n\nYes. I've quieted down, though, didn't I? Mainly because I see the merit\nin your long commit messages now. Never too late to learn.\n\nCiao,\nDscho\n"},{"id":"344002","messageId":"20180406201626.GA16310@sigill.intra.peff.net","threadId":"48235","inReplyTo":"nycvar.QRO.7.76.6.1804062212310.55@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz","subject":"Re: [PATCH] t5404: relax overzealous test","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-04-06T20:16:26Z","receivedAt":"2018-04-06T20:16:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 06, 2018 at 10:13:46PM +0200, Johannes Schindelin wrote:\n\n> On Fri, 6 Apr 2018, Jeff King wrote:\n> \n> > On Fri, Apr 06, 2018 at 09:31:22PM +0200, Johannes Schindelin wrote:\n> > \n> > > Suppose, however, that you are a big fan of whales. Or even better: your\n> > > IT administrator has a whale of a time picking cute user names, e.g.\n> > > referring to you (due to your like of India Pale Ales) as \"one of the\n> > > cuter rorquals\" (see https://en.wikipedia.org/wiki/Rorqual to learn a\n> > > thing or two about rorquals) and hence your home directory becomes\n> > > /home/cuterrorqual. If you now run t5404, it fails! Why? Because the\n> > > test calls `git push origin :b3` which outputs:\n> > > \n> > > To /home/cuterrorqual/git/t/trash directory.t5404-tracking-branches/.\n> > > - [deleted] b3\n> > \n> > This is from the same Dscho who complains about the length of some of my\n> > commit messages, right? ;P\n> \n> Yes. I've quieted down, though, didn't I? Mainly because I see the merit\n> in your long commit messages now. Never too late to learn.\n\nHeh, well I'm glad I've won you over, then. And thank you for cleaning\nup my (ancient) mess here.\n\n-Peff\n"},{"id":"344005","messageId":"87tvsoysww.fsf@evledraar.gmail.com","threadId":"48235","inReplyTo":"20180406195331.GC11450@sigill.intra.peff.net","subject":"Re: [PATCH] t5404: relax overzealous test","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-04-06T20:35:43Z","receivedAt":"2018-04-06T20:35:51Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Apr 06 2018, Jeff King wrote:\n\n> On Fri, Apr 06, 2018 at 09:31:22PM +0200, Johannes Schindelin wrote:\n>> This patch chooses instead to look for the prefix \"error:\" at the\n>> beginning of the line, so that there can be no ambiguity that any catch\n>> was indeed a message generated by Git's `error_builtin()` function.\n>\n> Yep, this seems obviously correct.\n>\n> Right now we do not localize the \"error\" string, but I wonder if this\n> ought to use test_i18ngrep. With or without that change, the patch looks\n> good to me.\n\nI think it's much better not to do that.\n\nThere's value in the i18n versions checking for only those things that\nare translated, and no more, because we should really be on the lookout\nfor any i18n changes that potentially change plumbing output, or other\nthings programs expect to parse.\n"},{"id":"344146","messageId":"xmqq60516rwq.fsf@gitster-ct.c.googlers.com","threadId":"48235","inReplyTo":"20180406195331.GC11450@sigill.intra.peff.net","subject":"Re: [PATCH] t5404: relax overzealous test","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-04-09T02:21:41Z","receivedAt":"2018-04-09T02:21:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Apr 06, 2018 at 09:31:22PM +0200, Johannes Schindelin wrote:\n>\n>> In 0b294c0abf0 (make deleting a missing ref more quiet, 2008-07-08), we\n>> added a test to verify that deleting an already-deleted ref does not\n>> show an error.\n>\n> Amazing that it took this long to come up.\n> ...\n>> This patch chooses instead to look for the prefix \"error:\" at the\n>> beginning of the line, so that there can be no ambiguity that any catch\n>> was indeed a message generated by Git's `error_builtin()` function.\n>\n> Yep, this seems obviously correct.\n\nHits in\n\n $ git grep 'grep [\"'\\'']*error' t\n\nshows that many checks for errors that are not anchored, but I do\nnot think any of them are looking for the string in a pathname other\nthan this instance, so it would be OK.\n\nThanks, both. Will apply.\n\n\n"},{"id":"344196","messageId":"nycvar.QRO.7.76.6.1804091551170.55@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz","threadId":"48235","inReplyTo":"xmqq60516rwq.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] t5404: relax overzealous test","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-04-09T13:52:42Z","receivedAt":"2018-04-09T13:52:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 9 Apr 2018, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > On Fri, Apr 06, 2018 at 09:31:22PM +0200, Johannes Schindelin wrote:\n> >\n> >> In 0b294c0abf0 (make deleting a missing ref more quiet, 2008-07-08), we\n> >> added a test to verify that deleting an already-deleted ref does not\n> >> show an error.\n> >\n> > Amazing that it took this long to come up.\n> > ...\n> >> This patch chooses instead to look for the prefix \"error:\" at the\n> >> beginning of the line, so that there can be no ambiguity that any catch\n> >> was indeed a message generated by Git's `error_builtin()` function.\n> >\n> > Yep, this seems obviously correct.\n> \n> Hits in\n> \n> $ git grep 'grep [\"'\\'']*error' t\n> \n> shows that many checks for errors that are not anchored, but I do\n> not think any of them are looking for the string in a pathname other\n> than this instance, so it would be OK.\n\nYes, they are okay, I did run the entire test suite in a worktree with the\nsame name as my branch name: fix-t5404-error.\n\nCiao,\nDscho\n"}]}