threads / bug / 10885

[BUG] t7004 (master) busted on Leopard

Subject: [BUG] t7004 (master) busted on Leopard

## tl;dr

26 messages between Nov 15, 2007 and Nov 17, 2007.

replies: 25people: 6as markdown or json

Wincent Colaiuta· Nov 15, 2007, 13:38 UTC · lore

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· Nov 15, 2007, 14:37 UTC · re: Wincent Colaiuta · lore

Re: [BUG] t7004 (master) busted on Leopard

Hi,
On Thu, 15 Nov 2007, Wincent Colaiuta wrote:
Show 6 quoted lines
> 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· Nov 15, 2007, 14:48 UTC · re: Johannes Schindelin · lore

Re: [BUG] t7004 (master) busted on Leopard

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 14 quoted lines
> 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· Nov 15, 2007, 15:12 UTC · re: Johannes Schindelin · lore

Re: [BUG] t7004 (master) busted on Leopard

El 15/11/2007, a las 15:37, Johannes Schindelin escribió:
Show 13 quoted lines
> 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.

Show 5 quoted lines
> 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
Show 5 quoted lines
> 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· Nov 15, 2007, 15:16 UTC · re: Johannes Schindelin · lore

[PATCH] Fix git-tag test breakage caused by broken sed on Leopard

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 <win@wincent.com>
---
  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· Nov 15, 2007, 15:47 UTC · re: Wincent Colaiuta · lore

[PATCH v2] Fix git-tag test breakage caused by broken sed on Leopard

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 <win@wincent.com>
---

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· Nov 16, 2007, 13:45 UTC · lore

Re: [PATCH v2] Fix git-tag test breakage caused by broken sed on Leopard

El 16/11/2007, a las 6:14, Junio C Hamano escribió:
Show 16 quoted lines
> Wincent Colaiuta <win@wincent.com> 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· Nov 16, 2007, 13:48 UTC · re: Wincent Colaiuta · lore

Re: [PATCH v2] Fix git-tag test breakage caused by broken sed on Leopard

El 16/11/2007, a las 14:45, Wincent Colaiuta escribió:
Show 26 quoted lines
> El 16/11/2007, a las 6:14, Junio C Hamano escribió:
>
>> Wincent Colaiuta <win@wincent.com> 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· Nov 16, 2007, 16:59 UTC · re: Wincent Colaiuta · lore

Re: [PATCH v2] Fix git-tag test breakage caused by broken sed on Leopard

On Fri, Nov 16, 2007 at 02:48:09PM +0100, Wincent Colaiuta wrote:
Show 35 quoted lines
> 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 <win@wincent.com> 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· Nov 16, 2007, 17:25 UTC · re: Mike Hommey · lore

Re: [PATCH v2] Fix git-tag test breakage caused by broken sed on Leopard

El 16/11/2007, a las 17:59, Mike Hommey escribió:
Show 41 quoted lines
> 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 <win@wincent.com> 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· Nov 16, 2007, 17:26 UTC · re: Mike Hommey · lore

[PATCH] Fix t7004 which fails with retarded sed

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 <mh@glandium.org>
---
 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· Nov 16, 2007, 17:58 UTC · re: Mike Hommey · lore

Re: [PATCH] Fix t7004 which fails with retarded sed

On Nov 16, 2007, at 6:26 PM, Mike Hommey wrote:
Show 26 quoted lines
> 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 <mh@glandium.org>
> ---
>  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
Mike Hommey· Nov 16, 2007, 19:15 UTC · re: Benoit Sigoure · lore

Re: [PATCH] Fix t7004 which fails with retarded sed

On Fri, Nov 16, 2007 at 06:58:21PM +0100, Benoit Sigoure wrote:
Show 33 quoted lines
> 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 <mh@glandium.org>
>> ---
>>  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
Wincent Colaiuta· Nov 16, 2007, 18:02 UTC · re: Mike Hommey · lore

Re: [PATCH] Fix t7004 which fails with retarded sed

El 16/11/2007, a las 18:26, Mike Hommey escribió:
Show 13 quoted lines
> 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· Nov 16, 2007, 18:26 UTC · re: Mike Hommey · lore

Re: [PATCH] Fix t7004 which fails with retarded sed

Mike Hommey <mh@glandium.org> 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.

Show 7 quoted lines
>  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· Nov 16, 2007, 19:17 UTC · re: Junio C Hamano · lore

Re: [PATCH] Fix t7004 which fails with retarded sed

On Fri, Nov 16, 2007 at 10:26:58AM -0800, Junio C Hamano wrote:
Show 42 quoted lines
> Mike Hommey <mh@glandium.org> 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.
Show 22 quoted lines
> 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· Nov 16, 2007, 19:36 UTC · re: Mike Hommey · lore

Re: [PATCH] Fix t7004 which fails with retarded sed

Mike Hommey <mh@glandium.org> writes:
Show 17 quoted lines
>> 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· Nov 16, 2007, 20:28 UTC · re: Junio C Hamano · lore

[PATCH] Fix and improve t7004

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 <mh@glandium.org>
---
 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· Nov 16, 2007, 21:04 UTC · re: Mike Hommey · lore

Re: [PATCH] Fix and improve t7004

On Nov 16, 2007, at 9:28 PM, Mike Hommey wrote:
Show 41 quoted lines
> 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 <mh@glandium.org>
> ---
>  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· Nov 16, 2007, 21:11 UTC · re: Benoit Sigoure · lore

Re: [PATCH] Fix and improve t7004

On Fri, Nov 16, 2007 at 10:04:57PM +0100, Benoit Sigoure wrote:
Show 45 quoted lines
> 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 <mh@glandium.org>
>> ---
>>  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 ?

Show 10 quoted lines
>> +	  ! 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· Nov 16, 2007, 21:31 UTC · re: Mike Hommey · lore

Re: [PATCH] Fix and improve t7004

On Nov 16, 2007, at 10:11 PM, Mike Hommey wrote:
Show 14 quoted lines
> 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· Nov 16, 2007, 21:35 UTC · re: Benoit Sigoure · lore

Re: [PATCH] Fix and improve t7004

On Fri, Nov 16, 2007 at 10:31:15PM +0100, Benoit Sigoure wrote:
Show 22 quoted lines
> 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
Benoit Sigoure· Nov 16, 2007, 21:47 UTC · re: Mike Hommey · lore

Re: [PATCH] Fix and improve t7004

On Nov 16, 2007, at 10:35 PM, Mike Hommey wrote:
Show 29 quoted lines
> 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
Junio C Hamano· Nov 16, 2007, 21:42 UTC · re: Mike Hommey · lore

Re: [PATCH] Fix and improve t7004

Mike Hommey <mh@glandium.org> writes:
Show 21 quoted lines
>>> +	( 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 <actual) &&
        test -z "$first" &&
	# remove commented lines from the remainder -- should be empty
        rest=$(sed -e 1d -e '/^#/d' <actual) &&
        test -z "$rest"
Mike Hommey· Nov 16, 2007, 22:02 UTC · re: Junio C Hamano · lore

[PATCH] Fix and improve t7004

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 <mh@glandium.org>
---
Show 8 quoted lines
> Perhaps...
> 
>       # check the first line --- should be empty
>       first=$(sed -e 1q <actual) &&
>         test -z "$first" &&
>       # remove commented lines from the remainder -- should be empty
>         rest=$(sed -e 1d -e '/^#/d' <actual) &&
>         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 <actual) &&
+	test -z "$first" &&
+	# remove commented lines from the remainder -- should be empty
+	rest=$(sed -e 1d -e '/^#/d' <actual) &&
+	test -z "$rest"
 '
 
 get_tag_header reuse $commit commit $time >expect

-- 
1.5.3.5

← back to recent threads