[BUG] t7004 (master) busted on Leopard

[BUG] t7004 (master) busted on Leopard

[Wincent Colaiuta]

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

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

[Johannes Schindelin]

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

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

[David Kastrup]

Johannes Schindelin <Johannes.Schindelin@gmx.de> 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

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

[Wincent Colaiuta]

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

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

[Wincent Colaiuta]

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

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

[Wincent Colaiuta]

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

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

[Wincent Colaiuta]

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

Cheers,
Wincent

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

[Wincent Colaiuta]

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

Cheers,
Wincent

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

[Mike Hommey]

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.

grep -e "^$" actual && grep -e "^#" actual would actually be a better
test.

Mike

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

[Wincent Colaiuta]

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 <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

[PATCH] Fix t7004 which fails with retarded sed

[Mike Hommey]

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

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

[Benoit Sigoure]

[-- Attachment #1: Type: text/plain, Size: 1300 bytes --]

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

Cheers,

-- 
Benoit Sigoure aka Tsuna
EPITA Research and Development Laboratory



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 186 bytes --]

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

[Mike Hommey]

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 <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

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

[Wincent Colaiuta]

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

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

[Junio C Hamano]

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

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".

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

[Mike Hommey]

On Fri, Nov 16, 2007 at 10:26:58AM -0800, Junio C Hamano wrote:
> 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.

> 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

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

[Junio C Hamano]

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

[PATCH] Fix and improve t7004

[Mike Hommey]

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

Re: [PATCH] Fix and improve t7004

[Benoit Sigoure]

[-- Attachment #1: Type: text/plain, Size: 2185 bytes --]

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" ] ?

> +	  ! 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



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 186 bytes --]

Re: [PATCH] Fix and improve t7004

[Mike Hommey]

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 <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 ?

>> +	  ! 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

Re: [PATCH] Fix and improve t7004

[Benoit Sigoure]

[-- Attachment #1: Type: text/plain, Size: 933 bytes --]

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



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 186 bytes --]

Re: [PATCH] Fix and improve t7004

[Mike Hommey]

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

Re: [PATCH] Fix and improve t7004

[Benoit Sigoure]

[-- Attachment #1: Type: text/plain, Size: 1323 bytes --]

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



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 186 bytes --]

Re: [PATCH] Fix and improve t7004

[Junio C Hamano]

Mike Hommey <mh@glandium.org> 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 <actual) &&
        test -z "$first" &&
	# remove commented lines from the remainder -- should be empty
        rest=$(sed -e 1d -e '/^#/d' <actual) &&
        test -z "$rest"

[PATCH] Fix and improve t7004

[Mike Hommey]

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>
---
> 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

Re: [PATCH] Fix and improve t7004

[Junio C Hamano]

Thanks.