{"thread":{"id":"37606","subject":"Bug/request: the empty string should be a valid git note","startedAt":"2014-09-20T19:47:42Z","lastAt":"2014-09-22T17:39:00Z","messageCount":8,"participants":["James H. Fisher","Johan Herland","Torsten Bögershausen","Kyle J. McKay","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"249702","messageId":"F9750CC0-3FAC-4B50-AB6A-BFD6A7D0BE97@trifork.com","threadId":"37606","inReplyTo":null,"subject":"Bug/request: the empty string should be a valid git note","fromName":"James H. Fisher","fromEmail":"jhf@trifork.com","sentAt":"2014-09-20T19:47:42Z","receivedAt":"2014-09-20T19:47:42Z","isPatch":false,"sender":{"key":"jhf@trifork.com","avatar":null},"body":"In the documentation for git notes [1] I read:\n\n In principle, a note is a regular Git blob, and any kind of (non-)format is accepted.\n\nThen, since the empty string is a valid regular Git blob, the empty string is also a valid git note.\n\nTherefore this behavior was unexpected for me:\n\n > git notes --ref=foo add -m ''\n Removing note for object 97b8860c071898d9e162678ea1035a8ced2f8b1f\n\nI was surprised to see that this behavior was deliberately introduced:\n\n > git log -1 a0b4dfa\n commit a0b4dfa9b35a2ebac578ea5547b041bb78557238\n Author: Johan Herland \n Date: Sat Feb 13 22:28:24 2010 +0100\n\n Teach builtin-notes to remove empty notes\n\n When the result of editing a note is an empty string, the associated note\n entry should be deleted from the notes tree.\n\n This allows deleting notes by invoking either \"git notes -m ''\" or\n \"git notes -F /dev/null\".\n\n Signed-off-by: Johan Herland \n Signed-off-by: Junio C Hamano \n\nI don’t understand what the motivation for this change was. Yes, it \"allows deleting notes\" by providing the empty string, but there is a specific subcommand for removal of a note, `git notes remove`, which makes this intention much clearer.\n\nI have specific motivation for wanting to store the empty string as a git note, as distinct from the non-existence of a note for the object. (Specifically I have a tool to annotate a commit with a list of files that satisfy a certain condition. The empty string represents the empty list, a valid value which asserts that no files satisfied the condition. I can imagine many other use cases for which the empty string is a useful git note.)\n\nDoes anyone know why we have the existing behavior? Is it for \"technical reasons” or was it actually considered desirable?\n\nJames Fisher \n\n[1]: https://www.kernel.org/pub/software/scm/git/docs/git-notes.html"},{"id":"249704","messageId":"9AE29EAE-8471-4F1F-B4CB-37FA830E3DAF@trifork.com","threadId":"37606","inReplyTo":"F9750CC0-3FAC-4B50-AB6A-BFD6A7D0BE97@trifork.com","subject":"Re: Bug/request: the empty string should be a valid git note","fromName":"James H. Fisher","fromEmail":"jhf@trifork.com","sentAt":"2014-09-20T21:50:53Z","receivedAt":"2014-09-20T21:50:53Z","isPatch":false,"sender":{"key":"jhf@trifork.com","avatar":null},"body":"Apologies for re-sending; unclear whether my email was delivered since I sent it before my subscription was confirmed.\n\nOn 20 Sep 2014, at 20:47, James H. Fisher wrote:\n\n> In the documentation for git notes [1] I read:\n> \n> In principle, a note is a regular Git blob, and any kind of (non-)format is accepted.\n> \n> Then, since the empty string is a valid regular Git blob, the empty string is also a valid git note.\n> \n> Therefore this behavior was unexpected for me:\n> \n>> git notes --ref=foo add -m ''\n> Removing note for object 97b8860c071898d9e162678ea1035a8ced2f8b1f\n> \n> I was surprised to see that this behavior was deliberately introduced:\n> \n>> git log -1 a0b4dfa\n> commit a0b4dfa9b35a2ebac578ea5547b041bb78557238\n> Author: Johan Herland \n> Date: Sat Feb 13 22:28:24 2010 +0100\n> \n> Teach builtin-notes to remove empty notes\n> \n> When the result of editing a note is an empty string, the associated note\n> entry should be deleted from the notes tree.\n> \n> This allows deleting notes by invoking either \"git notes -m ''\" or\n> \"git notes -F /dev/null\".\n> \n> Signed-off-by: Johan Herland \n> Signed-off-by: Junio C Hamano \n> \n> I don’t understand what the motivation for this change was. Yes, it \"allows deleting notes\" by providing the empty string, but there is a specific subcommand for removal of a note, `git notes remove`, which makes this intention much clearer.\n> \n> I have specific motivation for wanting to store the empty string as a git note, as distinct from the non-existence of a note for the object. (Specifically I have a tool to annotate a commit with a list of files that satisfy a certain condition. The empty string represents the empty list, a valid value which asserts that no files satisfied the condition. I can imagine many other use cases for which the empty string is a useful git note.)\n> \n> Does anyone know why we have the existing behavior? Is it for \"technical reasons” or was it actually considered desirable?\n> \n> James Fisher \n> \n> [1]: https://www.kernel.org/pub/software/scm/git/docs/git-notes.html\n"},{"id":"249705","messageId":"CALKQrgd9BPUTrgZvFCj_fznRG6RmfiGzW68XF++yykMguypTig@mail.gmail.com","threadId":"37606","inReplyTo":"F9750CC0-3FAC-4B50-AB6A-BFD6A7D0BE97@trifork.com","subject":"Re: Bug/request: the empty string should be a valid git note","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2014-09-21T01:44:39Z","receivedAt":"2014-09-21T01:44:39Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Sat, Sep 20, 2014 at 9:47 PM, James H. Fisher wrote:\n> In the documentation for git notes [1] I read:\n>\n> In principle, a note is a regular Git blob, and any kind of\n> (non-)format is accepted.\n>\n> Then, since the empty string is a valid regular Git blob, the empty\n> string is also a valid git note.\n\nAgreed.\n\n> Therefore this behavior was unexpected for me:\n>\n> > git notes --ref=foo add -m ''\n> Removing note for object 97b8860c071898d9e162678ea1035a8ced2f8b1f\n\nAlthough I can understand your surprise, if you keep reading the\nsentences following the above quote from the manual page, it says:\n\n You can binary-safely create notes from arbitrary files using\n 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\nThis might indicate that even if \"git notes add -m ''\" does not create\nan empty note, then at least the following command should:\n\n git notes add -C e69de29bb2d1d6434b8b29ae775ad8c2e48c5391\n\n(that is the SHA1 sum of the empty blob)\n\nHowever (and this is where I start to grow surprised as well), that\n_also_ totally fails to create an empty note!\n\nThis is IMHO clearly a bug: If we advertise a binary-safe method for\ncreating git-notes, then that method should definitely also support\ncreating empty notes.\n\nWhether or not \"git notes add -m ''\" should also create an empty note,\nis not necessarily that clear to me. There are historical reasons for\nthe current behavior (see below), but there is also a certain element\nof convenience for the user here; consider the following:\n\nWhen you run \"git commit\", git opens your editor to let you write the\ncommit message. However, if you change your mind, and want to abort\nthe commit, you can simply write nothing (or exit without saving),\nand git will respond with:\n\n Aborting commit due to empty commit message.\n\nThe same applies to \"git commit -m ''\".\n\nThis is the pattern on which \"git notes add\" is modeled: Empty input\nis interpreted as an intent to abort, which, in the notes case is to\nsimply not create the note at all.\n\n> I was surprised to see that this behavior was deliberately introduced:\n>\n> > git log -1 a0b4dfa\n> commit a0b4dfa9b35a2ebac578ea5547b041bb78557238\n> Author: Johan Herland \n> Date: Sat Feb 13 22:28:24 2010 +0100\n>\n> Teach builtin-notes to remove empty notes\n>\n> When the result of editing a note is an empty string, the\n> associated note entry should be deleted from the notes tree.\n>\n> This allows deleting notes by invoking either \"git notes -m ''\"\n> or \"git notes -F /dev/null\".\n>\n> Signed-off-by: Johan Herland \n> Signed-off-by: Junio C Hamano \n>\n> I don’t understand what the motivation for this change was. Yes, it\n> \"allows deleting notes\" by providing the empty string, but there is\n> a specific subcommand for removal of a note, `git notes remove`,\n> which makes this intention much clearer.\n\nActually, if you take a closer look at the context for that commit,\nyou will see that things were in fact the other way around: auto-\nremoving empty notes was added _before_ \"git notes remove\", and if\nyou look at 92b3385f which introduces \"git notes remove\", you'll see\nthat it was initially implemented wholly in terms of editing the note\nto become empty, which then caused its removal.\n\nThat said, these are unimportant historical details, and should not\nserve as justification for continued incorrect behavior.\n\n> I have specific motivation for wanting to store the empty string as\n> a git note, as distinct from the non-existence of a note for the\n> object. (Specifically I have a tool to annotate a commit with a list\n> of files that satisfy a certain condition. The empty string represents\n> the empty list, a valid value which asserts that no files satisfied\n> the condition. I can imagine many other use cases for which the empty\n> string is a useful git note.)\n\nI do not disagre with your use case at all, and it is regrettable that\nthere is no simple workaround for creating empty notes. Assuming that\nyour lists of files use e.g. a newline to separate filenames, then I\nguess you could use a note consisting of a single newline to signify\nan empty list (\"echo | git hash-object -w --stdin\"). At least as a\ntemporary workaround until we get this fixed in Git.\n\n> Does anyone know why we have the existing behavior? Is it for\n> \"technical reasons” or was it actually considered desirable?\n\nI guess it's \"historical reasons\". But we should definitely fix it.\n\nAt least, we should fix\n\n git notes add -C e69de29bb2d1d6434b8b29ae775ad8c2e48c5391\n\nWhether we should also change\n\n git notes add -m ''\n\nto create an empty note, or leave it as-is, (i.e. similar in spirit to\n\"git commit -m ''\"), I'll leave up to further discussion.\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"249708","messageId":"1411268449-14636-1-git-send-email-johan@herland.net","threadId":"37606","inReplyTo":"CALKQrgd9BPUTrgZvFCj_fznRG6RmfiGzW68XF++yykMguypTig@mail.gmail.com","subject":"[RFC/PATCH] notes: Allow adding empty notes with -C","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2014-09-21T03:00:49Z","receivedAt":"2014-09-21T03:00:49Z","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 with the -C option, we currently do not support adding the\nempty 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 a flag to builtin/notes which indicates whether or not to\nremove an empty note, and disable that flag for the -C option (leave it\nenabled to preserve the current behavior for all other options).\n\nAlso add a test case that verifies that we can not indeed add empty notes\nwith \"git notes add -C $empty_blob\".\n\nReported-by: James H. Fisher \nSigned-off-by: Johan Herland \n---\n builtin/notes.c | 16 +++++++++++-----\n notes.c | 3 +--\n t/t3301-notes.sh | 19 +++++++++++++++++++\n 3 files changed, 31 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 67d0bb1..f8ff590 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -95,6 +95,7 @@ static const char note_template[] =\n struct msg_arg {\n \tint given;\n \tint use_editor;\n+\tint remove_if_empty;\n \tstruct strbuf buf;\n };\n \n@@ -202,7 +203,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 (msg->remove_if_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@@ -233,6 +234,7 @@ static int parse_msg_arg(const struct option *opt, const char *arg, int unset)\n \tstripspace(&(msg->buf), 0);\n \n \tmsg->given = 1;\n+\tmsg->remove_if_empty = 1;\n \treturn 0;\n }\n \n@@ -250,6 +252,7 @@ static int parse_file_arg(const struct option *opt, const char *arg, int unset)\n \tstripspace(&(msg->buf), 0);\n \n \tmsg->given = 1;\n+\tmsg->remove_if_empty = 1;\n \treturn 0;\n }\n \n@@ -266,7 +269,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@@ -278,14 +281,17 @@ static int parse_reuse_arg(const struct option *opt, const char *arg, int unset)\n \tfree(buf);\n \n \tmsg->given = 1;\n+\tmsg->remove_if_empty = 0;\n \treturn 0;\n }\n \n static int parse_reedit_arg(const struct option *opt, const char *arg, int unset)\n {\n+\tint ret = parse_reuse_arg(opt, arg, unset);\n \tstruct msg_arg *msg = opt->value;\n \tmsg->use_editor = 1;\n-\treturn parse_reuse_arg(opt, arg, unset);\n+\tmsg->remove_if_empty = 1;\n+\treturn ret;\n }\n \n static int notes_copy_from_stdin(int force, const char *rewrite_cmd)\n@@ -403,7 +409,7 @@ static int add(int argc, const char **argv, const char *prefix)\n \tunsigned char object[20], new_note[20];\n \tchar logmsg[100];\n \tconst unsigned char *note;\n-\tstruct msg_arg msg = { 0, 0, STRBUF_INIT };\n+\tstruct msg_arg msg = { 0, 0, 1, STRBUF_INIT };\n \tstruct option options[] = {\n \t\t{ OPTION_CALLBACK, 'm', \"message\", &msg, N_(\"message\"),\n \t\t\tN_(\"note contents as a string\"), PARSE_OPT_NONEG,\n@@ -560,7 +566,7 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \tconst unsigned char *note;\n \tchar logmsg[100];\n \tconst char * const *usage;\n-\tstruct msg_arg msg = { 0, 0, STRBUF_INIT };\n+\tstruct msg_arg msg = { 0, 0, 1, STRBUF_INIT };\n \tstruct option options[] = {\n \t\t{ OPTION_CALLBACK, 'm', \"message\", &msg, N_(\"message\"),\n \t\t\tN_(\"note contents as a string\"), PARSE_OPT_NONEG,\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/t3301-notes.sh b/t/t3301-notes.sh\nindex cfd67ff..a6c399b 100755\n--- a/t/t3301-notes.sh\n+++ b/t/t3301-notes.sh\n@@ -1239,4 +1239,23 @@ test_expect_success 'git notes get-ref (--ref)' '\n \ttest \"$(GIT_NOTES_REF=refs/notes/bar git notes --ref=baz get-ref)\" = \"refs/notes/baz\"\n '\n \n+cat > expect << EOF\n+commit 085b0d1309902c3148feb5a136515bdb9a1cd614\n+Author: A U Thor \n+Date: Thu Apr 7 15:28:13 2005 -0700\n+\n+ 16th\n+\n+Notes (foo):\n+EOF\n+\n+test_expect_success 'can create empty note with \"git notes add -C $empty_blob\"' '\n+\ttest_commit 16th &&\n+\tblob=e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 &&\n+\tgit notes add -C $blob &&\n+\tgit log -1 > actual &&\n+\ttest_cmp expect actual &&\n+\ttest \"$(git notes list HEAD)\" = \"$blob\"\n+'\n+\n test_done\n-- \n2.1.1.392.g062cc5d\n"},{"id":"249716","messageId":"541E9203.9040702@web.de","threadId":"37606","inReplyTo":"1411268449-14636-1-git-send-email-johan@herland.net","subject":"Re: [RFC/PATCH] notes: Allow adding empty notes with -C","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2014-09-21T08:53:23Z","receivedAt":"2014-09-21T08:53:23Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2014-09-21 05.00, Johan Herland wrote:\n[]\n> diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh\n> index cfd67ff..a6c399b 100755\n> --- a/t/t3301-notes.sh\n> +++ b/t/t3301-notes.sh\n> @@ -1239,4 +1239,23 @@ test_expect_success 'git notes get-ref (--ref)' '\n> \ttest \"$(GIT_NOTES_REF=refs/notes/bar git notes --ref=baz get-ref)\" = \"refs/notes/baz\"\n> '\n> \n> +cat > expect << EOF\nGit style for shell scripts: Plase put no space between < or > or >> and the file name:\ncat >expect < +commit 085b0d1309902c3148feb5a136515bdb9a1cd614\n> +Author: A U Thor \n> +Date: Thu Apr 7 15:28:13 2005 -0700\n> +\n> + 16th\n> +\n> +Notes (foo):\n> +EOF\n> +\n> +test_expect_success 'can create empty note with \"git notes add -C $empty_blob\"' '\n> +\ttest_commit 16th &&\n> +\tblob=e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 &&\n> +\tgit notes add -C $blob &&\n> +\tgit log -1 > actual &&\ngit log -1 >actual &&\n\n> +\ttest_cmp expect actual &&\n> +\ttest \"$(git notes list HEAD)\" = \"$blob\"\n> +'\n> +\n> test_done\n> \n"},{"id":"249719","messageId":"CALKQrgcP_G9G4+NACbSgjTG9kssgRJxbH-St0RKVVbHEHgA2tA@mail.gmail.com","threadId":"37606","inReplyTo":"541E9203.9040702@web.de","subject":"Re: [RFC/PATCH] notes: Allow adding empty notes with -C","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2014-09-21T09:46:23Z","receivedAt":"2014-09-21T09:46:23Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Sun, Sep 21, 2014 at 10:53 AM, Torsten Bögershausen wrote:\n> On 2014-09-21 05.00, Johan Herland wrote:\n\n[...]\n\n>> +cat > expect << EOF\n> Git style for shell scripts: Plase put no space between < or > or >> and the file name:\n> cat >expect <> + git log -1 > actual &&\n> git log -1 >actual &&\n\nOk, will fix (although the rest of the file leans towards the\nalternative style with space).\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"249731","messageId":"1A2394C0-50A4-40F4-B0B9-B2EC38109083@gmail.com","threadId":"37606","inReplyTo":"CALKQrgd9BPUTrgZvFCj_fznRG6RmfiGzW68XF++yykMguypTig@mail.gmail.com","subject":"Re: Bug/request: the empty string should be a valid git note","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2014-09-21T23:32:51Z","receivedAt":"2014-09-21T23:32:51Z","isPatch":false,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On Sep 20, 2014, at 18:44, Johan Herland wrote:\n\n> At least, we should fix\n>\n> git notes add -C e69de29bb2d1d6434b8b29ae775ad8c2e48c5391\n>\n> Whether we should also change\n>\n> git notes add -m ''\n>\n> to create an empty note, or leave it as-is, (i.e. similar in spirit to\n> \"git commit -m ''\"), I'll leave up to further discussion.\n\nThe help for git commit has this:\n\n --allow-empty-message\n Like --allow-empty this command is primarily for use by\n foreign SCM interface scripts. It allows you to create\n a commit with an empty commit message without using\n plumbing commands like git-commit-tree(1).\n\nWhy not add the same/similar option to git notes add?\n\nSo this:\n\n git notes add --allow-empty-message -m ''\n\ncreates an empty note. (Perhaps --allow-empty-note should\nbe an alias?)\n\nWith your patch to allow -C e69de29bb2d1d6434b8b29ae775ad8c2e48c5391\nthere's already support for it, it just needs the option\nparsing added. :)\n\n--Kyle\n"},{"id":"249741","messageId":"xmqqoau7lfm3.fsf@gitster.dls.corp.google.com","threadId":"37606","inReplyTo":"1A2394C0-50A4-40F4-B0B9-B2EC38109083@gmail.com","subject":"Re: Bug/request: the empty string should be a valid git note","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-22T17:39:00Z","receivedAt":"2014-09-22T17:39:00Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kyle J. McKay\" writes:\n\n> On Sep 20, 2014, at 18:44, Johan Herland wrote:\n>\n>> At least, we should fix\n>>\n>> git notes add -C e69de29bb2d1d6434b8b29ae775ad8c2e48c5391\n>>\n>> Whether we should also change\n>>\n>> git notes add -m ''\n>>\n>> to create an empty note, or leave it as-is, (i.e. similar in spirit to\n>> \"git commit -m ''\"), I'll leave up to further discussion.\n>\n> The help for git commit has this:\n>\n> --allow-empty-message\n> Like --allow-empty this command is primarily for use by\n> foreign SCM interface scripts. It allows you to create\n> a commit with an empty commit message without using\n> plumbing commands like git-commit-tree(1).\n>\n> Why not add the same/similar option to git notes add?\n\nSounds like a good direction to go.\n"}]}