threads / patch / 17614

patch, 2 partshandle empty notes gracefully

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

## tl;dr

9 messages between Feb 6, 2009 and Feb 6, 2009. Diffs are folded; open one to read it.

replies: 8people: 3as markdown or json

Michael J Gruber· Feb 6, 2009, 15:19 UTC · lore

[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(-)
Michael J Gruber· Feb 6, 2009, 15:19 UTC · re: Michael J Gruber · lore

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

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(-)
Show changes to t/t3301-notes.sh +5 −0
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
Michael J Gruber· Feb 6, 2009, 15:19 UTC · re: Michael J Gruber · lore
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(-)
Show changes to 2 files +3 −1

git-notes.sh, t/t3301-notes.sh

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
Johannes Schindelin· Feb 6, 2009, 15:38 UTC · re: Michael J Gruber · lore

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

Hi,
On Fri, 6 Feb 2009, Michael J Gruber wrote:
Show 17 quoted lines
> 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.
Show 11 quoted lines
> 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

Johannes Sixt· Feb 6, 2009, 15:49 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin schrieb:
Show 6 quoted lines
> 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
Michael J Gruber· Feb 6, 2009, 15:50 UTC · re: Johannes Schindelin · lore

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

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
Johannes Schindelin· Feb 6, 2009, 15:36 UTC · re: Michael J Gruber · lore

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

Hi,
On Fri, 6 Feb 2009, Michael J Gruber wrote:
Show 17 quoted lines
> 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

Michael J Gruber· Feb 6, 2009, 15:42 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin venit, vidit, dixit 06.02.2009 16:36:
Show 23 quoted lines
> 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
Michael J Gruber· Feb 6, 2009, 15:37 UTC · re: Michael J Gruber · lore

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

Michael J Gruber venit, vidit, dixit 06.02.2009 16:19:
Show 15 quoted lines
> "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>

← back to recent threads