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

"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(-)

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

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

Hi,

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

That test would succeed if the exit status is 0.

Ciao,
Dscho

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

Hi,

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

This looks good.

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

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

Hi,

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

quoted

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

Johannes Schindelin schrieb:

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

quoted

+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

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

Hi,

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

quoted

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.

quoted

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