# [BUG] t7004 (master) busted on Leopard 26 messages from 2007-11-15 to 2007-11-17. Participants: Wincent Colaiuta, Johannes Schindelin, David Kastrup, Mike Hommey, Benoit Sigoure, Junio C Hamano. Thread: https://gitlist.dev/t/10885 ## Wincent Colaiuta, 2007-11-15 13:38 Subject: [BUG] t7004 (master) busted on Leopard Message-ID: <17E3F66C-4644-4B64-817C-88062727A2D9@wincent.com> URL: https://gitlist.dev/e/17E3F66C-4644-4B64-817C-88062727A2D9%40wincent.com ``` Commit 4d8b1dc850 added a couple of tests to t7004, and my testing reveals that this one has been broken on Leopard since then: * FAIL 83: message in editor has initial comment GIT_EDITOR=cat git tag -a initial-comment > actual || true && test $(sed -n "/^\(#\|\$\)/p" actual | wc -l) -gt 0 (Not sure whether this affects other platforms, or versions of Mac OS X prior to Leopard, as I only have one machine here.) The problem is the version of sed that ships with Leopard fails to match the initial comment with this syntax: sed -n "/^\(#\|\$\)/p" actual An alternative that works is: sed -n "/^[#\$]/p" actual Can someone with knowledge of sed compatibility across multiple platforms suggest an alternative which will work on a wider range of systems? Cheers, Wincent ``` ## Johannes Schindelin, 2007-11-15 14:37 Subject: Re: [BUG] t7004 (master) busted on Leopard Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0711151434060.30886%40racer.site In-Reply-To: <17E3F66C-4644-4B64-817C-88062727A2D9@wincent.com> ``` Hi, On Thu, 15 Nov 2007, Wincent Colaiuta wrote: > Commit 4d8b1dc850 added a couple of tests to t7004, and my testing reveals > that this one has been broken on Leopard since then: > > * FAIL 83: message in editor has initial comment > GIT_EDITOR=cat git tag -a initial-comment > actual || true && > test $(sed -n "/^\(#\|\$\)/p" actual | wc -l) -gt 0 I think this is our good old friend, MacOSX' sed. (Wasn't there a question today what's wrong with using sed? I think this issue qualifies.) I imagine that it is that MacOSX' sed is adding a trailing newline (not the regexp like you suggested). Which means that "wc -l" would print "1". (You can see for yourself if you run the script with "sh -x ...".) IMHO a good solution would be test -z "$(grep -e '^#' -e '^$' actual)" Could you test, please? Thanks, Dscho ``` ## David Kastrup, 2007-11-15 14:48 Subject: Re: [BUG] t7004 (master) busted on Leopard Message-ID: <86k5oj36pq.fsf@lola.quinscape.zz> URL: https://gitlist.dev/e/86k5oj36pq.fsf%40lola.quinscape.zz In-Reply-To: ``` Johannes Schindelin writes: > Hi, > > On Thu, 15 Nov 2007, Wincent Colaiuta wrote: > >> Commit 4d8b1dc850 added a couple of tests to t7004, and my testing reveals >> that this one has been broken on Leopard since then: >> >> * FAIL 83: message in editor has initial comment >> GIT_EDITOR=cat git tag -a initial-comment > actual || true && >> test $(sed -n "/^\(#\|\$\)/p" actual | wc -l) -gt 0 > > I think this is our good old friend, MacOSX' sed. (Wasn't there a > question today what's wrong with using sed? I think this issue > qualifies.) \| is not portable. $ in groups is not portable. > I imagine that it is that MacOSX' sed is adding a trailing newline (not > the regexp like you suggested). I imagine it is one of the above. Most sed's add trailing newlines. -- David Kastrup ``` ## Wincent Colaiuta, 2007-11-15 15:12 Subject: Re: [BUG] t7004 (master) busted on Leopard Message-ID: URL: https://gitlist.dev/e/A3467E97-601B-45EE-816D-376F4FFE69E8%40wincent.com In-Reply-To: ``` El 15/11/2007, a las 15:37, Johannes Schindelin escribió: > Hi, > > On Thu, 15 Nov 2007, Wincent Colaiuta wrote: > >> Commit 4d8b1dc850 added a couple of tests to t7004, and my testing >> reveals >> that this one has been broken on Leopard since then: >> >> * FAIL 83: message in editor has initial comment >> GIT_EDITOR=cat git tag -a initial-comment > actual || true && >> test $(sed -n "/^\(#\|\$\)/p" actual | wc -l) -gt 0 > > I think this is our good old friend, MacOSX' sed. Yes, that's exactly what I said in the part of my post that you didn't quote. > I imagine that it is that MacOSX' sed is adding a trailing newline > (not > the regexp like you suggested). Which means that "wc -l" would > print "1". > (You can see for yourself if you run the script with "sh -x ...".) Unless I am misreading the test, any output from "wc -l" that is greater than 0 will cause the test to pass, so even if it outputted "1" as you suggest then that wouldn't be the cause of the failure. I do think the cause of the failure is the limited regexp syntax that sed accepts on Leopard; witness: on Mac OS X the following prints 0: echo "# hello" | sed -n "/^\(#\|\$\)/p" | wc -l Whereas the following prints 1: echo "# hello" | sed -n "/^[#\$]/p" | wc -l Here's the output for the failing test run under "sh -x" as you suggest, although I must admit that I can't really parse it myself. + test_expect_success 'message in editor has initial comment' ' GIT_EDITOR=cat git tag -a initial-comment > actual || true && test $(sed -n "/^\(#\|\$\)/p" actual | wc -l) -gt 0 ' + test 2 = 2 + test_skip 'message in editor has initial comment' ' GIT_EDITOR=cat git tag -a initial-comment > actual || true && test $(sed -n "/^\(#\|\$\)/p" actual | wc -l) -gt 0 ' ++ expr ./t7004-tag.sh : '.*/\(t[0-9]*\)-[^/]*$' + this_test=t7004 ++ expr 82 + 1 + this_test=t7004.83 + to_skip= + case "$to_skip" in + false + say 'expecting success: GIT_EDITOR=cat git tag -a initial-comment > actual || true && test $(sed -n "/^\(#\|\$\)/p" actual | wc -l) -gt 0 ' + say_color info 'expecting success: GIT_EDITOR=cat git tag -a initial-comment > actual || true && test $(sed -n "/^\(#\|\$\)/p" actual | wc -l) -gt 0 ' + case "$1" in + tput setaf 3 + shift + echo '* expecting success: GIT_EDITOR=cat git tag -a initial-comment > actual || true && test $(sed -n "/^\(#\|\$\)/p" actual | wc -l) -gt 0 ' + tput sgr0 + test_run_ ' GIT_EDITOR=cat git tag -a initial-comment > actual || true && test $(sed -n "/^\(#\|\$\)/p" actual | wc -l) -gt 0 ' + eval ' GIT_EDITOR=cat git tag -a initial-comment > actual || true && test $(sed -n "/^\(#\|\$\)/p" actual | wc -l) -gt 0 ' + eval_ret=1 + return 0 + '[' 0 = 0 -a 1 = 0 ']' + test_failure_ 'message in editor has initial comment' ' GIT_EDITOR=cat git tag -a initial-comment > actual || true && test $(sed -n "/^\(#\|\$\)/p" actual | wc -l) -gt 0 ' ++ expr 82 + 1 + test_count=83 ++ expr 0 + 1 + test_failure=1 + say_color error 'FAIL 83: message in editor has initial comment' + case "$1" in + tput bold + tput setaf 1 + shift + echo '* FAIL 83: message in editor has initial comment' * FAIL 83: message in editor has initial comment + tput sgr0 + shift > IMHO a good solution would be > > test -z "$(grep -e '^#' -e '^$' actual)" > > Could you test, please? Yes, the test passes with that, although it has to be written as follows in the actual test file seeing as it's inside a single-quoted string: test -z "$(grep -e \"^#\" -e \"^$\" actual)" Will follow this up with a patch that implements your proposed fix. Cheers, Wincent ``` ## Wincent Colaiuta, 2007-11-15 15:16 Subject: [PATCH] Fix git-tag test breakage caused by broken sed on Leopard Message-ID: <90D416F6-A3CA-43C8-B71F-38EA955A762B@wincent.com> URL: https://gitlist.dev/e/90D416F6-A3CA-43C8-B71F-38EA955A762B%40wincent.com In-Reply-To: ``` The 'message in editor has initial comment' test fails on Leopard (and possibly on other versions of Mac OS X as well) due to the limited sed syntax available on that platform. Avoid the breakage by using grep instead (suggested by Johannes Schindelin). Signed-off-by: Wincent Colaiuta --- t/t7004-tag.sh | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh index 096fe33..02ec9c3 100755 --- a/t/t7004-tag.sh +++ b/t/t7004-tag.sh @@ -1007,7 +1007,7 @@ test_expect_failure \ test_expect_success \ 'message in editor has initial comment' ' GIT_EDITOR=cat git tag -a initial-comment > actual || true && - test $(sed -n "/^\(#\|\$\)/p" actual | wc -l) -gt 0 + test -z "$(grep -e \"^#\" -e \"^$\" actual)" ' get_tag_header reuse $commit commit $time >expect -- 1.5.3.5 ``` ## Wincent Colaiuta, 2007-11-15 15:47 Subject: [PATCH v2] Fix git-tag test breakage caused by broken sed on Leopard Message-ID: URL: https://gitlist.dev/e/C9DFAD77-DFEF-47AC-83D5-CFDF245D95E4%40wincent.com In-Reply-To: <90D416F6-A3CA-43C8-B71F-38EA955A762B@wincent.com> ``` The 'message in editor has initial comment' test fails on Leopard (and possibly on other versions of Mac OS X as well) due to the limited sed syntax available on that platform. Avoid the breakage by using grep instead (suggested by Johannes Schindelin). Signed-off-by: Wincent Colaiuta --- The patch I previously sent had the test sense inverted (it used 'test -z' to test for an empty string when we should have actually been looking for a non-empty string), so it really only passed by mistake. This revised version maintains the sense of the original, sed-based test. t/t7004-tag.sh | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh index 096fe33..b54c2e0 100755 --- a/t/t7004-tag.sh +++ b/t/t7004-tag.sh @@ -1007,7 +1007,7 @@ test_expect_failure \ test_expect_success \ 'message in editor has initial comment' ' GIT_EDITOR=cat git tag -a initial-comment > actual || true && - test $(sed -n "/^\(#\|\$\)/p" actual | wc -l) -gt 0 + test $(grep -e "^#" -e "^\$" actual | wc -l ) -gt 0 ' get_tag_header reuse $commit commit $time >expect -- 1.5.3.5 ``` ## Wincent Colaiuta, 2007-11-16 13:45 Subject: Re: [PATCH v2] Fix git-tag test breakage caused by broken sed on Leopard Message-ID: <7A6820DE-C6EE-4E3E-A97F-97766F682B85@wincent.com> URL: https://gitlist.dev/e/7A6820DE-C6EE-4E3E-A97F-97766F682B85%40wincent.com In-Reply-To: <7v4pfm3h6f.fsf@gitster.siamese.dyndns.org> ``` El 16/11/2007, a las 6:14, Junio C Hamano escribió: > Wincent Colaiuta writes: > >> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh >> index 096fe33..b54c2e0 100755 >> --- a/t/t7004-tag.sh >> +++ b/t/t7004-tag.sh >> @@ -1007,7 +1007,7 @@ test_expect_failure \ >> test_expect_success \ >> 'message in editor has initial comment' ' >> GIT_EDITOR=cat git tag -a initial-comment > actual || true && >> - test $(sed -n "/^\(#\|\$\)/p" actual | wc -l) -gt 0 >> + test $(grep -e "^#" -e "^\$" actual | wc -l ) -gt 0 >> ' > > Heh, doesn't grep exit with zero only when it found some lines > that match the pattern already? What's that "wc -l" for? I was just trying to make the minimal change (swapping grep for sed), but if you want a shorter version then we don't even need the "test"; it could just be: - test $(sed -n "/^\(#\|\$\)/p" actual | wc -l) -gt 0 + grep -e "^#" -e "^\$" actual Cheers, Wincent ``` ## Wincent Colaiuta, 2007-11-16 13:48 Subject: Re: [PATCH v2] Fix git-tag test breakage caused by broken sed on Leopard Message-ID: <03D3DFCD-B5F7-47CF-AFD2-F1408BB11AB3@wincent.com> URL: https://gitlist.dev/e/03D3DFCD-B5F7-47CF-AFD2-F1408BB11AB3%40wincent.com In-Reply-To: <7A6820DE-C6EE-4E3E-A97F-97766F682B85@wincent.com> ``` El 16/11/2007, a las 14:45, Wincent Colaiuta escribió: > El 16/11/2007, a las 6:14, Junio C Hamano escribió: > >> Wincent Colaiuta writes: >> >>> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh >>> index 096fe33..b54c2e0 100755 >>> --- a/t/t7004-tag.sh >>> +++ b/t/t7004-tag.sh >>> @@ -1007,7 +1007,7 @@ test_expect_failure \ >>> test_expect_success \ >>> 'message in editor has initial comment' ' >>> GIT_EDITOR=cat git tag -a initial-comment > actual || true && >>> - test $(sed -n "/^\(#\|\$\)/p" actual | wc -l) -gt 0 >>> + test $(grep -e "^#" -e "^\$" actual | wc -l ) -gt 0 >>> ' >> >> Heh, doesn't grep exit with zero only when it found some lines >> that match the pattern already? What's that "wc -l" for? > > > I was just trying to make the minimal change (swapping grep for > sed), but if you want a shorter version then we don't even need the > "test"; it could just be: > > - test $(sed -n "/^\(#\|\$\)/p" actual | wc -l) -gt 0 > + grep -e "^#" -e "^\$" actual Although I don't know if we should be testing for empty lines there because an 0-byte empty "actual" file would spuriously pass the test. Perhaps this would be better: - test $(sed -n "/^\(#\|\$\)/p" actual | wc -l) -gt 0 + grep -e "^#" actual Cheers, Wincent ``` ## Mike Hommey, 2007-11-16 16:59 Subject: Re: [PATCH v2] Fix git-tag test breakage caused by broken sed on Leopard Message-ID: <20071116165944.GB29272@glandium.org> URL: https://gitlist.dev/e/20071116165944.GB29272%40glandium.org In-Reply-To: <03D3DFCD-B5F7-47CF-AFD2-F1408BB11AB3@wincent.com> ``` On Fri, Nov 16, 2007 at 02:48:09PM +0100, Wincent Colaiuta wrote: > El 16/11/2007, a las 14:45, Wincent Colaiuta escribió: > > > El 16/11/2007, a las 6:14, Junio C Hamano escribió: > > > >> Wincent Colaiuta writes: > >> > >>> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh > >>> index 096fe33..b54c2e0 100755 > >>> --- a/t/t7004-tag.sh > >>> +++ b/t/t7004-tag.sh > >>> @@ -1007,7 +1007,7 @@ test_expect_failure \ > >>> test_expect_success \ > >>> 'message in editor has initial comment' ' > >>> GIT_EDITOR=cat git tag -a initial-comment > actual || true && > >>> - test $(sed -n "/^\(#\|\$\)/p" actual | wc -l) -gt 0 > >>> + test $(grep -e "^#" -e "^\$" actual | wc -l ) -gt 0 > >>> ' > >> > >> Heh, doesn't grep exit with zero only when it found some lines > >> that match the pattern already? What's that "wc -l" for? > > > > > > I was just trying to make the minimal change (swapping grep for > > sed), but if you want a shorter version then we don't even need the > > "test"; it could just be: > > > > - test $(sed -n "/^\(#\|\$\)/p" actual | wc -l) -gt 0 > > + grep -e "^#" -e "^\$" actual > > Although I don't know if we should be testing for empty lines there > because an 0-byte empty "actual" file would spuriously pass the test. > Perhaps this would be better: > > - test $(sed -n "/^\(#\|\$\)/p" actual | wc -l) -gt 0 > + grep -e "^#" actual Matching both would as in your previous pseudo patch wouldn't catch empty file. On the other hand, both my initial bloated version and yours won't catch a file that doesn't contain the comment. grep -e "^$" actual && grep -e "^#" actual would actually be a better test. Mike ``` ## Wincent Colaiuta, 2007-11-16 17:25 Subject: Re: [PATCH v2] Fix git-tag test breakage caused by broken sed on Leopard Message-ID: URL: https://gitlist.dev/e/E4B5D2FD-5757-4178-B77C-1D61E7B3F589%40wincent.com In-Reply-To: <20071116165944.GB29272@glandium.org> ``` El 16/11/2007, a las 17:59, Mike Hommey escribió: > On Fri, Nov 16, 2007 at 02:48:09PM +0100, Wincent Colaiuta wrote: >> El 16/11/2007, a las 14:45, Wincent Colaiuta escribió: >> >>> El 16/11/2007, a las 6:14, Junio C Hamano escribió: >>> >>>> Wincent Colaiuta writes: >>>> >>>>> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh >>>>> index 096fe33..b54c2e0 100755 >>>>> --- a/t/t7004-tag.sh >>>>> +++ b/t/t7004-tag.sh >>>>> @@ -1007,7 +1007,7 @@ test_expect_failure \ >>>>> test_expect_success \ >>>>> 'message in editor has initial comment' ' >>>>> GIT_EDITOR=cat git tag -a initial-comment > actual || true && >>>>> - test $(sed -n "/^\(#\|\$\)/p" actual | wc -l) -gt 0 >>>>> + test $(grep -e "^#" -e "^\$" actual | wc -l ) -gt 0 >>>>> ' >>>> >>>> Heh, doesn't grep exit with zero only when it found some lines >>>> that match the pattern already? What's that "wc -l" for? >>> >>> >>> I was just trying to make the minimal change (swapping grep for >>> sed), but if you want a shorter version then we don't even need the >>> "test"; it could just be: >>> >>> - test $(sed -n "/^\(#\|\$\)/p" actual | wc -l) -gt 0 >>> + grep -e "^#" -e "^\$" actual >> >> Although I don't know if we should be testing for empty lines there >> because an 0-byte empty "actual" file would spuriously pass the test. >> Perhaps this would be better: >> >> - test $(sed -n "/^\(#\|\$\)/p" actual | wc -l) -gt 0 >> + grep -e "^#" actual > > Matching both would as in your previous pseudo patch wouldn't catch > empty file. On the other hand, both my initial bloated version and > yours > won't catch a file that doesn't contain the comment. But if git-tag works then it will contain a comment; and isn't the purpose of the test to confirm the comment's presence? > grep -e "^$" actual && grep -e "^#" actual would actually be a better > test. What are we really trying to test here? The test is labelled as 'message in editor has initial comment'. Basically the editor will be prepopulated with a blank line followed by this: # # Write a tag message # So it's the presence of that text which we want to confirm. If I understand the intent of the original sed-based test, it was to confirm that there were 1 or more lines that started with "#" or were empty. It also suffered from the bug that an empty message (by which I mean a 1-byte file containing only an LF) would pass the test (spuriously in my opinion), and my first attempt faithfully translated that bug into grep syntax. The alternative you suggest will (correctly) fail on a zero byte file, and pass on 1-byte file containing only a LF, just like the other approaches. So I'm still wondering, what is it that we're trying to test here? We could test for the exact "Write a tag message" text, but that may be brittle (must be updated if/when the text ever changes), but looking at other tests I see there are some which do precise equality tests for expected results. Cheers, Wincent ``` ## Mike Hommey, 2007-11-16 17:26 Subject: [PATCH] Fix t7004 which fails with retarded sed Message-ID: <1195233971-12288-1-git-send-email-mh@glandium.org> URL: https://gitlist.dev/e/1195233971-12288-1-git-send-email-mh%40glandium.org In-Reply-To: <20071116165944.GB29272@glandium.org> ``` Brown paper bag fix to avoid test failure with retarded sed. The test by itself didn't catch what it was supposed to, anyways. So now, we test whether the editor gets at least an empty line, some commented lines, and doesn't get anything else. Signed-off-by: Mike Hommey --- t/t7004-tag.sh | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-) diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh index 096fe33..3813f23 100755 --- a/t/t7004-tag.sh +++ b/t/t7004-tag.sh @@ -1007,7 +1007,9 @@ test_expect_failure \ test_expect_success \ 'message in editor has initial comment' ' GIT_EDITOR=cat git tag -a initial-comment > actual || true && - test $(sed -n "/^\(#\|\$\)/p" actual | wc -l) -gt 0 + grep -e "^$" actual > /dev/null 2>&1 && + grep -e "^#" actual > /dev/null 2>&1 && + ! grep -e "^[^#]" actual > /dev/null 2>&1 ' get_tag_header reuse $commit commit $time >expect -- 1.5.3.5 ``` ## Benoit Sigoure, 2007-11-16 17:58 Subject: Re: [PATCH] Fix t7004 which fails with retarded sed Message-ID: <6E925AF7-FCCB-4793-936F-7BC513272E75@lrde.epita.fr> URL: https://gitlist.dev/e/6E925AF7-FCCB-4793-936F-7BC513272E75%40lrde.epita.fr In-Reply-To: <1195233971-12288-1-git-send-email-mh@glandium.org> ``` On Nov 16, 2007, at 6:26 PM, Mike Hommey wrote: > Brown paper bag fix to avoid test failure with retarded sed. The test > by itself didn't catch what it was supposed to, anyways. > > So now, we test whether the editor gets at least an empty line, some > commented lines, and doesn't get anything else. > > Signed-off-by: Mike Hommey > --- > t/t7004-tag.sh | 4 +++- > 1 files changed, 3 insertions(+), 1 deletions(-) > > diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh > index 096fe33..3813f23 100755 > --- a/t/t7004-tag.sh > +++ b/t/t7004-tag.sh > @@ -1007,7 +1007,9 @@ test_expect_failure \ > test_expect_success \ > 'message in editor has initial comment' ' > GIT_EDITOR=cat git tag -a initial-comment > actual || true && > - test $(sed -n "/^\(#\|\$\)/p" actual | wc -l) -gt 0 > + grep -e "^$" actual > /dev/null 2>&1 && > + grep -e "^#" actual > /dev/null 2>&1 && > + ! grep -e "^[^#]" actual > /dev/null 2>&1 > ' > > get_tag_header reuse $commit commit $time >expect If your system has a retarded `sed', it will most likely also have a brain-dead `/bin/sh' which doesn't handle `! command'. So I suggest you rewrite the last line as: grep -ve "^[^#]" actual > /dev/null 2>&1 Cheers, -- Benoit Sigoure aka Tsuna EPITA Research and Development Laboratory ``` ## Wincent Colaiuta, 2007-11-16 18:02 Subject: Re: [PATCH] Fix t7004 which fails with retarded sed Message-ID: <83DEF29B-4E72-4913-A77E-8C00D69B48E4@wincent.com> URL: https://gitlist.dev/e/83DEF29B-4E72-4913-A77E-8C00D69B48E4%40wincent.com In-Reply-To: <1195233971-12288-1-git-send-email-mh@glandium.org> ``` El 16/11/2007, a las 18:26, Mike Hommey escribió: > diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh > index 096fe33..3813f23 100755 > --- a/t/t7004-tag.sh > +++ b/t/t7004-tag.sh > @@ -1007,7 +1007,9 @@ test_expect_failure \ > test_expect_success \ > 'message in editor has initial comment' ' > GIT_EDITOR=cat git tag -a initial-comment > actual || true && > - test $(sed -n "/^\(#\|\$\)/p" actual | wc -l) -gt 0 > + grep -e "^$" actual > /dev/null 2>&1 && > + grep -e "^#" actual > /dev/null 2>&1 && > + ! grep -e "^[^#]" actual > /dev/null 2>&1 > ' Yep, that works here, and is a much more rigourous test than what we had. Cheers, Wincent ``` ## Junio C Hamano, 2007-11-16 18:26 Subject: Re: [PATCH] Fix t7004 which fails with retarded sed Message-ID: <7vpryaxczx.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vpryaxczx.fsf%40gitster.siamese.dyndns.org In-Reply-To: <1195233971-12288-1-git-send-email-mh@glandium.org> ``` Mike Hommey writes: > Brown paper bag fix to avoid test failure with retarded sed. The test > by itself didn't catch what it was supposed to, anyways. I'd rather not to say "retarded sed". It is not fair to blame the implementation when the script we feed to it is not portable. > test_expect_success \ > 'message in editor has initial comment' ' > GIT_EDITOR=cat git tag -a initial-comment > actual || true && > - test $(sed -n "/^\(#\|\$\)/p" actual | wc -l) -gt 0 > + grep -e "^$" actual > /dev/null 2>&1 && > + grep -e "^#" actual > /dev/null 2>&1 && > + ! grep -e "^[^#]" actual > /dev/null 2>&1 The test simulates a case where $ git tag -a initial-comment is run and the user exits the editor without editing the file to add any text to annotate the tag. The behaviour we want from such an invocation is (1) the user sees a sensible template in the editor, and (2) the command to error out, saying "Hey, you did not leave any message for us to use". So the earlier part of the test GIT_EDITOR=cat git tag -a initial-comment > actual || true && is already bogus with respect to the latter point. It will happily continue if "git tag" erroneously returns success, and the above does not catch it. if GIT_EDITOR=cat git tag -a initial-comment >actual then echo >&2 oops we should have errored out false else : happy -- anything else we want to check? fi The "actual" file contains what the user saw in the editor and returned to the "git tag" command. The template we give to the user begins with a blank line, followed by a brief instruction "write a tag message" as comments to be stripped. The test is trying to make sure that is what was given to the user. As you already discussed in the thread, the exact wording may change in the future and we would not want to adjust the test every time. I think the important points about this template are: * It begins with a single blank line, where the invoked editor would typically place the editing curser at so that the user can immediately start typing; * It has some instruction but that comes after that initial blank line, all lines prefixed with "#". I do not think we would want to check the wording of this instruction. * And it has nothing else, as the expected behaviour is "Hey you did not leave any message". ``` ## Mike Hommey, 2007-11-16 19:15 Subject: Re: [PATCH] Fix t7004 which fails with retarded sed Message-ID: <20071116191533.GA7624@glandium.org> URL: https://gitlist.dev/e/20071116191533.GA7624%40glandium.org In-Reply-To: <6E925AF7-FCCB-4793-936F-7BC513272E75@lrde.epita.fr> ``` On Fri, Nov 16, 2007 at 06:58:21PM +0100, Benoit Sigoure wrote: > On Nov 16, 2007, at 6:26 PM, Mike Hommey wrote: > >> Brown paper bag fix to avoid test failure with retarded sed. The test >> by itself didn't catch what it was supposed to, anyways. >> >> So now, we test whether the editor gets at least an empty line, some >> commented lines, and doesn't get anything else. >> >> Signed-off-by: Mike Hommey >> --- >> t/t7004-tag.sh | 4 +++- >> 1 files changed, 3 insertions(+), 1 deletions(-) >> >> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh >> index 096fe33..3813f23 100755 >> --- a/t/t7004-tag.sh >> +++ b/t/t7004-tag.sh >> @@ -1007,7 +1007,9 @@ test_expect_failure \ >> test_expect_success \ >> 'message in editor has initial comment' ' >> GIT_EDITOR=cat git tag -a initial-comment > actual || true && >> - test $(sed -n "/^\(#\|\$\)/p" actual | wc -l) -gt 0 >> + grep -e "^$" actual > /dev/null 2>&1 && >> + grep -e "^#" actual > /dev/null 2>&1 && >> + ! grep -e "^[^#]" actual > /dev/null 2>&1 >> ' >> >> get_tag_header reuse $commit commit $time >expect > > If your system has a retarded `sed', it will most likely also have a > brain-dead `/bin/sh' which doesn't handle `! command'. So I suggest you > rewrite the last line as: > grep -ve "^[^#]" actual > /dev/null 2>&1 A whole lot of the test suite uses `! command', which is why i chose to use it. Mike ``` ## Mike Hommey, 2007-11-16 19:17 Subject: Re: [PATCH] Fix t7004 which fails with retarded sed Message-ID: <20071116191715.GB7624@glandium.org> URL: https://gitlist.dev/e/20071116191715.GB7624%40glandium.org In-Reply-To: <7vpryaxczx.fsf@gitster.siamese.dyndns.org> ``` On Fri, Nov 16, 2007 at 10:26:58AM -0800, Junio C Hamano wrote: > Mike Hommey writes: > > > Brown paper bag fix to avoid test failure with retarded sed. The test > > by itself didn't catch what it was supposed to, anyways. > > I'd rather not to say "retarded sed". It is not fair to blame > the implementation when the script we feed to it is not > portable. > > > test_expect_success \ > > 'message in editor has initial comment' ' > > GIT_EDITOR=cat git tag -a initial-comment > actual || true && > > - test $(sed -n "/^\(#\|\$\)/p" actual | wc -l) -gt 0 > > + grep -e "^$" actual > /dev/null 2>&1 && > > + grep -e "^#" actual > /dev/null 2>&1 && > > + ! grep -e "^[^#]" actual > /dev/null 2>&1 > > The test simulates a case where > > $ git tag -a initial-comment > > is run and the user exits the editor without editing the file to > add any text to annotate the tag. The behaviour we want from > such an invocation is (1) the user sees a sensible template in > the editor, and (2) the command to error out, saying "Hey, you > did not leave any message for us to use". > > So the earlier part of the test > > GIT_EDITOR=cat git tag -a initial-comment > actual || true && > > is already bogus with respect to the latter point. It will > happily continue if "git tag" erroneously returns success, and > the above does not catch it. > > if GIT_EDITOR=cat git tag -a initial-comment >actual > then > echo >&2 oops we should have errored out > false > else > : happy -- anything else we want to check? > fi This should probably be tested in another test block. > The "actual" file contains what the user saw in the editor and > returned to the "git tag" command. > > The template we give to the user begins with a blank line, > followed by a brief instruction "write a tag message" as > comments to be stripped. The test is trying to make sure that > is what was given to the user. As you already discussed in the > thread, the exact wording may change in the future and we would > not want to adjust the test every time. > > I think the important points about this template are: > > * It begins with a single blank line, where the invoked editor > would typically place the editing curser at so that the user > can immediately start typing; > > * It has some instruction but that comes after that initial > blank line, all lines prefixed with "#". I do not think we > would want to check the wording of this instruction. > > * And it has nothing else, as the expected behaviour is "Hey > you did not leave any message". Which is roughly what my patch does, except it doesn't check for ordering. Mike ``` ## Junio C Hamano, 2007-11-16 19:36 Subject: Re: [PATCH] Fix t7004 which fails with retarded sed Message-ID: <7vlk8yx9rp.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vlk8yx9rp.fsf%40gitster.siamese.dyndns.org In-Reply-To: <20071116191715.GB7624@glandium.org> ``` Mike Hommey writes: >> So the earlier part of the test >> >> GIT_EDITOR=cat git tag -a initial-comment > actual || true && >> >> is already bogus with respect to the latter point. It will >> happily continue if "git tag" erroneously returns success, and >> the above does not catch it. >> >> if GIT_EDITOR=cat git tag -a initial-comment >actual >> then >> echo >&2 oops we should have errored out >> false >> else >> : happy -- anything else we want to check? >> fi > > This should probably be tested in another test block. Fair enough; please make it so. > Which is roughly what my patch does, except it doesn't check for > ordering. ... which I think is more important part. If the blank like were at the end, that would be irritating for the user, wouldn't it? ``` ## Mike Hommey, 2007-11-16 20:28 Subject: [PATCH] Fix and improve t7004 Message-ID: <1195244917-25659-1-git-send-email-mh@glandium.org> URL: https://gitlist.dev/e/1195244917-25659-1-git-send-email-mh%40glandium.org In-Reply-To: <7vlk8yx9rp.fsf@gitster.siamese.dyndns.org> ``` Brown paper bag fix to avoid using non portable sed syntax. The test by itself didn't catch what it was supposed to, anyways. The new test first checks whether the user exited the editor without editing the file, then whether what the user was presented in the editor was any useful to her, which we define as the following: * It begins with a single blank line, where the invoked editor would typically place the editing curser at so that the user can immediately start typing; * It has some instruction but that comes after that initial blank line, all lines prefixed with "#". * And it has nothing else, as the expected behaviour is "Hey you did not leave any message". Signed-off-by: Mike Hommey --- t/t7004-tag.sh | 9 ++++++++- 1 files changed, 8 insertions(+), 1 deletions(-) diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh index 096fe33..42b1f97 100755 --- a/t/t7004-tag.sh +++ b/t/t7004-tag.sh @@ -1004,10 +1004,17 @@ test_expect_failure \ 'verify signed tag fails when public key is not present' \ 'git-tag -v signed-tag' +test_expect_failure \ + 'git-tag -a fails if tag annotation is empty' ' + GIT_EDITOR=cat git tag -a initial-comment > /dev/null 2>&1 +' + test_expect_success \ 'message in editor has initial comment' ' GIT_EDITOR=cat git tag -a initial-comment > actual || true && - test $(sed -n "/^\(#\|\$\)/p" actual | wc -l) -gt 0 + ( read empty ; + [ "$empty" ] && exit 1 ; + ! grep -ve "^#" > /dev/null 2>&1 ) < actual ' get_tag_header reuse $commit commit $time >expect -- 1.5.3.5 ``` ## Benoit Sigoure, 2007-11-16 21:04 Subject: Re: [PATCH] Fix and improve t7004 Message-ID: URL: https://gitlist.dev/e/EEE710FA-0408-489F-8128-B4C1F06D34FF%40lrde.epita.fr In-Reply-To: <1195244917-25659-1-git-send-email-mh@glandium.org> ``` On Nov 16, 2007, at 9:28 PM, Mike Hommey wrote: > Brown paper bag fix to avoid using non portable sed syntax. The > test by itself didn't catch what it was supposed to, anyways. > > The new test first checks whether the user exited the editor > without editing the file, then whether what the user was > presented in the editor was any useful to her, which we define > as the following: > * It begins with a single blank line, where the invoked editor > would typically place the editing curser at so that the user > can immediately start typing; > > * It has some instruction but that comes after that initial > blank line, all lines prefixed with "#". > > * And it has nothing else, as the expected behaviour is "Hey > you did not leave any message". > > Signed-off-by: Mike Hommey > --- > t/t7004-tag.sh | 9 ++++++++- > 1 files changed, 8 insertions(+), 1 deletions(-) > > diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh > index 096fe33..42b1f97 100755 > --- a/t/t7004-tag.sh > +++ b/t/t7004-tag.sh > @@ -1004,10 +1004,17 @@ test_expect_failure \ > 'verify signed tag fails when public key is not present' \ > 'git-tag -v signed-tag' > > +test_expect_failure \ > + 'git-tag -a fails if tag annotation is empty' ' > + GIT_EDITOR=cat git tag -a initial-comment > /dev/null 2>&1 > +' > + > test_expect_success \ > 'message in editor has initial comment' ' > GIT_EDITOR=cat git tag -a initial-comment > actual || true && > - test $(sed -n "/^\(#\|\$\)/p" actual | wc -l) -gt 0 > + ( read empty ; > + [ "$empty" ] && exit 1 ; What is this meant to do? Did you mean [ -n "$empty" ] ? > + ! grep -ve "^#" > /dev/null 2>&1 ) < actual The double negation is harder to read. May I suggest something along these lines (which seems more readable to me): while read line; do case $line in #( '#'*) ;; # Accept comments ( *) exit 1;; esac done Advantages: The purpose of the test is more obvious The test is more easily extendable The test saves a fork ;o > ' > > get_tag_header reuse $commit commit $time >expect Cheers, -- Benoit Sigoure aka Tsuna EPITA Research and Development Laboratory ``` ## Mike Hommey, 2007-11-16 21:11 Subject: Re: [PATCH] Fix and improve t7004 Message-ID: <20071116211148.GA28966@glandium.org> URL: https://gitlist.dev/e/20071116211148.GA28966%40glandium.org In-Reply-To: ``` On Fri, Nov 16, 2007 at 10:04:57PM +0100, Benoit Sigoure wrote: > On Nov 16, 2007, at 9:28 PM, Mike Hommey wrote: > >> Brown paper bag fix to avoid using non portable sed syntax. The >> test by itself didn't catch what it was supposed to, anyways. >> >> The new test first checks whether the user exited the editor >> without editing the file, then whether what the user was >> presented in the editor was any useful to her, which we define >> as the following: >> * It begins with a single blank line, where the invoked editor >> would typically place the editing curser at so that the user >> can immediately start typing; >> >> * It has some instruction but that comes after that initial >> blank line, all lines prefixed with "#". >> >> * And it has nothing else, as the expected behaviour is "Hey >> you did not leave any message". >> >> Signed-off-by: Mike Hommey >> --- >> t/t7004-tag.sh | 9 ++++++++- >> 1 files changed, 8 insertions(+), 1 deletions(-) >> >> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh >> index 096fe33..42b1f97 100755 >> --- a/t/t7004-tag.sh >> +++ b/t/t7004-tag.sh >> @@ -1004,10 +1004,17 @@ test_expect_failure \ >> 'verify signed tag fails when public key is not present' \ >> 'git-tag -v signed-tag' >> >> +test_expect_failure \ >> + 'git-tag -a fails if tag annotation is empty' ' >> + GIT_EDITOR=cat git tag -a initial-comment > /dev/null 2>&1 >> +' >> + >> test_expect_success \ >> 'message in editor has initial comment' ' >> GIT_EDITOR=cat git tag -a initial-comment > actual || true && >> - test $(sed -n "/^\(#\|\$\)/p" actual | wc -l) -gt 0 >> + ( read empty ; >> + [ "$empty" ] && exit 1 ; > > What is this meant to do? Did you mean [ -n "$empty" ] ? Replacing with [ -n "$empty" ] would not work properly, except if you replace the following ; with &&. Does that really make a readability difference ? >> + ! grep -ve "^#" > /dev/null 2>&1 ) < actual > > The double negation is harder to read. May I suggest something along these > lines (which seems more readable to me): > while read line; do > case $line in #( > '#'*) ;; # Accept comments ( > *) exit 1;; > esac > done I'm not really convinced. What do other people have to say ? Mike ``` ## Benoit Sigoure, 2007-11-16 21:31 Subject: Re: [PATCH] Fix and improve t7004 Message-ID: URL: https://gitlist.dev/e/EC3B30DC-E81C-4ECA-BE7F-F237E1338603%40lrde.epita.fr In-Reply-To: <20071116211148.GA28966@glandium.org> ``` On Nov 16, 2007, at 10:11 PM, Mike Hommey wrote: > On Fri, Nov 16, 2007 at 10:04:57PM +0100, Benoit Sigoure wrote: >> On Nov 16, 2007, at 9:28 PM, Mike Hommey wrote: >>> test_expect_success \ >>> 'message in editor has initial comment' ' >>> GIT_EDITOR=cat git tag -a initial-comment > actual || true && >>> - test $(sed -n "/^\(#\|\$\)/p" actual | wc -l) -gt 0 >>> + ( read empty ; >>> + [ "$empty" ] && exit 1 ; >> >> What is this meant to do? Did you mean [ -n "$empty" ] ? > > Replacing with [ -n "$empty" ] would not work properly, except if you > replace the following ; with &&. Does that really make a readability > difference ? I don't get it. As far as I understand, you're trying to check whether $empty is indeed empty, right? So how is `[ "$empty" ]' meant to work? [ -n "$empty" ] && exit 1 will exit 1 if empty isn't empty. -- Benoit Sigoure aka Tsuna EPITA Research and Development Laboratory ``` ## Mike Hommey, 2007-11-16 21:35 Subject: Re: [PATCH] Fix and improve t7004 Message-ID: <20071116213538.GA30076@glandium.org> URL: https://gitlist.dev/e/20071116213538.GA30076%40glandium.org In-Reply-To: ``` On Fri, Nov 16, 2007 at 10:31:15PM +0100, Benoit Sigoure wrote: > On Nov 16, 2007, at 10:11 PM, Mike Hommey wrote: > >> On Fri, Nov 16, 2007 at 10:04:57PM +0100, Benoit Sigoure wrote: >>> On Nov 16, 2007, at 9:28 PM, Mike Hommey wrote: >>>> test_expect_success \ >>>> 'message in editor has initial comment' ' >>>> GIT_EDITOR=cat git tag -a initial-comment > actual || true && >>>> - test $(sed -n "/^\(#\|\$\)/p" actual | wc -l) -gt 0 >>>> + ( read empty ; >>>> + [ "$empty" ] && exit 1 ; >>> >>> What is this meant to do? Did you mean [ -n "$empty" ] ? >> >> Replacing with [ -n "$empty" ] would not work properly, except if you >> replace the following ; with &&. Does that really make a readability >> difference ? > > I don't get it. As far as I understand, you're trying to check whether > $empty is indeed empty, right? So how is `[ "$empty" ]' meant to work? > [ -n "$empty" ] && exit 1 > > will exit 1 if empty isn't empty. Sorry, I read '-z', not '-n'. [ "$empty" ] and [ -n "$empty" ] are the same thing. Mike ``` ## Junio C Hamano, 2007-11-16 21:42 Subject: Re: [PATCH] Fix and improve t7004 Message-ID: <7vbq9tyii3.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vbq9tyii3.fsf%40gitster.siamese.dyndns.org In-Reply-To: <20071116211148.GA28966@glandium.org> ``` Mike Hommey writes: >>> + ( read empty ; >>> + [ "$empty" ] && exit 1 ; >> >> What is this meant to do? Did you mean [ -n "$empty" ] ? > > Replacing with [ -n "$empty" ] would not work properly, except if you > replace the following ; with &&. Does that really make a readability > difference ? > >>> + ! grep -ve "^#" > /dev/null 2>&1 ) < actual >> >> The double negation is harder to read. May I suggest something along these >> lines (which seems more readable to me): >> while read line; do >> case $line in #( >> '#'*) ;; # Accept comments ( >> *) exit 1;; >> esac >> done > > I'm not really convinced. What do other people have to say ? As shell "read" loses information (a backslash sequence is interpreted, and trailing whitespaces are stripped and not assigned to "line" above), it is not such a good vehicle if you want to make a reasonably strict test on top of. Some shells do not implement "read -r" either, so it is also a portability hassle. Perhaps... # check the first line --- should be empty first=$(sed -e 1q URL: https://gitlist.dev/e/225E846F-7EDB-4B57-923C-909F3F000D82%40lrde.epita.fr In-Reply-To: <20071116213538.GA30076@glandium.org> ``` On Nov 16, 2007, at 10:35 PM, Mike Hommey wrote: > On Fri, Nov 16, 2007 at 10:31:15PM +0100, Benoit Sigoure wrote: >> On Nov 16, 2007, at 10:11 PM, Mike Hommey wrote: >> >>> On Fri, Nov 16, 2007 at 10:04:57PM +0100, Benoit Sigoure wrote: >>>> On Nov 16, 2007, at 9:28 PM, Mike Hommey wrote: >>>>> test_expect_success \ >>>>> 'message in editor has initial comment' ' >>>>> GIT_EDITOR=cat git tag -a initial-comment > actual || true && >>>>> - test $(sed -n "/^\(#\|\$\)/p" actual | wc -l) -gt 0 >>>>> + ( read empty ; >>>>> + [ "$empty" ] && exit 1 ; >>>> >>>> What is this meant to do? Did you mean [ -n "$empty" ] ? >>> >>> Replacing with [ -n "$empty" ] would not work properly, except if >>> you >>> replace the following ; with &&. Does that really make a readability >>> difference ? >> >> I don't get it. As far as I understand, you're trying to check >> whether >> $empty is indeed empty, right? So how is `[ "$empty" ]' meant to >> work? >> [ -n "$empty" ] && exit 1 >> >> will exit 1 if empty isn't empty. > > Sorry, I read '-z', not '-n'. [ "$empty" ] and [ -n "$empty" ] are the > same thing. Heh, forgive my ignorance, I did not know the [ "string" ] notation. Amazing, after all these years of shell scripting... -- Benoit Sigoure aka Tsuna EPITA Research and Development Laboratory ``` ## Mike Hommey, 2007-11-16 22:02 Subject: [PATCH] Fix and improve t7004 Message-ID: <1195250528-9727-1-git-send-email-mh@glandium.org> URL: https://gitlist.dev/e/1195250528-9727-1-git-send-email-mh%40glandium.org In-Reply-To: <7vbq9tyii3.fsf@gitster.siamese.dyndns.org> ``` Brown paper bag fix to avoid using non portable sed syntax. The test by itself didn't catch what it was supposed to, anyways. The new test first checks whether the user exited the editor without editing the file, then whether what the user was presented in the editor was any useful to her, which we define as the following: * It begins with a single blank line, where the invoked editor would typically place the editing curser at so that the user can immediately start typing; * It has some instruction but that comes after that initial blank line, all lines prefixed with "#". * And it has nothing else, as the expected behaviour is "Hey you did not leave any message". Signed-off-by: Mike Hommey --- > Perhaps... > > # check the first line --- should be empty > first=$(sed -e 1q test -z "$first" && > # remove commented lines from the remainder -- should be empty > rest=$(sed -e 1d -e '/^#/d' test -z "$rest" I like that better. So here is a new version using this. In the end, the patch and most of the comment are yours, so maybe you should claim it is your patch. t/t7004-tag.sh | 12 +++++++++++- 1 files changed, 11 insertions(+), 1 deletions(-) diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh index 096fe33..162bdf7 100755 --- a/t/t7004-tag.sh +++ b/t/t7004-tag.sh @@ -1004,10 +1004,20 @@ test_expect_failure \ 'verify signed tag fails when public key is not present' \ 'git-tag -v signed-tag' +test_expect_failure \ + 'git-tag -a fails if tag annotation is empty' ' + GIT_EDITOR=cat git tag -a initial-comment > /dev/null 2>&1 +' + test_expect_success \ 'message in editor has initial comment' ' GIT_EDITOR=cat git tag -a initial-comment > actual || true && - test $(sed -n "/^\(#\|\$\)/p" actual | wc -l) -gt 0 + # check the first line --- should be empty + first=$(sed -e 1q expect -- 1.5.3.5 ``` ## Junio C Hamano, 2007-11-17 08:55 Subject: Re: [PATCH] Fix and improve t7004 Message-ID: <7vfxz5uu86.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vfxz5uu86.fsf%40gitster.siamese.dyndns.org In-Reply-To: <1195250528-9727-1-git-send-email-mh@glandium.org> ``` Thanks. ```