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

From: Wincent Colaiuta <win@wincent.com>
Date: Nov 16, 2007, 17:25 UTC
Message-ID: <E4B5D2FD-5757-4178-B77C-1D61E7B3F589@wincent.com>
In-Reply-To: <20071116165944.GB29272@glandium.org>

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