Re: [PATCH 2/2] handle empty notes gracefully

From: Michael J Gruber <git@drmicha.warpmail.net>
Date: Feb 6, 2009, 15:50 UTC
Message-ID: <498C5C39.7020503@drmicha.warpmail.net>
In-Reply-To: <alpine.DEB.1.00.0902061636320.7377@intel-tinevez-2-302>

Johannes Schindelin venit, vidit, dixit 06.02.2009 16:38:

Show 38 quoted lines

> Hi,
> 
> On Fri, 6 Feb 2009, Michael J Gruber wrote:
> 
>> Currently, git-notes barfs when asked to show an empty (i.e.
>> non-existing) note. Change this to explicitely say there is none.
>> ---
>>  git-notes.sh     |    2 ++
>>  t/t3301-notes.sh |    2 +-
>>  2 files changed, 3 insertions(+), 1 deletions(-)
>>
>> diff --git a/git-notes.sh b/git-notes.sh
>> index bfdbaa8..9cbad02 100755
>> --- a/git-notes.sh
>> +++ b/git-notes.sh
>> @@ -58,6 +58,8 @@ edit)
>>  		"$GIT_NOTES_REF" $NEW_HEAD $CURRENT_HEAD
>>  ;;
>>  show)
>> +	git rev-parse -q --verify "$GIT_NOTES_REF":$COMMIT > /dev/null ||
>> +		die "No note for commit $COMMIT."
> 
> This looks good.
> 
>> diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
>> index 4900dca..81d5028 100755
>> --- a/t/t3301-notes.sh
>> +++ b/t/t3301-notes.sh
>> @@ -36,7 +36,7 @@ test_expect_success 'need valid notes ref' '
>>  '
>>  
>>  # 1 indicates caught gracefully by die, 128 means git-show barfed
>> -test_expect_failure 'handle empty notes gracefully' '
>> +test_expect_success 'handle empty notes gracefully' '
>>  	git notes show || test 1 = $?
> 
> Completely forgot to mention that I think you want to use test_must_fail 
> here.

What does test_must_fail mean? I don't see it in t/README.

> And maybe you want to be more explicit, by specifying which 
> commit's notes are expected not to be there.

Well, HEAD's notes. I can say HEAD explicitly, of course.

> We would not want the test to succeed for all the wrong reasons, would we?

Well, I could test for the friendly "No note for commit" message on output, I just thought that is fragile.

You see, I did not even want to write a test for such a simple patch... ;)

OK, do we agree on the following intended behaviour for git notes show:
- return 0 if a note can be shown
- return 1 if there is none (i.e. die gracefully)
- return something else (i.e. die fatally) if something really bad happens

Then I should rewrite the test to check for "1" and only "1".

Now, the coffee...

Michael