{"thread":{"id":"37880","subject":"[PATCH 2/2] notes: Add --allow-empty, to allow storing empty notes","startedAt":"2014-11-05T01:32:54Z","lastAt":"2014-11-05T18:36:17Z","messageCount":5,"participants":["Johan Herland","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"251398","messageId":"1415151175-1682-1-git-send-email-johan@herland.net","threadId":"37880","inReplyTo":null,"subject":"[PATCH 1/2] t3312-notes-empty: Test that 'git notes' removes empty notes by default","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2014-11-05T01:32:54Z","receivedAt":"2014-11-05T01:32:54Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"Add test cases documenting the current behavior when trying to\nadd/append/edit empty notes. This is in preparation for adding\n--allow-empty; to allow empty notes to be stored.\n\nSigned-off-by: Johan Herland <johan@herland.net>\n---\n t/t3312-notes-empty.sh | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 58 insertions(+)\n create mode 100755 t/t3312-notes-empty.sh\n\ndiff --git a/t/t3312-notes-empty.sh b/t/t3312-notes-empty.sh\nnew file mode 100755\nindex 0000000..2806d27\n--- /dev/null\n+++ b/t/t3312-notes-empty.sh\n@@ -0,0 +1,58 @@\n+#!/bin/sh\n+\n+test_description='Test adding/editing of empty notes'\n+. ./test-lib.sh\n+\n+cat >fake_editor.sh <<\\EOF\n+#!/bin/sh\n+echo \"$MSG\" >\"$1\"\n+echo \"$MSG\" >& 2\n+EOF\n+chmod a+x fake_editor.sh\n+GIT_EDITOR=./fake_editor.sh\n+export GIT_EDITOR\n+\n+test_expect_success 'setup' '\n+\ttest_commit one &&\n+\tempty_blob=$(git hash-object -w /dev/null)\n+'\n+\n+cleanup_notes() {\n+\tgit update-ref -d refs/notes/commits\n+}\n+\n+cat >expect_missing <<\\EOF\n+commit d79ce1670bdcb76e6d1da2ae095e890ccb326ae9\n+Author: A U Thor <author@example.com>\n+Date:   Thu Apr 7 15:13:13 2005 -0700\n+\n+    one\n+EOF\n+\n+verify_missing() {\n+\tgit log -1 > actual &&\n+\ttest_cmp expect_missing actual &&\n+\t! git notes list HEAD\n+}\n+\n+for cmd in \\\n+\t'add' \\\n+\t'add -F /dev/null' \\\n+\t'add -m \"\"' \\\n+\t'add -c \"$empty_blob\"' \\\n+\t'add -C \"$empty_blob\"' \\\n+\t'append' \\\n+\t'append -F /dev/null' \\\n+\t'append -m \"\"' \\\n+\t'append -c \"$empty_blob\"' \\\n+\t'append -C \"$empty_blob\"' \\\n+\t'edit'\n+do\n+\ttest_expect_success \"'git notes $cmd' removes empty note\" \"\n+\t\tcleanup_notes &&\n+\t\tMSG= git notes $cmd &&\n+\t\tverify_missing\n+\t\"\n+done\n+\n+test_done\n-- \n2.0.0.rc4.501.gdaf83ca\n"},{"id":"251397","messageId":"1415151175-1682-2-git-send-email-johan@herland.net","threadId":"37880","inReplyTo":"1415151175-1682-1-git-send-email-johan@herland.net","subject":"[PATCH 2/2] notes: Add --allow-empty, to allow storing empty notes","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2014-11-05T01:32:55Z","receivedAt":"2014-11-05T01:32:55Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"Although the \"git notes\" man page advertises that we support binary-safe\nnotes addition (using the -C option), we currently do not support adding\nthe empty note (i.e. using the empty blob to annotate an object). Instead,\nan empty note is always treated as an intent to remove the note\naltogether.\n\nIntroduce the --allow-empty option to the add/append/edit subcommands,\nto explicitly allow an empty note to be stored into the notes tree.\n\nAlso update the documentation, and add test cases for the new option.\n\nReported-by: James H. Fisher <jhf@trifork.com>\nImproved-by: Kyle J. McKay <mackyle@gmail.com>\nSigned-off-by: Johan Herland <johan@herland.net>\n---\n Documentation/git-notes.txt | 12 ++++++++----\n builtin/notes.c             | 25 +++++++++++++++----------\n notes.c                     |  3 +--\n t/t3312-notes-empty.sh      | 20 +++++++++++++++++++-\n 4 files changed, 43 insertions(+), 17 deletions(-)\n\ndiff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt\nindex 310f0a5..851518d 100644\n--- a/Documentation/git-notes.txt\n+++ b/Documentation/git-notes.txt\n@@ -9,10 +9,10 @@ SYNOPSIS\n --------\n [verse]\n 'git notes' [list [<object>]]\n-'git notes' add [-f] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>]\n+'git notes' add [-f] [--allow-empty] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>]\n 'git notes' copy [-f] ( --stdin | <from-object> <to-object> )\n-'git notes' append [-F <file> | -m <msg> | (-c | -C) <object>] [<object>]\n-'git notes' edit [<object>]\n+'git notes' append [--allow-empty] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>]\n+'git notes' edit [--allow-empty] [<object>]\n 'git notes' show [<object>]\n 'git notes' merge [-v | -q] [-s <strategy> ] <notes-ref>\n 'git notes' merge --commit [-v | -q]\n@@ -155,6 +155,10 @@ OPTIONS\n \tLike '-C', but with '-c' the editor is invoked, so that\n \tthe user can further edit the note message.\n \n+--allow-empty::\n+\tAllow an empty note object to be stored. The default behavior is\n+\tto automatically remove empty notes.\n+\n --ref <ref>::\n \tManipulate the notes tree in <ref>.  This overrides\n \t'GIT_NOTES_REF' and the \"core.notesRef\" configuration.  The ref\n@@ -287,7 +291,7 @@ arbitrary files using 'git hash-object':\n ------------\n $ cc *.c\n $ blob=$(git hash-object -w a.out)\n-$ git notes --ref=built add -C \"$blob\" HEAD\n+$ git notes --ref=built add --allow-empty -C \"$blob\" HEAD\n ------------\n \n (You cannot simply use `git notes --ref=built add -F a.out HEAD`\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 68b6cd8..038a419 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -22,10 +22,10 @@\n \n static const char * const git_notes_usage[] = {\n \tN_(\"git notes [--ref <notes_ref>] [list [<object>]]\"),\n-\tN_(\"git notes [--ref <notes_ref>] add [-f] [-m <msg> | -F <file> | (-c | -C) <object>] [<object>]\"),\n+\tN_(\"git notes [--ref <notes_ref>] add [-f] [--allow-empty] [-m <msg> | -F <file> | (-c | -C) <object>] [<object>]\"),\n \tN_(\"git notes [--ref <notes_ref>] copy [-f] <from-object> <to-object>\"),\n-\tN_(\"git notes [--ref <notes_ref>] append [-m <msg> | -F <file> | (-c | -C) <object>] [<object>]\"),\n-\tN_(\"git notes [--ref <notes_ref>] edit [<object>]\"),\n+\tN_(\"git notes [--ref <notes_ref>] append [--allow-empty] [-m <msg> | -F <file> | (-c | -C) <object>] [<object>]\"),\n+\tN_(\"git notes [--ref <notes_ref>] edit [--allow-empty] [<object>]\"),\n \tN_(\"git notes [--ref <notes_ref>] show [<object>]\"),\n \tN_(\"git notes [--ref <notes_ref>] merge [-v | -q] [-s <strategy> ] <notes_ref>\"),\n \tN_(\"git notes merge --commit [-v | -q]\"),\n@@ -150,8 +150,8 @@ static void write_commented_object(int fd, const unsigned char *object)\n }\n \n static void create_note(const unsigned char *object, struct msg_arg *msg,\n-\t\t\tint append_only, const unsigned char *prev,\n-\t\t\tunsigned char *result)\n+\t\t\tint append_only, int allow_empty,\n+\t\t\tconst unsigned char *prev, unsigned char *result)\n {\n \tchar *path = NULL;\n \n@@ -202,7 +202,7 @@ static void create_note(const unsigned char *object, struct msg_arg *msg,\n \t\tfree(prev_buf);\n \t}\n \n-\tif (!msg->buf.len) {\n+\tif (!allow_empty && !msg->buf.len) {\n \t\tfprintf(stderr, _(\"Removing note for object %s\\n\"),\n \t\t\tsha1_to_hex(object));\n \t\thashclr(result);\n@@ -266,7 +266,7 @@ static int parse_reuse_arg(const struct option *opt, const char *arg, int unset)\n \n \tif (get_sha1(arg, object))\n \t\tdie(_(\"Failed to resolve '%s' as a valid ref.\"), arg);\n-\tif (!(buf = read_sha1_file(object, &type, &len)) || !len) {\n+\tif (!(buf = read_sha1_file(object, &type, &len))) {\n \t\tfree(buf);\n \t\tdie(_(\"Failed to read object '%s'.\"), arg);\n \t}\n@@ -397,7 +397,7 @@ static int append_edit(int argc, const char **argv, const char *prefix);\n \n static int add(int argc, const char **argv, const char *prefix)\n {\n-\tint retval = 0, force = 0;\n+\tint retval = 0, force = 0, allow_empty = 0;\n \tconst char *object_ref;\n \tstruct notes_tree *t;\n \tunsigned char object[20], new_note[20];\n@@ -417,6 +417,8 @@ static int add(int argc, const char **argv, const char *prefix)\n \t\t{ OPTION_CALLBACK, 'C', \"reuse-message\", &msg, N_(\"object\"),\n \t\t\tN_(\"reuse specified note object\"), PARSE_OPT_NONEG,\n \t\t\tparse_reuse_arg},\n+\t\tOPT_BOOL(0, \"allow-empty\", &allow_empty,\n+\t\t\tN_(\"allow storing empty note\")),\n \t\tOPT__FORCE(&force, N_(\"replace existing notes\")),\n \t\tOPT_END()\n \t};\n@@ -460,7 +462,7 @@ static int add(int argc, const char **argv, const char *prefix)\n \t\t\tsha1_to_hex(object));\n \t}\n \n-\tcreate_note(object, &msg, 0, note, new_note);\n+\tcreate_note(object, &msg, 0, allow_empty, note, new_note);\n \n \tif (is_null_sha1(new_note))\n \t\tremove_note(t, object);\n@@ -554,6 +556,7 @@ out:\n \n static int append_edit(int argc, const char **argv, const char *prefix)\n {\n+\tint allow_empty = 0;\n \tconst char *object_ref;\n \tstruct notes_tree *t;\n \tunsigned char object[20], new_note[20];\n@@ -574,6 +577,8 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \t\t{ OPTION_CALLBACK, 'C', \"reuse-message\", &msg, N_(\"object\"),\n \t\t\tN_(\"reuse specified note object\"), PARSE_OPT_NONEG,\n \t\t\tparse_reuse_arg},\n+\t\tOPT_BOOL(0, \"allow-empty\", &allow_empty,\n+\t\t\tN_(\"allow storing empty note\")),\n \t\tOPT_END()\n \t};\n \tint edit = !strcmp(argv[0], \"edit\");\n@@ -600,7 +605,7 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \tt = init_notes_check(argv[0]);\n \tnote = get_note(t, object);\n \n-\tcreate_note(object, &msg, !edit, note, new_note);\n+\tcreate_note(object, &msg, !edit, allow_empty, note, new_note);\n \n \tif (is_null_sha1(new_note))\n \t\tremove_note(t, object);\ndiff --git a/notes.c b/notes.c\nindex 5fe691d..62bc6e1 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -1218,8 +1218,7 @@ static void format_note(struct notes_tree *t, const unsigned char *object_sha1,\n \tif (!sha1)\n \t\treturn;\n \n-\tif (!(msg = read_sha1_file(sha1, &type, &msglen)) || !msglen ||\n-\t\t\ttype != OBJ_BLOB) {\n+\tif (!(msg = read_sha1_file(sha1, &type, &msglen)) || type != OBJ_BLOB) {\n \t\tfree(msg);\n \t\treturn;\n \t}\ndiff --git a/t/t3312-notes-empty.sh b/t/t3312-notes-empty.sh\nindex 2806d27..f89fbc9 100755\n--- a/t/t3312-notes-empty.sh\n+++ b/t/t3312-notes-empty.sh\n@@ -1,6 +1,6 @@\n #!/bin/sh\n \n-test_description='Test adding/editing of empty notes'\n+test_description='Test adding/editing of empty notes with/without --allow-empty'\n . ./test-lib.sh\n \n cat >fake_editor.sh <<\\EOF\n@@ -35,6 +35,18 @@ verify_missing() {\n \t! git notes list HEAD\n }\n \n+cp expect_missing expect_empty\n+cat >>expect_empty <<\\EOF\n+\n+Notes:\n+EOF\n+\n+verify_empty() {\n+\tgit log -1 > actual &&\n+\ttest_cmp expect_empty actual &&\n+\ttest \"$(git notes list HEAD)\" = \"$empty_blob\"\n+}\n+\n for cmd in \\\n \t'add' \\\n \t'add -F /dev/null' \\\n@@ -53,6 +65,12 @@ do\n \t\tMSG= git notes $cmd &&\n \t\tverify_missing\n \t\"\n+\n+\ttest_expect_success \"'git notes $cmd --allow-empty' stores empty note\" \"\n+\t\tcleanup_notes &&\n+\t\tMSG= git notes $cmd --allow-empty &&\n+\t\tverify_empty\n+\t\"\n done\n \n test_done\n-- \n2.0.0.rc4.501.gdaf83ca\n"},{"id":"251400","messageId":"CAPig+cT4-1bY5tq8KioC8Js3ZUfZCuFEwOZMeoPW4M_brK+QXw@mail.gmail.com","threadId":"37880","inReplyTo":"1415151175-1682-1-git-send-email-johan@herland.net","subject":"Re: [PATCH 1/2] t3312-notes-empty: Test that 'git notes' removes empty notes by default","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-11-05T04:10:22Z","receivedAt":"2014-11-05T04:10:22Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Nov 4, 2014 at 8:32 PM, Johan Herland <johan@herland.net> wrote:\n> Add test cases documenting the current behavior when trying to\n> add/append/edit empty notes. This is in preparation for adding\n> --allow-empty; to allow empty notes to be stored.\n>\n> Signed-off-by: Johan Herland <johan@herland.net>\n> ---\n>  t/t3312-notes-empty.sh | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++\n>  1 file changed, 58 insertions(+)\n>  create mode 100755 t/t3312-notes-empty.sh\n>\n> diff --git a/t/t3312-notes-empty.sh b/t/t3312-notes-empty.sh\n> new file mode 100755\n> index 0000000..2806d27\n> --- /dev/null\n> +++ b/t/t3312-notes-empty.sh\n> @@ -0,0 +1,58 @@\n> +#!/bin/sh\n> +\n> +test_description='Test adding/editing of empty notes'\n> +. ./test-lib.sh\n> +\n> +cat >fake_editor.sh <<\\EOF\n> +#!/bin/sh\n> +echo \"$MSG\" >\"$1\"\n> +echo \"$MSG\" >& 2\n> +EOF\n> +chmod a+x fake_editor.sh\n\nwrite_script() would allow you to drop the #!/bin/sh and chmod lines.\n\n> +GIT_EDITOR=./fake_editor.sh\n> +export GIT_EDITOR\n> +\n> +test_expect_success 'setup' '\n> +       test_commit one &&\n> +       empty_blob=$(git hash-object -w /dev/null)\n> +'\n> +\n> +cleanup_notes() {\n> +       git update-ref -d refs/notes/commits\n> +}\n> +\n> +cat >expect_missing <<\\EOF\n> +commit d79ce1670bdcb76e6d1da2ae095e890ccb326ae9\n> +Author: A U Thor <author@example.com>\n> +Date:   Thu Apr 7 15:13:13 2005 -0700\n> +\n> +    one\n> +EOF\n\nRather than hard-coding this output, generating it would make the test\nscript less fragile:\n\n    git log -1 >expect_missing\n\n> +verify_missing() {\n> +       git log -1 > actual &&\n> +       test_cmp expect_missing actual &&\n> +       ! git notes list HEAD\n> +}\n> +\n> +for cmd in \\\n> +       'add' \\\n> +       'add -F /dev/null' \\\n> +       'add -m \"\"' \\\n> +       'add -c \"$empty_blob\"' \\\n> +       'add -C \"$empty_blob\"' \\\n> +       'append' \\\n> +       'append -F /dev/null' \\\n> +       'append -m \"\"' \\\n> +       'append -c \"$empty_blob\"' \\\n> +       'append -C \"$empty_blob\"' \\\n> +       'edit'\n> +do\n> +       test_expect_success \"'git notes $cmd' removes empty note\" \"\n> +               cleanup_notes &&\n> +               MSG= git notes $cmd &&\n> +               verify_missing\n> +       \"\n> +done\n\nEach -c/-C case fails for me when trying to read $empty_object. For example:\n\nfatal: Failed to read object 'e69de29bb2d1d6434b8b29ae775ad8c2e48c5391'.\nnot ok 5 - 'git notes add -c \"$empty_blob\"' removes empty note\n\n> +\n> +test_done\n> --\n> 2.0.0.rc4.501.gdaf83ca\n"},{"id":"251401","messageId":"CALKQrgdtvfZ+LFn+VSE-yjvJf1zwTZdEov48eDbhvx0JWHpeug@mail.gmail.com","threadId":"37880","inReplyTo":"CAPig+cT4-1bY5tq8KioC8Js3ZUfZCuFEwOZMeoPW4M_brK+QXw@mail.gmail.com","subject":"Re: [PATCH 1/2] t3312-notes-empty: Test that 'git notes' removes empty notes by default","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2014-11-05T08:32:49Z","receivedAt":"2014-11-05T08:32:49Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Wed, Nov 5, 2014 at 5:10 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n\n[...]\n\n> write_script() would allow you to drop the #!/bin/sh and chmod lines.\n\n[...]\n\n> Rather than hard-coding this output, generating it would make the test\n> script less fragile:\n>\n>     git log -1 >expect_missing\n\n[...]\n\n> Each -c/-C case fails for me when trying to read $empty_object. For example:\n>\n> fatal: Failed to read object 'e69de29bb2d1d6434b8b29ae775ad8c2e48c5391'.\n> not ok 5 - 'git notes add -c \"$empty_blob\"' removes empty note\n\nThese are all fixed in the re-roll.\n\nThanks for the feedback!\n\n\n...Johan\n\n-- \nJohan Herland, <johan@herland.net>\nwww.herland.net\n"},{"id":"251413","messageId":"xmqqr3xho5pa.fsf@gitster.dls.corp.google.com","threadId":"37880","inReplyTo":"1415151175-1682-1-git-send-email-johan@herland.net","subject":"Re: [PATCH 1/2] t3312-notes-empty: Test that 'git notes' removes empty notes by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-05T18:36:17Z","receivedAt":"2014-11-05T18:36:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland <johan@herland.net> writes:\n\n> Add test cases documenting the current behavior when trying to\n> add/append/edit empty notes. This is in preparation for adding\n> --allow-empty; to allow empty notes to be stored.\n>\n> Signed-off-by: Johan Herland <johan@herland.net>\n> ---\n>  t/t3312-notes-empty.sh | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++\n>  1 file changed, 58 insertions(+)\n>  create mode 100755 t/t3312-notes-empty.sh\n\nBy definition, an empty note is empty ;-) and devoid of useful\ninformation other than a single bit, its existence.  I would\nunderstand a handful of tests that check \"oh by the way, we should\nalso handle empty ones sensibly\", but are you sure that a _new_\nseparate test script, not addition to existing test script, is worth\nto check _only_ empty notes?\n\n> diff --git a/t/t3312-notes-empty.sh b/t/t3312-notes-empty.sh\n> new file mode 100755\n> index 0000000..2806d27\n> --- /dev/null\n> +++ b/t/t3312-notes-empty.sh\n> @@ -0,0 +1,58 @@\n> +#!/bin/sh\n> +\n> +test_description='Test adding/editing of empty notes'\n> +. ./test-lib.sh\n> +\n> +cat >fake_editor.sh <<\\EOF\n> +#!/bin/sh\n> +echo \"$MSG\" >\"$1\"\n> +echo \"$MSG\" >& 2\n> +EOF\n> +chmod a+x fake_editor.sh\n> +GIT_EDITOR=./fake_editor.sh\n> +export GIT_EDITOR\n> +\n> +test_expect_success 'setup' '\n> +\ttest_commit one &&\n> +\tempty_blob=$(git hash-object -w /dev/null)\n> +'\n> +\n> +cleanup_notes() {\n> +\tgit update-ref -d refs/notes/commits\n> +}\n> +\n> +cat >expect_missing <<\\EOF\n> +commit d79ce1670bdcb76e6d1da2ae095e890ccb326ae9\n> +Author: A U Thor <author@example.com>\n> +Date:   Thu Apr 7 15:13:13 2005 -0700\n> +\n> +    one\n> +EOF\n> +\n> +verify_missing() {\n> +\tgit log -1 > actual &&\n> +\ttest_cmp expect_missing actual &&\n> +\t! git notes list HEAD\n> +}\n> +\n> +for cmd in \\\n> +\t'add' \\\n> +\t'add -F /dev/null' \\\n> +\t'add -m \"\"' \\\n> +\t'add -c \"$empty_blob\"' \\\n> +\t'add -C \"$empty_blob\"' \\\n> +\t'append' \\\n> +\t'append -F /dev/null' \\\n> +\t'append -m \"\"' \\\n> +\t'append -c \"$empty_blob\"' \\\n> +\t'append -C \"$empty_blob\"' \\\n> +\t'edit'\n> +do\n> +\ttest_expect_success \"'git notes $cmd' removes empty note\" \"\n> +\t\tcleanup_notes &&\n> +\t\tMSG= git notes $cmd &&\n> +\t\tverify_missing\n> +\t\"\n> +done\n> +\n> +test_done\n"}]}