Re: [PATCH] Fix and improve t7004

[Mike Hommey <mh@glandium.org>]

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