{"thread":{"id":"17614","subject":"[PATCH 2/2] handle empty notes gracefully","startedAt":"2009-02-06T15:19:01Z","lastAt":"2009-02-06T15:50:17Z","messageCount":9,"participants":["Michael J Gruber","Johannes Schindelin","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"103500","messageId":"1233933543-28407-1-git-send-email-git@drmicha.warpmail.net","threadId":"17614","inReplyTo":null,"subject":"[PATCH 0/2] git notes show: handle empty notes gracefully","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2009-02-06T15:19:01Z","receivedAt":"2009-02-06T15:19:01Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"\"git notes show\" barfs when there is no note to show. This introduces a\ntest (yes, a test!) and, in a second round, reacts more gracefully to\nempty notes and adjust the expected test output accordingly.\n\nNote that in both cases (before/after the patch) the return code is\nnon-zero: It's 128 in the ungraceful case, 1 when \"dieing gracefully\",\nuhm...\n\nMichael J Gruber (2):\n  git notes show: test empty notes\n  handle empty notes gracefully\n\n git-notes.sh     |    2 ++\n t/t3301-notes.sh |    5 +++++\n 2 files changed, 7 insertions(+), 0 deletions(-)\n"},{"id":"103499","messageId":"1233933543-28407-2-git-send-email-git@drmicha.warpmail.net","threadId":"17614","inReplyTo":"1233933543-28407-1-git-send-email-git@drmicha.warpmail.net","subject":"[PATCH 1/2] git notes show: test empty notes","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2009-02-06T15:19:02Z","receivedAt":"2009-02-06T15:19:02Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Add a test for the handling of empty notes by \"git notes show\".\n---\n t/t3301-notes.sh |    5 +++++\n 1 files changed, 5 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t3301-notes.sh b/t/t3301-notes.sh\nindex 9393a25..4900dca 100755\n--- a/t/t3301-notes.sh\n+++ b/t/t3301-notes.sh\n@@ -35,6 +35,11 @@ test_expect_success 'need valid notes ref' '\n \t! MSG=2 GIT_NOTES_REF='/' git notes show\n '\n \n+# 1 indicates caught gracefully by die, 128 means git-show barfed\n+test_expect_failure 'handle empty notes gracefully' '\n+\tgit notes show || test 1 = $?\n+'\n+\n test_expect_success 'create notes' '\n \tgit config core.notesRef refs/notes/commits &&\n \tMSG=b1 git notes edit &&\n-- \n1.6.1.2.253.ga34a\n"},{"id":"103498","messageId":"1233933543-28407-3-git-send-email-git@drmicha.warpmail.net","threadId":"17614","inReplyTo":"1233933543-28407-2-git-send-email-git@drmicha.warpmail.net","subject":"[PATCH 2/2] handle empty notes gracefully","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2009-02-06T15:19:03Z","receivedAt":"2009-02-06T15:19:03Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Currently, git-notes barfs when asked to show an empty (i.e.\nnon-existing) note. Change this to explicitely say there is none.\n---\n git-notes.sh     |    2 ++\n t/t3301-notes.sh |    2 +-\n 2 files changed, 3 insertions(+), 1 deletions(-)\n\ndiff --git a/git-notes.sh b/git-notes.sh\nindex bfdbaa8..9cbad02 100755\n--- a/git-notes.sh\n+++ b/git-notes.sh\n@@ -58,6 +58,8 @@ edit)\n \t\t\"$GIT_NOTES_REF\" $NEW_HEAD $CURRENT_HEAD\n ;;\n show)\n+\tgit rev-parse -q --verify \"$GIT_NOTES_REF\":$COMMIT > /dev/null ||\n+\t\tdie \"No note for commit $COMMIT.\"\n \tgit show \"$GIT_NOTES_REF\":$COMMIT\n ;;\n *)\ndiff --git a/t/t3301-notes.sh b/t/t3301-notes.sh\nindex 4900dca..81d5028 100755\n--- a/t/t3301-notes.sh\n+++ b/t/t3301-notes.sh\n@@ -36,7 +36,7 @@ test_expect_success 'need valid notes ref' '\n '\n \n # 1 indicates caught gracefully by die, 128 means git-show barfed\n-test_expect_failure 'handle empty notes gracefully' '\n+test_expect_success 'handle empty notes gracefully' '\n \tgit notes show || test 1 = $?\n '\n \n-- \n1.6.1.2.253.ga34a\n"},{"id":"103506","messageId":"alpine.DEB.1.00.0902061635560.7377@intel-tinevez-2-302","threadId":"17614","inReplyTo":"1233933543-28407-2-git-send-email-git@drmicha.warpmail.net","subject":"Re: [PATCH 1/2] git notes show: test empty notes","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-02-06T15:36:14Z","receivedAt":"2009-02-06T15:36:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 6 Feb 2009, Michael J Gruber wrote:\n\n> Add a test for the handling of empty notes by \"git notes show\".\n> ---\n>  t/t3301-notes.sh |    5 +++++\n>  1 files changed, 5 insertions(+), 0 deletions(-)\n> \n> diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh\n> index 9393a25..4900dca 100755\n> --- a/t/t3301-notes.sh\n> +++ b/t/t3301-notes.sh\n> @@ -35,6 +35,11 @@ test_expect_success 'need valid notes ref' '\n>  \t! MSG=2 GIT_NOTES_REF='/' git notes show\n>  '\n>  \n> +# 1 indicates caught gracefully by die, 128 means git-show barfed\n> +test_expect_failure 'handle empty notes gracefully' '\n> +\tgit notes show || test 1 = $?\n> +'\n\nThat test would succeed if the exit status is 0.\n\nCiao,\nDscho\n"},{"id":"103507","messageId":"498C5923.8070009@drmicha.warpmail.net","threadId":"17614","inReplyTo":"1233933543-28407-1-git-send-email-git@drmicha.warpmail.net","subject":"Re: [PATCH 0/2] git notes show: handle empty notes gracefully","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2009-02-06T15:37:07Z","receivedAt":"2009-02-06T15:37:07Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Michael J Gruber venit, vidit, dixit 06.02.2009 16:19:\n> \"git notes show\" barfs when there is no note to show. This introduces a\n> test (yes, a test!) and, in a second round, reacts more gracefully to\n> empty notes and adjust the expected test output accordingly.\n> \n> Note that in both cases (before/after the patch) the return code is\n> non-zero: It's 128 in the ungraceful case, 1 when \"dieing gracefully\",\n> uhm...\n> \n> Michael J Gruber (2):\n>   git notes show: test empty notes\n>   handle empty notes gracefully\n> \n>  git-notes.sh     |    2 ++\n>  t/t3301-notes.sh |    5 +++++\n>  2 files changed, 7 insertions(+), 0 deletions(-)\n\nUhm, I'm about 1 hour late with my afternoon coffee, and I'm afraid that\nshows:\n\n1) I meant \"bark\", not \"barf\", although both make sense here.\n\n2) When did \"format.signoff = true\" in config stop working? OK, it never\ndid. So please consider this (anyway trivial series):\n\nSigned-off-by: Michael J Gruber <git@drmicha.warpmail.net>\n"},{"id":"103508","messageId":"alpine.DEB.1.00.0902061636320.7377@intel-tinevez-2-302","threadId":"17614","inReplyTo":"1233933543-28407-3-git-send-email-git@drmicha.warpmail.net","subject":"Re: [PATCH 2/2] handle empty notes gracefully","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-02-06T15:38:14Z","receivedAt":"2009-02-06T15:38:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 6 Feb 2009, Michael J Gruber wrote:\n\n> Currently, git-notes barfs when asked to show an empty (i.e.\n> non-existing) note. Change this to explicitely say there is none.\n> ---\n>  git-notes.sh     |    2 ++\n>  t/t3301-notes.sh |    2 +-\n>  2 files changed, 3 insertions(+), 1 deletions(-)\n> \n> diff --git a/git-notes.sh b/git-notes.sh\n> index bfdbaa8..9cbad02 100755\n> --- a/git-notes.sh\n> +++ b/git-notes.sh\n> @@ -58,6 +58,8 @@ edit)\n>  \t\t\"$GIT_NOTES_REF\" $NEW_HEAD $CURRENT_HEAD\n>  ;;\n>  show)\n> +\tgit rev-parse -q --verify \"$GIT_NOTES_REF\":$COMMIT > /dev/null ||\n> +\t\tdie \"No note for commit $COMMIT.\"\n\nThis looks good.\n\n> diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh\n> index 4900dca..81d5028 100755\n> --- a/t/t3301-notes.sh\n> +++ b/t/t3301-notes.sh\n> @@ -36,7 +36,7 @@ test_expect_success 'need valid notes ref' '\n>  '\n>  \n>  # 1 indicates caught gracefully by die, 128 means git-show barfed\n> -test_expect_failure 'handle empty notes gracefully' '\n> +test_expect_success 'handle empty notes gracefully' '\n>  \tgit notes show || test 1 = $?\n\nCompletely forgot to mention that I think you want to use test_must_fail \nhere.  And maybe you want to be more explicit, by specifying which \ncommit's notes are expected not to be there.\n\nWe would not want the test to succeed for all the wrong reasons, would we?\n\nCiao,\nDscho\n"},{"id":"103510","messageId":"498C5A7B.3030506@drmicha.warpmail.net","threadId":"17614","inReplyTo":"alpine.DEB.1.00.0902061635560.7377@intel-tinevez-2-302","subject":"Re: [PATCH 1/2] git notes show: test empty notes","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2009-02-06T15:42:51Z","receivedAt":"2009-02-06T15:42:51Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Johannes Schindelin venit, vidit, dixit 06.02.2009 16:36:\n> Hi,\n> \n> On Fri, 6 Feb 2009, Michael J Gruber wrote:\n> \n>> Add a test for the handling of empty notes by \"git notes show\".\n>> ---\n>>  t/t3301-notes.sh |    5 +++++\n>>  1 files changed, 5 insertions(+), 0 deletions(-)\n>>\n>> diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh\n>> index 9393a25..4900dca 100755\n>> --- a/t/t3301-notes.sh\n>> +++ b/t/t3301-notes.sh\n>> @@ -35,6 +35,11 @@ test_expect_success 'need valid notes ref' '\n>>  \t! MSG=2 GIT_NOTES_REF='/' git notes show\n>>  '\n>>  \n>> +# 1 indicates caught gracefully by die, 128 means git-show barfed\n>> +test_expect_failure 'handle empty notes gracefully' '\n>> +\tgit notes show || test 1 = $?\n>> +'\n> \n> That test would succeed if the exit status is 0.\n\nYes. If \"git notes show\" returns 0 then even better. It does neither\nbefore nor after the patch. Or should we always expect 1 and test for\nthat? I thought about grepping the output for \"fatal\" (which appears\nnow) but that seemed ugly.\n\nMichael\n"},{"id":"103514","messageId":"498C5BFA.7030107@viscovery.net","threadId":"17614","inReplyTo":"alpine.DEB.1.00.0902061636320.7377@intel-tinevez-2-302","subject":"Re: [PATCH 2/2] handle empty notes gracefully","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-02-06T15:49:14Z","receivedAt":"2009-02-06T15:49:14Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Johannes Schindelin schrieb:\n> On Fri, 6 Feb 2009, Michael J Gruber wrote:\n>> +test_expect_success 'handle empty notes gracefully' '\n>>  \tgit notes show || test 1 = $?\n> \n> Completely forgot to mention that I think you want to use test_must_fail \n> here.\n\nWouldn't work. We want to differentiate between different exit codes. But\nafter test_must_fail the exit code is gone. In this simple case this\nshould work:\n\n\tgit notes show; test 1 = $?\n\n-- Hannes\n"},{"id":"103515","messageId":"498C5C39.7020503@drmicha.warpmail.net","threadId":"17614","inReplyTo":"alpine.DEB.1.00.0902061636320.7377@intel-tinevez-2-302","subject":"Re: [PATCH 2/2] handle empty notes gracefully","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2009-02-06T15:50:17Z","receivedAt":"2009-02-06T15:50:17Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Johannes Schindelin venit, vidit, dixit 06.02.2009 16:38:\n> Hi,\n> \n> On Fri, 6 Feb 2009, Michael J Gruber wrote:\n> \n>> Currently, git-notes barfs when asked to show an empty (i.e.\n>> non-existing) note. Change this to explicitely say there is none.\n>> ---\n>>  git-notes.sh     |    2 ++\n>>  t/t3301-notes.sh |    2 +-\n>>  2 files changed, 3 insertions(+), 1 deletions(-)\n>>\n>> diff --git a/git-notes.sh b/git-notes.sh\n>> index bfdbaa8..9cbad02 100755\n>> --- a/git-notes.sh\n>> +++ b/git-notes.sh\n>> @@ -58,6 +58,8 @@ edit)\n>>  \t\t\"$GIT_NOTES_REF\" $NEW_HEAD $CURRENT_HEAD\n>>  ;;\n>>  show)\n>> +\tgit rev-parse -q --verify \"$GIT_NOTES_REF\":$COMMIT > /dev/null ||\n>> +\t\tdie \"No note for commit $COMMIT.\"\n> \n> This looks good.\n> \n>> diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh\n>> index 4900dca..81d5028 100755\n>> --- a/t/t3301-notes.sh\n>> +++ b/t/t3301-notes.sh\n>> @@ -36,7 +36,7 @@ test_expect_success 'need valid notes ref' '\n>>  '\n>>  \n>>  # 1 indicates caught gracefully by die, 128 means git-show barfed\n>> -test_expect_failure 'handle empty notes gracefully' '\n>> +test_expect_success 'handle empty notes gracefully' '\n>>  \tgit notes show || test 1 = $?\n> \n> Completely forgot to mention that I think you want to use test_must_fail \n> here. \n\nWhat does test_must_fail mean? I don't see it in t/README.\n\n> And maybe you want to be more explicit, by specifying which \n> commit's notes are expected not to be there.\n\nWell, HEAD's notes. I can say HEAD explicitly, of course.\n\n> We would not want the test to succeed for all the wrong reasons, would we?\n\nWell, I could test for the friendly \"No note for commit\" message on\noutput, I just thought that is fragile.\n\nYou see, I did not even want to write a test for such a simple patch... ;)\n\nOK, do we agree on the following intended behaviour for git notes show:\n- return 0 if a note can be shown\n- return 1 if there is none (i.e. die gracefully)\n- return something else (i.e. die fatally) if something really bad happens\n\nThen I should rewrite the test to check for \"1\" and only \"1\".\n\nNow, the coffee...\n\nMichael\n"}]}