[PATCH 0/2] git notes show: handle empty notes gracefully

[PATCH 0/2] git notes show: handle empty notes gracefully

[Michael J Gruber]

"git notes show" barfs when there is no note to show. This introduces a
test (yes, a test!) and, in a second round, reacts more gracefully to
empty notes and adjust the expected test output accordingly.

Note that in both cases (before/after the patch) the return code is
non-zero: It's 128 in the ungraceful case, 1 when "dieing gracefully",
uhm...

Michael J Gruber (2):
  git notes show: test empty notes
  handle empty notes gracefully

 git-notes.sh     |    2 ++
 t/t3301-notes.sh |    5 +++++
 2 files changed, 7 insertions(+), 0 deletions(-)

[PATCH 1/2] git notes show: test empty notes

[Michael J Gruber]

Add a test for the handling of empty notes by "git notes show".
---
 t/t3301-notes.sh |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index 9393a25..4900dca 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -35,6 +35,11 @@ test_expect_success 'need valid notes ref' '
 	! MSG=2 GIT_NOTES_REF='/' git notes show
 '
 
+# 1 indicates caught gracefully by die, 128 means git-show barfed
+test_expect_failure 'handle empty notes gracefully' '
+	git notes show || test 1 = $?
+'
+
 test_expect_success 'create notes' '
 	git config core.notesRef refs/notes/commits &&
 	MSG=b1 git notes edit &&
-- 
1.6.1.2.253.ga34a

[PATCH 2/2] handle empty notes gracefully

[Michael J Gruber]

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."
 	git show "$GIT_NOTES_REF":$COMMIT
 ;;
 *)
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 = $?
 '
 
-- 
1.6.1.2.253.ga34a

Re: [PATCH 1/2] git notes show: test empty notes

[Johannes Schindelin]

Hi,

On Fri, 6 Feb 2009, Michael J Gruber wrote:

> Add a test for the handling of empty notes by "git notes show".
> ---
>  t/t3301-notes.sh |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
> index 9393a25..4900dca 100755
> --- a/t/t3301-notes.sh
> +++ b/t/t3301-notes.sh
> @@ -35,6 +35,11 @@ test_expect_success 'need valid notes ref' '
>  	! MSG=2 GIT_NOTES_REF='/' git notes show
>  '
>  
> +# 1 indicates caught gracefully by die, 128 means git-show barfed
> +test_expect_failure 'handle empty notes gracefully' '
> +	git notes show || test 1 = $?
> +'

That test would succeed if the exit status is 0.

Ciao,
Dscho

Re: [PATCH 0/2] git notes show: handle empty notes gracefully

[Michael J Gruber]

Michael J Gruber venit, vidit, dixit 06.02.2009 16:19:
> "git notes show" barfs when there is no note to show. This introduces a
> test (yes, a test!) and, in a second round, reacts more gracefully to
> empty notes and adjust the expected test output accordingly.
> 
> Note that in both cases (before/after the patch) the return code is
> non-zero: It's 128 in the ungraceful case, 1 when "dieing gracefully",
> uhm...
> 
> Michael J Gruber (2):
>   git notes show: test empty notes
>   handle empty notes gracefully
> 
>  git-notes.sh     |    2 ++
>  t/t3301-notes.sh |    5 +++++
>  2 files changed, 7 insertions(+), 0 deletions(-)

Uhm, I'm about 1 hour late with my afternoon coffee, and I'm afraid that
shows:

1) I meant "bark", not "barf", although both make sense here.

2) When did "format.signoff = true" in config stop working? OK, it never
did. So please consider this (anyway trivial series):

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>

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

[Johannes Schindelin]

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.  And maybe you want to be more explicit, by specifying which 
commit's notes are expected not to be there.

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

Ciao,
Dscho

Re: [PATCH 1/2] git notes show: test empty notes

[Michael J Gruber]

Johannes Schindelin venit, vidit, dixit 06.02.2009 16:36:
> Hi,
> 
> On Fri, 6 Feb 2009, Michael J Gruber wrote:
> 
>> Add a test for the handling of empty notes by "git notes show".
>> ---
>>  t/t3301-notes.sh |    5 +++++
>>  1 files changed, 5 insertions(+), 0 deletions(-)
>>
>> diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
>> index 9393a25..4900dca 100755
>> --- a/t/t3301-notes.sh
>> +++ b/t/t3301-notes.sh
>> @@ -35,6 +35,11 @@ test_expect_success 'need valid notes ref' '
>>  	! MSG=2 GIT_NOTES_REF='/' git notes show
>>  '
>>  
>> +# 1 indicates caught gracefully by die, 128 means git-show barfed
>> +test_expect_failure 'handle empty notes gracefully' '
>> +	git notes show || test 1 = $?
>> +'
> 
> That test would succeed if the exit status is 0.

Yes. If "git notes show" returns 0 then even better. It does neither
before nor after the patch. Or should we always expect 1 and test for
that? I thought about grepping the output for "fatal" (which appears
now) but that seemed ugly.

Michael

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

[Johannes Sixt]

Johannes Schindelin schrieb:
> On Fri, 6 Feb 2009, Michael J Gruber wrote:
>> +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.

Wouldn't work. We want to differentiate between different exit codes. But
after test_must_fail the exit code is gone. In this simple case this
should work:

	git notes show; test 1 = $?

-- Hannes

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

[Michael J Gruber]

Johannes Schindelin venit, vidit, dixit 06.02.2009 16:38:
> 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