{"thread":{"id":"10885","subject":"[BUG] t7004 (master) busted on Leopard","startedAt":"2007-11-15T13:38:57Z","lastAt":"2007-11-17T08:55:21Z","messageCount":26,"participants":["Wincent Colaiuta","Johannes Schindelin","David Kastrup","Mike Hommey","Benoit Sigoure","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"59983","messageId":"17E3F66C-4644-4B64-817C-88062727A2D9@wincent.com","threadId":"10885","inReplyTo":null,"subject":"[BUG] t7004 (master) busted on Leopard","fromName":"Wincent Colaiuta","fromEmail":"win@wincent.com","sentAt":"2007-11-15T13:38:57Z","receivedAt":"2007-11-15T13:38:57Z","isPatch":false,"sender":{"key":"greg@hurrell.net","avatar":"https://avatars.githubusercontent.com/u/7074?v=4"},"body":"Commit 4d8b1dc850 added a couple of tests to t7004, and my testing \nreveals that this one has been broken on Leopard since then:\n\n* FAIL 83: message in editor has initial comment\nGIT_EDITOR=cat git tag -a initial-comment > actual || true &&\ntest $(sed -n \"/^\\(#\\|\\$\\)/p\" actual | wc -l) -gt 0\n\n(Not sure whether this affects other platforms, or versions of Mac OS \nX prior to Leopard, as I only have one machine here.)\n\nThe problem is the version of sed that ships with Leopard fails to \nmatch the initial comment with this syntax:\n\nsed -n \"/^\\(#\\|\\$\\)/p\" actual\n\nAn alternative that works is:\n\nsed -n \"/^[#\\$]/p\" actual\n\nCan someone with knowledge of sed compatibility across multiple \nplatforms suggest an alternative which will work on a wider range of \nsystems?\n\nCheers,\nWincent\n"},{"id":"59987","messageId":"Pine.LNX.4.64.0711151434060.30886@racer.site","threadId":"10885","inReplyTo":"17E3F66C-4644-4B64-817C-88062727A2D9@wincent.com","subject":"Re: [BUG] t7004 (master) busted on Leopard","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-11-15T14:37:36Z","receivedAt":"2007-11-15T14:37:36Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 15 Nov 2007, Wincent Colaiuta wrote:\n\n> Commit 4d8b1dc850 added a couple of tests to t7004, and my testing reveals\n> that this one has been broken on Leopard since then:\n> \n> * FAIL 83: message in editor has initial comment\n> GIT_EDITOR=cat git tag -a initial-comment > actual || true &&\n> test $(sed -n \"/^\\(#\\|\\$\\)/p\" actual | wc -l) -gt 0\n\nI think this is our good old friend, MacOSX' sed. (Wasn't there a \nquestion today what's wrong with using sed? I think this issue \nqualifies.)\n\nI imagine that it is that MacOSX' sed is adding a trailing newline (not \nthe regexp like you suggested). Which means that \"wc -l\" would print \"1\". \n(You can see for yourself if you run the script with \"sh -x ...\".)\n\nIMHO a good solution would be\n\n\ttest -z \"$(grep -e '^#' -e '^$' actual)\"\n\nCould you test, please?\n\nThanks,\nDscho\n"},{"id":"59988","messageId":"86k5oj36pq.fsf@lola.quinscape.zz","threadId":"10885","inReplyTo":"Pine.LNX.4.64.0711151434060.30886@racer.site","subject":"Re: [BUG] t7004 (master) busted on Leopard","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2007-11-15T14:48:33Z","receivedAt":"2007-11-15T14:48:33Z","isPatch":false,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Johannes Schindelin writes:\n\n> Hi,\n>\n> On Thu, 15 Nov 2007, Wincent Colaiuta wrote:\n>\n>> Commit 4d8b1dc850 added a couple of tests to t7004, and my testing reveals\n>> that this one has been broken on Leopard since then:\n>> \n>> * FAIL 83: message in editor has initial comment\n>> GIT_EDITOR=cat git tag -a initial-comment > actual || true &&\n>> test $(sed -n \"/^\\(#\\|\\$\\)/p\" actual | wc -l) -gt 0\n>\n> I think this is our good old friend, MacOSX' sed. (Wasn't there a \n> question today what's wrong with using sed? I think this issue \n> qualifies.)\n\n\\| is not portable. $ in groups is not portable.\n\n> I imagine that it is that MacOSX' sed is adding a trailing newline (not \n> the regexp like you suggested).\n\nI imagine it is one of the above. Most sed's add trailing newlines.\n\n-- \nDavid Kastrup\n"},{"id":"59990","messageId":"A3467E97-601B-45EE-816D-376F4FFE69E8@wincent.com","threadId":"10885","inReplyTo":"Pine.LNX.4.64.0711151434060.30886@racer.site","subject":"Re: [BUG] t7004 (master) busted on Leopard","fromName":"Wincent Colaiuta","fromEmail":"win@wincent.com","sentAt":"2007-11-15T15:12:44Z","receivedAt":"2007-11-15T15:12:44Z","isPatch":false,"sender":{"key":"greg@hurrell.net","avatar":"https://avatars.githubusercontent.com/u/7074?v=4"},"body":"El 15/11/2007, a las 15:37, Johannes Schindelin escribió:\n\n> Hi,\n>\n> On Thu, 15 Nov 2007, Wincent Colaiuta wrote:\n>\n>> Commit 4d8b1dc850 added a couple of tests to t7004, and my testing \n>> reveals\n>> that this one has been broken on Leopard since then:\n>>\n>> * FAIL 83: message in editor has initial comment\n>> GIT_EDITOR=cat git tag -a initial-comment > actual || true &&\n>> test $(sed -n \"/^\\(#\\|\\$\\)/p\" actual | wc -l) -gt 0\n>\n> I think this is our good old friend, MacOSX' sed.\n\nYes, that's exactly what I said in the part of my post that you didn't \nquote.\n\n> I imagine that it is that MacOSX' sed is adding a trailing newline \n> (not\n> the regexp like you suggested). Which means that \"wc -l\" would \n> print \"1\".\n> (You can see for yourself if you run the script with \"sh -x ...\".)\n\nUnless I am misreading the test, any output from \"wc -l\" that is \ngreater than 0 will cause the test to pass, so even if it outputted \n\"1\" as you suggest then that wouldn't be the cause of the failure.\n\nI do think the cause of the failure is the limited regexp syntax that \nsed accepts on Leopard; witness: on Mac OS X the following prints 0:\n\n echo \"# hello\" | sed -n \"/^\\(#\\|\\$\\)/p\" | wc -l\n\nWhereas the following prints 1:\n\n echo \"# hello\" | sed -n \"/^[#\\$]/p\" | wc -l\n\nHere's the output for the failing test run under \"sh -x\" as you \nsuggest, although I must admit that I can't really parse it myself.\n\n+ test_expect_success 'message in editor has initial comment' '\n\tGIT_EDITOR=cat git tag -a initial-comment > actual || true &&\n\ttest $(sed -n \"/^\\(#\\|\\$\\)/p\" actual | wc -l) -gt 0\n'\n+ test 2 = 2\n+ test_skip 'message in editor has initial comment' '\n\tGIT_EDITOR=cat git tag -a initial-comment > actual || true &&\n\ttest $(sed -n \"/^\\(#\\|\\$\\)/p\" actual | wc -l) -gt 0\n'\n++ expr ./t7004-tag.sh : '.*/\\(t[0-9]*\\)-[^/]*$'\n+ this_test=t7004\n++ expr 82 + 1\n+ this_test=t7004.83\n+ to_skip=\n+ case \"$to_skip\" in\n+ false\n+ say 'expecting success:\n\tGIT_EDITOR=cat git tag -a initial-comment > actual || true &&\n\ttest $(sed -n \"/^\\(#\\|\\$\\)/p\" actual | wc -l) -gt 0\n'\n+ say_color info 'expecting success:\n\tGIT_EDITOR=cat git tag -a initial-comment > actual || true &&\n\ttest $(sed -n \"/^\\(#\\|\\$\\)/p\" actual | wc -l) -gt 0\n'\n+ case \"$1\" in\n+ tput setaf 3\n+ shift\n+ echo '* expecting success:\n\tGIT_EDITOR=cat git tag -a initial-comment > actual || true &&\n\ttest $(sed -n \"/^\\(#\\|\\$\\)/p\" actual | wc -l) -gt 0\n'\n+ tput sgr0\n+ test_run_ '\n\tGIT_EDITOR=cat git tag -a initial-comment > actual || true &&\n\ttest $(sed -n \"/^\\(#\\|\\$\\)/p\" actual | wc -l) -gt 0\n'\n+ eval '\n\tGIT_EDITOR=cat git tag -a initial-comment > actual || true &&\n\ttest $(sed -n \"/^\\(#\\|\\$\\)/p\" actual | wc -l) -gt 0\n'\n+ eval_ret=1\n+ return 0\n+ '[' 0 = 0 -a 1 = 0 ']'\n+ test_failure_ 'message in editor has initial comment' '\n\tGIT_EDITOR=cat git tag -a initial-comment > actual || true &&\n\ttest $(sed -n \"/^\\(#\\|\\$\\)/p\" actual | wc -l) -gt 0\n'\n++ expr 82 + 1\n+ test_count=83\n++ expr 0 + 1\n+ test_failure=1\n+ say_color error 'FAIL 83: message in editor has initial comment'\n+ case \"$1\" in\n+ tput bold\n+ tput setaf 1\n+ shift\n+ echo '* FAIL 83: message in editor has initial comment'\n* FAIL 83: message in editor has initial comment\n+ tput sgr0\n+ shift\n\n> IMHO a good solution would be\n>\n> \ttest -z \"$(grep -e '^#' -e '^$' actual)\"\n>\n> Could you test, please?\n\nYes, the test passes with that, although it has to be written as \nfollows in the actual test file seeing as it's inside a single-quoted \nstring:\n\ntest -z \"$(grep -e \\\"^#\\\" -e \\\"^$\\\" actual)\"\n\nWill follow this up with a patch that implements your proposed fix.\n\nCheers,\nWincent\n"},{"id":"59991","messageId":"90D416F6-A3CA-43C8-B71F-38EA955A762B@wincent.com","threadId":"10885","inReplyTo":"Pine.LNX.4.64.0711151434060.30886@racer.site","subject":"[PATCH] Fix git-tag test breakage caused by broken sed on Leopard","fromName":"Wincent Colaiuta","fromEmail":"win@wincent.com","sentAt":"2007-11-15T15:16:52Z","receivedAt":"2007-11-15T15:16:52Z","isPatch":true,"sender":{"key":"greg@hurrell.net","avatar":"https://avatars.githubusercontent.com/u/7074?v=4"},"body":"The 'message in editor has initial comment' test fails on Leopard (and\npossibly on other versions of Mac OS X as well) due to the limited\nsed syntax available on that platform.\n\nAvoid the breakage by using grep instead (suggested by Johannes\nSchindelin).\n\nSigned-off-by: Wincent Colaiuta \n---\n t/t7004-tag.sh | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex 096fe33..02ec9c3 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1007,7 +1007,7 @@ test_expect_failure \\\n test_expect_success \\\n \t'message in editor has initial comment' '\n \tGIT_EDITOR=cat git tag -a initial-comment > actual || true &&\n-\ttest $(sed -n \"/^\\(#\\|\\$\\)/p\" actual | wc -l) -gt 0\n+\ttest -z \"$(grep -e \\\"^#\\\" -e \\\"^$\\\" actual)\"\n '\n\n get_tag_header reuse $commit commit $time >expect\n-- \n1.5.3.5\n"},{"id":"59992","messageId":"C9DFAD77-DFEF-47AC-83D5-CFDF245D95E4@wincent.com","threadId":"10885","inReplyTo":"90D416F6-A3CA-43C8-B71F-38EA955A762B@wincent.com","subject":"[PATCH v2] Fix git-tag test breakage caused by broken sed on Leopard","fromName":"Wincent Colaiuta","fromEmail":"win@wincent.com","sentAt":"2007-11-15T15:47:11Z","receivedAt":"2007-11-15T15:47:11Z","isPatch":true,"sender":{"key":"greg@hurrell.net","avatar":"https://avatars.githubusercontent.com/u/7074?v=4"},"body":"The 'message in editor has initial comment' test fails on Leopard (and\npossibly on other versions of Mac OS X as well) due to the limited\nsed syntax available on that platform.\n\nAvoid the breakage by using grep instead (suggested by Johannes\nSchindelin).\n\nSigned-off-by: Wincent Colaiuta \n---\n\nThe patch I previously sent had the test sense inverted (it used 'test \n-z' to test for an empty string when we should have actually been \nlooking for a non-empty string), so it really only passed by mistake. \nThis revised version maintains the sense of the original, sed-based \ntest.\n\nt/t7004-tag.sh | 2 +-\n1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex 096fe33..b54c2e0 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1007,7 +1007,7 @@ test_expect_failure \\\ntest_expect_success \\\n\t'message in editor has initial comment' '\n\tGIT_EDITOR=cat git tag -a initial-comment > actual || true &&\n-\ttest $(sed -n \"/^\\(#\\|\\$\\)/p\" actual | wc -l) -gt 0\n+\ttest $(grep -e \"^#\" -e \"^\\$\" actual | wc -l ) -gt 0\n'\n\nget_tag_header reuse $commit commit $time >expect\n-- \n1.5.3.5\n"},{"id":"60088","messageId":"7A6820DE-C6EE-4E3E-A97F-97766F682B85@wincent.com","threadId":"10885","inReplyTo":"7v4pfm3h6f.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH v2] Fix git-tag test breakage caused by broken sed on Leopard","fromName":"Wincent Colaiuta","fromEmail":"win@wincent.com","sentAt":"2007-11-16T13:45:08Z","receivedAt":"2007-11-16T13:45:08Z","isPatch":true,"sender":{"key":"greg@hurrell.net","avatar":"https://avatars.githubusercontent.com/u/7074?v=4"},"body":"El 16/11/2007, a las 6:14, Junio C Hamano escribió:\n\n> Wincent Colaiuta writes:\n>\n>> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\n>> index 096fe33..b54c2e0 100755\n>> --- a/t/t7004-tag.sh\n>> +++ b/t/t7004-tag.sh\n>> @@ -1007,7 +1007,7 @@ test_expect_failure \\\n>> test_expect_success \\\n>> \t'message in editor has initial comment' '\n>> \tGIT_EDITOR=cat git tag -a initial-comment > actual || true &&\n>> -\ttest $(sed -n \"/^\\(#\\|\\$\\)/p\" actual | wc -l) -gt 0\n>> +\ttest $(grep -e \"^#\" -e \"^\\$\" actual | wc -l ) -gt 0\n>> '\n>\n> Heh, doesn't grep exit with zero only when it found some lines\n> that match the pattern already? What's that \"wc -l\" for?\n\n\nI was just trying to make the minimal change (swapping grep for sed), \nbut if you want a shorter version then we don't even need the \"test\"; \nit could just be:\n\n-\ttest $(sed -n \"/^\\(#\\|\\$\\)/p\" actual | wc -l) -gt 0\n+\tgrep -e \"^#\" -e \"^\\$\" actual\n\nCheers,\nWincent\n"},{"id":"60089","messageId":"03D3DFCD-B5F7-47CF-AFD2-F1408BB11AB3@wincent.com","threadId":"10885","inReplyTo":"7A6820DE-C6EE-4E3E-A97F-97766F682B85@wincent.com","subject":"Re: [PATCH v2] Fix git-tag test breakage caused by broken sed on Leopard","fromName":"Wincent Colaiuta","fromEmail":"win@wincent.com","sentAt":"2007-11-16T13:48:09Z","receivedAt":"2007-11-16T13:48:09Z","isPatch":true,"sender":{"key":"greg@hurrell.net","avatar":"https://avatars.githubusercontent.com/u/7074?v=4"},"body":"El 16/11/2007, a las 14:45, Wincent Colaiuta escribió:\n\n> El 16/11/2007, a las 6:14, Junio C Hamano escribió:\n>\n>> Wincent Colaiuta writes:\n>>\n>>> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\n>>> index 096fe33..b54c2e0 100755\n>>> --- a/t/t7004-tag.sh\n>>> +++ b/t/t7004-tag.sh\n>>> @@ -1007,7 +1007,7 @@ test_expect_failure \\\n>>> test_expect_success \\\n>>> \t'message in editor has initial comment' '\n>>> \tGIT_EDITOR=cat git tag -a initial-comment > actual || true &&\n>>> -\ttest $(sed -n \"/^\\(#\\|\\$\\)/p\" actual | wc -l) -gt 0\n>>> +\ttest $(grep -e \"^#\" -e \"^\\$\" actual | wc -l ) -gt 0\n>>> '\n>>\n>> Heh, doesn't grep exit with zero only when it found some lines\n>> that match the pattern already? What's that \"wc -l\" for?\n>\n>\n> I was just trying to make the minimal change (swapping grep for \n> sed), but if you want a shorter version then we don't even need the \n> \"test\"; it could just be:\n>\n> -\ttest $(sed -n \"/^\\(#\\|\\$\\)/p\" actual | wc -l) -gt 0\n> +\tgrep -e \"^#\" -e \"^\\$\" actual\n\nAlthough I don't know if we should be testing for empty lines there \nbecause an 0-byte empty \"actual\" file would spuriously pass the test. \nPerhaps this would be better:\n\n-\ttest $(sed -n \"/^\\(#\\|\\$\\)/p\" actual | wc -l) -gt 0\n+\tgrep -e \"^#\" actual\n\nCheers,\nWincent\n"},{"id":"60094","messageId":"20071116165944.GB29272@glandium.org","threadId":"10885","inReplyTo":"03D3DFCD-B5F7-47CF-AFD2-F1408BB11AB3@wincent.com","subject":"Re: [PATCH v2] Fix git-tag test breakage caused by broken sed on Leopard","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2007-11-16T16:59:44Z","receivedAt":"2007-11-16T16:59:44Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Fri, Nov 16, 2007 at 02:48:09PM +0100, Wincent Colaiuta wrote:\n> El 16/11/2007, a las 14:45, Wincent Colaiuta escribió:\n> \n> > El 16/11/2007, a las 6:14, Junio C Hamano escribió:\n> >\n> >> Wincent Colaiuta writes:\n> >>\n> >>> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\n> >>> index 096fe33..b54c2e0 100755\n> >>> --- a/t/t7004-tag.sh\n> >>> +++ b/t/t7004-tag.sh\n> >>> @@ -1007,7 +1007,7 @@ test_expect_failure \\\n> >>> test_expect_success \\\n> >>> \t'message in editor has initial comment' '\n> >>> \tGIT_EDITOR=cat git tag -a initial-comment > actual || true &&\n> >>> -\ttest $(sed -n \"/^\\(#\\|\\$\\)/p\" actual | wc -l) -gt 0\n> >>> +\ttest $(grep -e \"^#\" -e \"^\\$\" actual | wc -l ) -gt 0\n> >>> '\n> >>\n> >> Heh, doesn't grep exit with zero only when it found some lines\n> >> that match the pattern already? What's that \"wc -l\" for?\n> >\n> >\n> > I was just trying to make the minimal change (swapping grep for \n> > sed), but if you want a shorter version then we don't even need the \n> > \"test\"; it could just be:\n> >\n> > -\ttest $(sed -n \"/^\\(#\\|\\$\\)/p\" actual | wc -l) -gt 0\n> > +\tgrep -e \"^#\" -e \"^\\$\" actual\n> \n> Although I don't know if we should be testing for empty lines there \n> because an 0-byte empty \"actual\" file would spuriously pass the test. \n> Perhaps this would be better:\n> \n> -\ttest $(sed -n \"/^\\(#\\|\\$\\)/p\" actual | wc -l) -gt 0\n> +\tgrep -e \"^#\" actual\n\nMatching both would as in your previous pseudo patch wouldn't catch\nempty file. On the other hand, both my initial bloated version and yours\nwon't catch a file that doesn't contain the comment.\n\ngrep -e \"^$\" actual && grep -e \"^#\" actual would actually be a better\ntest.\n\nMike\n"},{"id":"60095","messageId":"E4B5D2FD-5757-4178-B77C-1D61E7B3F589@wincent.com","threadId":"10885","inReplyTo":"20071116165944.GB29272@glandium.org","subject":"Re: [PATCH v2] Fix git-tag test breakage caused by broken sed on Leopard","fromName":"Wincent Colaiuta","fromEmail":"win@wincent.com","sentAt":"2007-11-16T17:25:43Z","receivedAt":"2007-11-16T17:25:43Z","isPatch":true,"sender":{"key":"greg@hurrell.net","avatar":"https://avatars.githubusercontent.com/u/7074?v=4"},"body":"El 16/11/2007, a las 17:59, Mike Hommey escribió:\n\n> On Fri, Nov 16, 2007 at 02:48:09PM +0100, Wincent Colaiuta wrote:\n>> El 16/11/2007, a las 14:45, Wincent Colaiuta escribió:\n>>\n>>> El 16/11/2007, a las 6:14, Junio C Hamano escribió:\n>>>\n>>>> Wincent Colaiuta writes:\n>>>>\n>>>>> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\n>>>>> index 096fe33..b54c2e0 100755\n>>>>> --- a/t/t7004-tag.sh\n>>>>> +++ b/t/t7004-tag.sh\n>>>>> @@ -1007,7 +1007,7 @@ test_expect_failure \\\n>>>>> test_expect_success \\\n>>>>> \t'message in editor has initial comment' '\n>>>>> \tGIT_EDITOR=cat git tag -a initial-comment > actual || true &&\n>>>>> -\ttest $(sed -n \"/^\\(#\\|\\$\\)/p\" actual | wc -l) -gt 0\n>>>>> +\ttest $(grep -e \"^#\" -e \"^\\$\" actual | wc -l ) -gt 0\n>>>>> '\n>>>>\n>>>> Heh, doesn't grep exit with zero only when it found some lines\n>>>> that match the pattern already? What's that \"wc -l\" for?\n>>>\n>>>\n>>> I was just trying to make the minimal change (swapping grep for\n>>> sed), but if you want a shorter version then we don't even need the\n>>> \"test\"; it could just be:\n>>>\n>>> -\ttest $(sed -n \"/^\\(#\\|\\$\\)/p\" actual | wc -l) -gt 0\n>>> +\tgrep -e \"^#\" -e \"^\\$\" actual\n>>\n>> Although I don't know if we should be testing for empty lines there\n>> because an 0-byte empty \"actual\" file would spuriously pass the test.\n>> Perhaps this would be better:\n>>\n>> -\ttest $(sed -n \"/^\\(#\\|\\$\\)/p\" actual | wc -l) -gt 0\n>> +\tgrep -e \"^#\" actual\n>\n> Matching both would as in your previous pseudo patch wouldn't catch\n> empty file. On the other hand, both my initial bloated version and \n> yours\n> won't catch a file that doesn't contain the comment.\n\nBut if git-tag works then it will contain a comment; and isn't the \npurpose of the test to confirm the comment's presence?\n\n> grep -e \"^$\" actual && grep -e \"^#\" actual would actually be a better\n> test.\n\nWhat are we really trying to test here? The test is labelled as \n'message in editor has initial comment'. Basically the editor will be \nprepopulated with a blank line followed by this:\n\n#\n# Write a tag message\n#\n\nSo it's the presence of that text which we want to confirm.\n\nIf I understand the intent of the original sed-based test, it was to \nconfirm that there were 1 or more lines that started with \"#\" or were \nempty. It also suffered from the bug that an empty message (by which I \nmean a 1-byte file containing only an LF) would pass the test \n(spuriously in my opinion), and my first attempt faithfully translated \nthat bug into grep syntax.\n\nThe alternative you suggest will (correctly) fail on a zero byte file, \nand pass on 1-byte file containing only a LF, just like the other \napproaches. So I'm still wondering, what is it that we're trying to \ntest here? We could test for the exact \"Write a tag message\" text, but \nthat may be brittle (must be updated if/when the text ever changes), \nbut looking at other tests I see there are some which do precise \nequality tests for expected results.\n\nCheers,\nWincent\n"},{"id":"60096","messageId":"1195233971-12288-1-git-send-email-mh@glandium.org","threadId":"10885","inReplyTo":"20071116165944.GB29272@glandium.org","subject":"[PATCH] Fix t7004 which fails with retarded sed","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2007-11-16T17:26:11Z","receivedAt":"2007-11-16T17:26:11Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"Brown paper bag fix to avoid test failure with retarded sed. The test\nby itself didn't catch what it was supposed to, anyways.\n\nSo now, we test whether the editor gets at least an empty line, some\ncommented lines, and doesn't get anything else.\n\nSigned-off-by: Mike Hommey \n---\n t/t7004-tag.sh | 4 +++-\n 1 files changed, 3 insertions(+), 1 deletions(-)\n\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex 096fe33..3813f23 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1007,7 +1007,9 @@ test_expect_failure \\\n test_expect_success \\\n \t'message in editor has initial comment' '\n \tGIT_EDITOR=cat git tag -a initial-comment > actual || true &&\n-\ttest $(sed -n \"/^\\(#\\|\\$\\)/p\" actual | wc -l) -gt 0\n+\tgrep -e \"^$\" actual > /dev/null 2>&1 &&\n+\tgrep -e \"^#\" actual > /dev/null 2>&1 &&\n+\t! grep -e \"^[^#]\" actual > /dev/null 2>&1\n '\n \n get_tag_header reuse $commit commit $time >expect\n-- \n1.5.3.5\n"},{"id":"60098","messageId":"6E925AF7-FCCB-4793-936F-7BC513272E75@lrde.epita.fr","threadId":"10885","inReplyTo":"1195233971-12288-1-git-send-email-mh@glandium.org","subject":"Re: [PATCH] Fix t7004 which fails with retarded sed","fromName":"Benoit Sigoure","fromEmail":"tsuna@lrde.epita.fr","sentAt":"2007-11-16T17:58:21Z","receivedAt":"2007-11-16T17:58:21Z","isPatch":true,"sender":{"key":"tsunanet@gmail.com","avatar":"https://avatars.githubusercontent.com/u/128281?v=4"},"body":"On Nov 16, 2007, at 6:26 PM, Mike Hommey wrote:\n\n> Brown paper bag fix to avoid test failure with retarded sed. The test\n> by itself didn't catch what it was supposed to, anyways.\n>\n> So now, we test whether the editor gets at least an empty line, some\n> commented lines, and doesn't get anything else.\n>\n> Signed-off-by: Mike Hommey \n> ---\n> t/t7004-tag.sh | 4 +++-\n> 1 files changed, 3 insertions(+), 1 deletions(-)\n>\n> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\n> index 096fe33..3813f23 100755\n> --- a/t/t7004-tag.sh\n> +++ b/t/t7004-tag.sh\n> @@ -1007,7 +1007,9 @@ test_expect_failure \\\n> test_expect_success \\\n> \t'message in editor has initial comment' '\n> \tGIT_EDITOR=cat git tag -a initial-comment > actual || true &&\n> -\ttest $(sed -n \"/^\\(#\\|\\$\\)/p\" actual | wc -l) -gt 0\n> +\tgrep -e \"^$\" actual > /dev/null 2>&1 &&\n> +\tgrep -e \"^#\" actual > /dev/null 2>&1 &&\n> +\t! grep -e \"^[^#]\" actual > /dev/null 2>&1\n> '\n>\n> get_tag_header reuse $commit commit $time >expect\n\nIf your system has a retarded `sed', it will most likely also have a \nbrain-dead `/bin/sh' which doesn't handle `! command'. So I suggest \nyou rewrite the last line as:\n grep -ve \"^[^#]\" actual > /dev/null 2>&1\n\nCheers,\n\n-- \nBenoit Sigoure aka Tsuna\nEPITA Research and Development Laboratory\n\n\n"},{"id":"60099","messageId":"83DEF29B-4E72-4913-A77E-8C00D69B48E4@wincent.com","threadId":"10885","inReplyTo":"1195233971-12288-1-git-send-email-mh@glandium.org","subject":"Re: [PATCH] Fix t7004 which fails with retarded sed","fromName":"Wincent Colaiuta","fromEmail":"win@wincent.com","sentAt":"2007-11-16T18:02:54Z","receivedAt":"2007-11-16T18:02:54Z","isPatch":true,"sender":{"key":"greg@hurrell.net","avatar":"https://avatars.githubusercontent.com/u/7074?v=4"},"body":"El 16/11/2007, a las 18:26, Mike Hommey escribió:\n\n> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\n> index 096fe33..3813f23 100755\n> --- a/t/t7004-tag.sh\n> +++ b/t/t7004-tag.sh\n> @@ -1007,7 +1007,9 @@ test_expect_failure \\\n> test_expect_success \\\n> \t'message in editor has initial comment' '\n> \tGIT_EDITOR=cat git tag -a initial-comment > actual || true &&\n> -\ttest $(sed -n \"/^\\(#\\|\\$\\)/p\" actual | wc -l) -gt 0\n> +\tgrep -e \"^$\" actual > /dev/null 2>&1 &&\n> +\tgrep -e \"^#\" actual > /dev/null 2>&1 &&\n> +\t! grep -e \"^[^#]\" actual > /dev/null 2>&1\n> '\n\nYep, that works here, and is a much more rigourous test than what we \nhad.\n\nCheers,\nWincent\n"},{"id":"60101","messageId":"7vpryaxczx.fsf@gitster.siamese.dyndns.org","threadId":"10885","inReplyTo":"1195233971-12288-1-git-send-email-mh@glandium.org","subject":"Re: [PATCH] Fix t7004 which fails with retarded sed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-16T18:26:58Z","receivedAt":"2007-11-16T18:26:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Hommey writes:\n\n> Brown paper bag fix to avoid test failure with retarded sed. The test\n> by itself didn't catch what it was supposed to, anyways.\n\nI'd rather not to say \"retarded sed\". It is not fair to blame\nthe implementation when the script we feed to it is not\nportable.\n\n> test_expect_success \\\n> \t'message in editor has initial comment' '\n> \tGIT_EDITOR=cat git tag -a initial-comment > actual || true &&\n> -\ttest $(sed -n \"/^\\(#\\|\\$\\)/p\" actual | wc -l) -gt 0\n> +\tgrep -e \"^$\" actual > /dev/null 2>&1 &&\n> +\tgrep -e \"^#\" actual > /dev/null 2>&1 &&\n> +\t! grep -e \"^[^#]\" actual > /dev/null 2>&1\n\nThe test simulates a case where\n\n\t$ git tag -a initial-comment\n\nis run and the user exits the editor without editing the file to\nadd any text to annotate the tag. The behaviour we want from\nsuch an invocation is (1) the user sees a sensible template in\nthe editor, and (2) the command to error out, saying \"Hey, you\ndid not leave any message for us to use\".\n\nSo the earlier part of the test\n\n\tGIT_EDITOR=cat git tag -a initial-comment > actual || true &&\n\nis already bogus with respect to the latter point. It will\nhappily continue if \"git tag\" erroneously returns success, and\nthe above does not catch it.\n\n\tif GIT_EDITOR=cat git tag -a initial-comment >actual\n then\n \techo >&2 oops we should have errored out\n false\n\telse\n \t: happy -- anything else we want to check?\n\tfi\n\nThe \"actual\" file contains what the user saw in the editor and\nreturned to the \"git tag\" command.\n\nThe template we give to the user begins with a blank line,\nfollowed by a brief instruction \"write a tag message\" as\ncomments to be stripped. The test is trying to make sure that\nis what was given to the user. As you already discussed in the\nthread, the exact wording may change in the future and we would\nnot want to adjust the test every time.\n\nI think the important points about this template are:\n\n * It begins with a single blank line, where the invoked editor\n would typically place the editing curser at so that the user\n can immediately start typing;\n\n * It has some instruction but that comes after that initial\n blank line, all lines prefixed with \"#\". I do not think we\n would want to check the wording of this instruction.\n\n * And it has nothing else, as the expected behaviour is \"Hey\n you did not leave any message\".\n"},{"id":"60105","messageId":"20071116191533.GA7624@glandium.org","threadId":"10885","inReplyTo":"6E925AF7-FCCB-4793-936F-7BC513272E75@lrde.epita.fr","subject":"Re: [PATCH] Fix t7004 which fails with retarded sed","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2007-11-16T19:15:33Z","receivedAt":"2007-11-16T19:15:33Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Fri, Nov 16, 2007 at 06:58:21PM +0100, Benoit Sigoure wrote:\n> On Nov 16, 2007, at 6:26 PM, Mike Hommey wrote:\n>\n>> Brown paper bag fix to avoid test failure with retarded sed. The test\n>> by itself didn't catch what it was supposed to, anyways.\n>>\n>> So now, we test whether the editor gets at least an empty line, some\n>> commented lines, and doesn't get anything else.\n>>\n>> Signed-off-by: Mike Hommey \n>> ---\n>> t/t7004-tag.sh | 4 +++-\n>> 1 files changed, 3 insertions(+), 1 deletions(-)\n>>\n>> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\n>> index 096fe33..3813f23 100755\n>> --- a/t/t7004-tag.sh\n>> +++ b/t/t7004-tag.sh\n>> @@ -1007,7 +1007,9 @@ test_expect_failure \\\n>> test_expect_success \\\n>> \t'message in editor has initial comment' '\n>> \tGIT_EDITOR=cat git tag -a initial-comment > actual || true &&\n>> -\ttest $(sed -n \"/^\\(#\\|\\$\\)/p\" actual | wc -l) -gt 0\n>> +\tgrep -e \"^$\" actual > /dev/null 2>&1 &&\n>> +\tgrep -e \"^#\" actual > /dev/null 2>&1 &&\n>> +\t! grep -e \"^[^#]\" actual > /dev/null 2>&1\n>> '\n>>\n>> get_tag_header reuse $commit commit $time >expect\n>\n> If your system has a retarded `sed', it will most likely also have a \n> brain-dead `/bin/sh' which doesn't handle `! command'. So I suggest you \n> rewrite the last line as:\n> grep -ve \"^[^#]\" actual > /dev/null 2>&1\n\nA whole lot of the test suite uses `! command', which is why i chose to\nuse it.\n\nMike\n"},{"id":"60106","messageId":"20071116191715.GB7624@glandium.org","threadId":"10885","inReplyTo":"7vpryaxczx.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Fix t7004 which fails with retarded sed","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2007-11-16T19:17:15Z","receivedAt":"2007-11-16T19:17:15Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Fri, Nov 16, 2007 at 10:26:58AM -0800, Junio C Hamano wrote:\n> Mike Hommey writes:\n> \n> > Brown paper bag fix to avoid test failure with retarded sed. The test\n> > by itself didn't catch what it was supposed to, anyways.\n> \n> I'd rather not to say \"retarded sed\". It is not fair to blame\n> the implementation when the script we feed to it is not\n> portable.\n> \n> > test_expect_success \\\n> > \t'message in editor has initial comment' '\n> > \tGIT_EDITOR=cat git tag -a initial-comment > actual || true &&\n> > -\ttest $(sed -n \"/^\\(#\\|\\$\\)/p\" actual | wc -l) -gt 0\n> > +\tgrep -e \"^$\" actual > /dev/null 2>&1 &&\n> > +\tgrep -e \"^#\" actual > /dev/null 2>&1 &&\n> > +\t! grep -e \"^[^#]\" actual > /dev/null 2>&1\n> \n> The test simulates a case where\n> \n> \t$ git tag -a initial-comment\n> \n> is run and the user exits the editor without editing the file to\n> add any text to annotate the tag. The behaviour we want from\n> such an invocation is (1) the user sees a sensible template in\n> the editor, and (2) the command to error out, saying \"Hey, you\n> did not leave any message for us to use\".\n> \n> So the earlier part of the test\n> \n> \tGIT_EDITOR=cat git tag -a initial-comment > actual || true &&\n> \n> is already bogus with respect to the latter point. It will\n> happily continue if \"git tag\" erroneously returns success, and\n> the above does not catch it.\n> \n> \tif GIT_EDITOR=cat git tag -a initial-comment >actual\n> then\n> \techo >&2 oops we should have errored out\n> false\n> \telse\n> \t: happy -- anything else we want to check?\n> \tfi\n\nThis should probably be tested in another test block.\n\n> The \"actual\" file contains what the user saw in the editor and\n> returned to the \"git tag\" command.\n> \n> The template we give to the user begins with a blank line,\n> followed by a brief instruction \"write a tag message\" as\n> comments to be stripped. The test is trying to make sure that\n> is what was given to the user. As you already discussed in the\n> thread, the exact wording may change in the future and we would\n> not want to adjust the test every time.\n> \n> I think the important points about this template are:\n> \n> * It begins with a single blank line, where the invoked editor\n> would typically place the editing curser at so that the user\n> can immediately start typing;\n> \n> * It has some instruction but that comes after that initial\n> blank line, all lines prefixed with \"#\". I do not think we\n> would want to check the wording of this instruction.\n> \n> * And it has nothing else, as the expected behaviour is \"Hey\n> you did not leave any message\".\n\nWhich is roughly what my patch does, except it doesn't check for\nordering.\n\nMike\n"},{"id":"60107","messageId":"7vlk8yx9rp.fsf@gitster.siamese.dyndns.org","threadId":"10885","inReplyTo":"20071116191715.GB7624@glandium.org","subject":"Re: [PATCH] Fix t7004 which fails with retarded sed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-16T19:36:42Z","receivedAt":"2007-11-16T19:36:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Hommey writes:\n\n>> So the earlier part of the test\n>> \n>> \tGIT_EDITOR=cat git tag -a initial-comment > actual || true &&\n>> \n>> is already bogus with respect to the latter point. It will\n>> happily continue if \"git tag\" erroneously returns success, and\n>> the above does not catch it.\n>> \n>> \tif GIT_EDITOR=cat git tag -a initial-comment >actual\n>> then\n>> \techo >&2 oops we should have errored out\n>> false\n>> \telse\n>> \t: happy -- anything else we want to check?\n>> \tfi\n>\n> This should probably be tested in another test block.\n\nFair enough; please make it so.\n\n> Which is roughly what my patch does, except it doesn't check for\n> ordering.\n\n... which I think is more important part. If the blank like\nwere at the end, that would be irritating for the user, wouldn't\nit?\n"},{"id":"60108","messageId":"1195244917-25659-1-git-send-email-mh@glandium.org","threadId":"10885","inReplyTo":"7vlk8yx9rp.fsf@gitster.siamese.dyndns.org","subject":"[PATCH] Fix and improve t7004","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2007-11-16T20:28:37Z","receivedAt":"2007-11-16T20:28:37Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"Brown paper bag fix to avoid using non portable sed syntax. The\ntest by itself didn't catch what it was supposed to, anyways.\n\nThe new test first checks whether the user exited the editor\nwithout editing the file, then whether what the user was\npresented in the editor was any useful to her, which we define\nas the following:\n * It begins with a single blank line, where the invoked editor\n would typically place the editing curser at so that the user\n can immediately start typing;\n\n * It has some instruction but that comes after that initial\n blank line, all lines prefixed with \"#\".\n\n * And it has nothing else, as the expected behaviour is \"Hey\n you did not leave any message\".\n\nSigned-off-by: Mike Hommey \n---\n t/t7004-tag.sh | 9 ++++++++-\n 1 files changed, 8 insertions(+), 1 deletions(-)\n\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex 096fe33..42b1f97 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1004,10 +1004,17 @@ test_expect_failure \\\n \t'verify signed tag fails when public key is not present' \\\n \t'git-tag -v signed-tag'\n \n+test_expect_failure \\\n+\t'git-tag -a fails if tag annotation is empty' '\n+\tGIT_EDITOR=cat git tag -a initial-comment > /dev/null 2>&1\n+'\n+\n test_expect_success \\\n \t'message in editor has initial comment' '\n \tGIT_EDITOR=cat git tag -a initial-comment > actual || true &&\n-\ttest $(sed -n \"/^\\(#\\|\\$\\)/p\" actual | wc -l) -gt 0\n+\t( read empty ;\n+\t [ \"$empty\" ] && exit 1 ;\n+\t ! grep -ve \"^#\" > /dev/null 2>&1 ) < actual\n '\n \n get_tag_header reuse $commit commit $time >expect\n-- \n1.5.3.5\n"},{"id":"60109","messageId":"EEE710FA-0408-489F-8128-B4C1F06D34FF@lrde.epita.fr","threadId":"10885","inReplyTo":"1195244917-25659-1-git-send-email-mh@glandium.org","subject":"Re: [PATCH] Fix and improve t7004","fromName":"Benoit Sigoure","fromEmail":"tsuna@lrde.epita.fr","sentAt":"2007-11-16T21:04:57Z","receivedAt":"2007-11-16T21:04:57Z","isPatch":true,"sender":{"key":"tsunanet@gmail.com","avatar":"https://avatars.githubusercontent.com/u/128281?v=4"},"body":"On Nov 16, 2007, at 9:28 PM, Mike Hommey wrote:\n\n> Brown paper bag fix to avoid using non portable sed syntax. The\n> test by itself didn't catch what it was supposed to, anyways.\n>\n> The new test first checks whether the user exited the editor\n> without editing the file, then whether what the user was\n> presented in the editor was any useful to her, which we define\n> as the following:\n> * It begins with a single blank line, where the invoked editor\n> would typically place the editing curser at so that the user\n> can immediately start typing;\n>\n> * It has some instruction but that comes after that initial\n> blank line, all lines prefixed with \"#\".\n>\n> * And it has nothing else, as the expected behaviour is \"Hey\n> you did not leave any message\".\n>\n> Signed-off-by: Mike Hommey \n> ---\n> t/t7004-tag.sh | 9 ++++++++-\n> 1 files changed, 8 insertions(+), 1 deletions(-)\n>\n> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\n> index 096fe33..42b1f97 100755\n> --- a/t/t7004-tag.sh\n> +++ b/t/t7004-tag.sh\n> @@ -1004,10 +1004,17 @@ test_expect_failure \\\n> \t'verify signed tag fails when public key is not present' \\\n> \t'git-tag -v signed-tag'\n>\n> +test_expect_failure \\\n> +\t'git-tag -a fails if tag annotation is empty' '\n> +\tGIT_EDITOR=cat git tag -a initial-comment > /dev/null 2>&1\n> +'\n> +\n> test_expect_success \\\n> \t'message in editor has initial comment' '\n> \tGIT_EDITOR=cat git tag -a initial-comment > actual || true &&\n> -\ttest $(sed -n \"/^\\(#\\|\\$\\)/p\" actual | wc -l) -gt 0\n> +\t( read empty ;\n> +\t [ \"$empty\" ] && exit 1 ;\n\nWhat is this meant to do? Did you mean [ -n \"$empty\" ] ?\n\n> +\t ! grep -ve \"^#\" > /dev/null 2>&1 ) < actual\n\nThe double negation is harder to read. May I suggest something along \nthese lines (which seems more readable to me):\n while read line; do\n case $line in #(\n '#'*) ;; # Accept comments (\n *) exit 1;;\n esac\n done\n\nAdvantages:\n The purpose of the test is more obvious\n The test is more easily extendable\n The test saves a fork ;o\n\n> '\n>\n> get_tag_header reuse $commit commit $time >expect\n\nCheers,\n\n-- \nBenoit Sigoure aka Tsuna\nEPITA Research and Development Laboratory\n\n\n"},{"id":"60110","messageId":"20071116211148.GA28966@glandium.org","threadId":"10885","inReplyTo":"EEE710FA-0408-489F-8128-B4C1F06D34FF@lrde.epita.fr","subject":"Re: [PATCH] Fix and improve t7004","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2007-11-16T21:11:48Z","receivedAt":"2007-11-16T21:11:48Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Fri, Nov 16, 2007 at 10:04:57PM +0100, Benoit Sigoure wrote:\n> On Nov 16, 2007, at 9:28 PM, Mike Hommey wrote:\n>\n>> Brown paper bag fix to avoid using non portable sed syntax. The\n>> test by itself didn't catch what it was supposed to, anyways.\n>>\n>> The new test first checks whether the user exited the editor\n>> without editing the file, then whether what the user was\n>> presented in the editor was any useful to her, which we define\n>> as the following:\n>> * It begins with a single blank line, where the invoked editor\n>> would typically place the editing curser at so that the user\n>> can immediately start typing;\n>>\n>> * It has some instruction but that comes after that initial\n>> blank line, all lines prefixed with \"#\".\n>>\n>> * And it has nothing else, as the expected behaviour is \"Hey\n>> you did not leave any message\".\n>>\n>> Signed-off-by: Mike Hommey \n>> ---\n>> t/t7004-tag.sh | 9 ++++++++-\n>> 1 files changed, 8 insertions(+), 1 deletions(-)\n>>\n>> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\n>> index 096fe33..42b1f97 100755\n>> --- a/t/t7004-tag.sh\n>> +++ b/t/t7004-tag.sh\n>> @@ -1004,10 +1004,17 @@ test_expect_failure \\\n>> \t'verify signed tag fails when public key is not present' \\\n>> \t'git-tag -v signed-tag'\n>>\n>> +test_expect_failure \\\n>> +\t'git-tag -a fails if tag annotation is empty' '\n>> +\tGIT_EDITOR=cat git tag -a initial-comment > /dev/null 2>&1\n>> +'\n>> +\n>> test_expect_success \\\n>> \t'message in editor has initial comment' '\n>> \tGIT_EDITOR=cat git tag -a initial-comment > actual || true &&\n>> -\ttest $(sed -n \"/^\\(#\\|\\$\\)/p\" actual | wc -l) -gt 0\n>> +\t( read empty ;\n>> +\t [ \"$empty\" ] && exit 1 ;\n>\n> What is this meant to do? Did you mean [ -n \"$empty\" ] ?\n\nReplacing with [ -n \"$empty\" ] would not work properly, except if you\nreplace the following ; with &&. Does that really make a readability\ndifference ?\n\n>> +\t ! grep -ve \"^#\" > /dev/null 2>&1 ) < actual\n>\n> The double negation is harder to read. May I suggest something along these \n> lines (which seems more readable to me):\n> while read line; do\n> case $line in #(\n> '#'*) ;; # Accept comments (\n> *) exit 1;;\n> esac\n> done\n\nI'm not really convinced. What do other people have to say ?\n\nMike\n"},{"id":"60111","messageId":"EC3B30DC-E81C-4ECA-BE7F-F237E1338603@lrde.epita.fr","threadId":"10885","inReplyTo":"20071116211148.GA28966@glandium.org","subject":"Re: [PATCH] Fix and improve t7004","fromName":"Benoit Sigoure","fromEmail":"tsuna@lrde.epita.fr","sentAt":"2007-11-16T21:31:15Z","receivedAt":"2007-11-16T21:31:15Z","isPatch":true,"sender":{"key":"tsunanet@gmail.com","avatar":"https://avatars.githubusercontent.com/u/128281?v=4"},"body":"On Nov 16, 2007, at 10:11 PM, Mike Hommey wrote:\n\n> On Fri, Nov 16, 2007 at 10:04:57PM +0100, Benoit Sigoure wrote:\n>> On Nov 16, 2007, at 9:28 PM, Mike Hommey wrote:\n>>> test_expect_success \\\n>>> \t'message in editor has initial comment' '\n>>> \tGIT_EDITOR=cat git tag -a initial-comment > actual || true &&\n>>> -\ttest $(sed -n \"/^\\(#\\|\\$\\)/p\" actual | wc -l) -gt 0\n>>> +\t( read empty ;\n>>> +\t [ \"$empty\" ] && exit 1 ;\n>>\n>> What is this meant to do? Did you mean [ -n \"$empty\" ] ?\n>\n> Replacing with [ -n \"$empty\" ] would not work properly, except if you\n> replace the following ; with &&. Does that really make a readability\n> difference ?\n\nI don't get it. As far as I understand, you're trying to check \nwhether $empty is indeed empty, right? So how is `[ \"$empty\" ]' \nmeant to work?\n [ -n \"$empty\" ] && exit 1\n\nwill exit 1 if empty isn't empty.\n\n-- \nBenoit Sigoure aka Tsuna\nEPITA Research and Development Laboratory\n\n\n"},{"id":"60112","messageId":"20071116213538.GA30076@glandium.org","threadId":"10885","inReplyTo":"EC3B30DC-E81C-4ECA-BE7F-F237E1338603@lrde.epita.fr","subject":"Re: [PATCH] Fix and improve t7004","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2007-11-16T21:35:38Z","receivedAt":"2007-11-16T21:35:38Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Fri, Nov 16, 2007 at 10:31:15PM +0100, Benoit Sigoure wrote:\n> On Nov 16, 2007, at 10:11 PM, Mike Hommey wrote:\n>\n>> On Fri, Nov 16, 2007 at 10:04:57PM +0100, Benoit Sigoure wrote:\n>>> On Nov 16, 2007, at 9:28 PM, Mike Hommey wrote:\n>>>> test_expect_success \\\n>>>> \t'message in editor has initial comment' '\n>>>> \tGIT_EDITOR=cat git tag -a initial-comment > actual || true &&\n>>>> -\ttest $(sed -n \"/^\\(#\\|\\$\\)/p\" actual | wc -l) -gt 0\n>>>> +\t( read empty ;\n>>>> +\t [ \"$empty\" ] && exit 1 ;\n>>>\n>>> What is this meant to do? Did you mean [ -n \"$empty\" ] ?\n>>\n>> Replacing with [ -n \"$empty\" ] would not work properly, except if you\n>> replace the following ; with &&. Does that really make a readability\n>> difference ?\n>\n> I don't get it. As far as I understand, you're trying to check whether \n> $empty is indeed empty, right? So how is `[ \"$empty\" ]' meant to work?\n> [ -n \"$empty\" ] && exit 1\n>\n> will exit 1 if empty isn't empty.\n\nSorry, I read '-z', not '-n'. [ \"$empty\" ] and [ -n \"$empty\" ] are the\nsame thing.\n\nMike\n"},{"id":"60113","messageId":"7vbq9tyii3.fsf@gitster.siamese.dyndns.org","threadId":"10885","inReplyTo":"20071116211148.GA28966@glandium.org","subject":"Re: [PATCH] Fix and improve t7004","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-16T21:42:44Z","receivedAt":"2007-11-16T21:42:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Hommey writes:\n\n>>> +\t( read empty ;\n>>> +\t [ \"$empty\" ] && exit 1 ;\n>>\n>> What is this meant to do? Did you mean [ -n \"$empty\" ] ?\n>\n> Replacing with [ -n \"$empty\" ] would not work properly, except if you\n> replace the following ; with &&. Does that really make a readability\n> difference ?\n>\n>>> +\t ! grep -ve \"^#\" > /dev/null 2>&1 ) < actual\n>>\n>> The double negation is harder to read. May I suggest something along these \n>> lines (which seems more readable to me):\n>> while read line; do\n>> case $line in #(\n>> '#'*) ;; # Accept comments (\n>> *) exit 1;;\n>> esac\n>> done\n>\n> I'm not really convinced. What do other people have to say ?\n\nAs shell \"read\" loses information (a backslash sequence is\ninterpreted, and trailing whitespaces are stripped and not\nassigned to \"line\" above), it is not such a good vehicle if you\nwant to make a reasonably strict test on top of. Some shells\ndo not implement \"read -r\" either, so it is also a portability\nhassle.\n\nPerhaps...\n\n\t# check the first line --- should be empty\n\tfirst=$(sed -e 1q On Fri, Nov 16, 2007 at 10:31:15PM +0100, Benoit Sigoure wrote:\n>> On Nov 16, 2007, at 10:11 PM, Mike Hommey wrote:\n>>\n>>> On Fri, Nov 16, 2007 at 10:04:57PM +0100, Benoit Sigoure wrote:\n>>>> On Nov 16, 2007, at 9:28 PM, Mike Hommey wrote:\n>>>>> test_expect_success \\\n>>>>> \t'message in editor has initial comment' '\n>>>>> \tGIT_EDITOR=cat git tag -a initial-comment > actual || true &&\n>>>>> -\ttest $(sed -n \"/^\\(#\\|\\$\\)/p\" actual | wc -l) -gt 0\n>>>>> +\t( read empty ;\n>>>>> +\t [ \"$empty\" ] && exit 1 ;\n>>>>\n>>>> What is this meant to do? Did you mean [ -n \"$empty\" ] ?\n>>>\n>>> Replacing with [ -n \"$empty\" ] would not work properly, except if \n>>> you\n>>> replace the following ; with &&. Does that really make a readability\n>>> difference ?\n>>\n>> I don't get it. As far as I understand, you're trying to check \n>> whether\n>> $empty is indeed empty, right? So how is `[ \"$empty\" ]' meant to \n>> work?\n>> [ -n \"$empty\" ] && exit 1\n>>\n>> will exit 1 if empty isn't empty.\n>\n> Sorry, I read '-z', not '-n'. [ \"$empty\" ] and [ -n \"$empty\" ] are the\n> same thing.\n\nHeh, forgive my ignorance, I did not know the [ \"string\" ] notation. \nAmazing, after all these years of shell scripting...\n\n-- \nBenoit Sigoure aka Tsuna\nEPITA Research and Development Laboratory\n\n\n"},{"id":"60124","messageId":"1195250528-9727-1-git-send-email-mh@glandium.org","threadId":"10885","inReplyTo":"7vbq9tyii3.fsf@gitster.siamese.dyndns.org","subject":"[PATCH] Fix and improve t7004","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2007-11-16T22:02:08Z","receivedAt":"2007-11-16T22:02:08Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"Brown paper bag fix to avoid using non portable sed syntax. The\ntest by itself didn't catch what it was supposed to, anyways.\n\nThe new test first checks whether the user exited the editor\nwithout editing the file, then whether what the user was\npresented in the editor was any useful to her, which we define\nas the following:\n * It begins with a single blank line, where the invoked editor\n would typically place the editing curser at so that the user\n can immediately start typing;\n\n * It has some instruction but that comes after that initial\n blank line, all lines prefixed with \"#\".\n\n * And it has nothing else, as the expected behaviour is \"Hey\n you did not leave any message\".\n\nSigned-off-by: Mike Hommey \n---\n> Perhaps...\n> \n> # check the first line --- should be empty\n> first=$(sed -e 1q test -z \"$first\" &&\n> # remove commented lines from the remainder -- should be empty\n> rest=$(sed -e 1d -e '/^#/d' test -z \"$rest\"\n\nI like that better. So here is a new version using this. In the end, the\npatch and most of the comment are yours, so maybe you should claim it is\nyour patch.\n\n t/t7004-tag.sh | 12 +++++++++++-\n 1 files changed, 11 insertions(+), 1 deletions(-)\n\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex 096fe33..162bdf7 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1004,10 +1004,20 @@ test_expect_failure \\\n \t'verify signed tag fails when public key is not present' \\\n \t'git-tag -v signed-tag'\n \n+test_expect_failure \\\n+\t'git-tag -a fails if tag annotation is empty' '\n+\tGIT_EDITOR=cat git tag -a initial-comment > /dev/null 2>&1\n+'\n+\n test_expect_success \\\n \t'message in editor has initial comment' '\n \tGIT_EDITOR=cat git tag -a initial-comment > actual || true &&\n-\ttest $(sed -n \"/^\\(#\\|\\$\\)/p\" actual | wc -l) -gt 0\n+\t# check the first line --- should be empty\n+\tfirst=$(sed -e 1q expect\n-- \n1.5.3.5\n"},{"id":"60126","messageId":"7vfxz5uu86.fsf@gitster.siamese.dyndns.org","threadId":"10885","inReplyTo":"1195250528-9727-1-git-send-email-mh@glandium.org","subject":"Re: [PATCH] Fix and improve t7004","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-17T08:55:21Z","receivedAt":"2007-11-17T08:55:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks.\n"}]}