Bug/request: the empty string should be a valid git note

Bug/request: the empty string should be a valid git note

[James H. Fisher]

In the documentation for git notes [1] I read:

    In principle, a note is a regular Git blob, and any kind of (non-)format is accepted.

Then, since the empty string is a valid regular Git blob, the empty string is also a valid git note.

Therefore this behavior was unexpected for me:

    > git notes --ref=foo add -m ''
    Removing note for object 97b8860c071898d9e162678ea1035a8ced2f8b1f

I was surprised to see that this behavior was deliberately introduced:

    > git log -1 a0b4dfa
    commit a0b4dfa9b35a2ebac578ea5547b041bb78557238
    Author: Johan Herland <johan@herland.net>
    Date:   Sat Feb 13 22:28:24 2010 +0100

        Teach builtin-notes to remove empty notes

        When the result of editing a note is an empty string, the associated note
        entry should be deleted from the notes tree.

        This allows deleting notes by invoking either "git notes -m ''" or
        "git notes -F /dev/null".

        Signed-off-by: Johan Herland <johan@herland.net>
        Signed-off-by: Junio C Hamano <gitster@pobox.com>

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.

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.)

Does anyone know why we have the existing behavior? Is it for "technical reasons” or was it actually considered desirable?

James Fisher 

[1]: https://www.kernel.org/pub/software/scm/git/docs/git-notes.html

Re: Bug/request: the empty string should be a valid git note

[James H. Fisher]

Apologies for re-sending; unclear whether my email was delivered since I sent it before my subscription was confirmed.

On 20 Sep 2014, at 20:47, James H. Fisher <jhf@trifork.com> wrote:

> In the documentation for git notes [1] I read:
> 
>    In principle, a note is a regular Git blob, and any kind of (non-)format is accepted.
> 
> Then, since the empty string is a valid regular Git blob, the empty string is also a valid git note.
> 
> Therefore this behavior was unexpected for me:
> 
>> git notes --ref=foo add -m ''
>    Removing note for object 97b8860c071898d9e162678ea1035a8ced2f8b1f
> 
> I was surprised to see that this behavior was deliberately introduced:
> 
>> git log -1 a0b4dfa
>    commit a0b4dfa9b35a2ebac578ea5547b041bb78557238
>    Author: Johan Herland <johan@herland.net>
>    Date:   Sat Feb 13 22:28:24 2010 +0100
> 
>        Teach builtin-notes to remove empty notes
> 
>        When the result of editing a note is an empty string, the associated note
>        entry should be deleted from the notes tree.
> 
>        This allows deleting notes by invoking either "git notes -m ''" or
>        "git notes -F /dev/null".
> 
>        Signed-off-by: Johan Herland <johan@herland.net>
>        Signed-off-by: Junio C Hamano <gitster@pobox.com>
> 
> 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.
> 
> 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.)
> 
> Does anyone know why we have the existing behavior? Is it for "technical reasons” or was it actually considered desirable?
> 
> James Fisher 
> 
> [1]: https://www.kernel.org/pub/software/scm/git/docs/git-notes.html

Re: Bug/request: the empty string should be a valid git note

[Johan Herland]

On Sat, Sep 20, 2014 at 9:47 PM, James H. Fisher <jhf@trifork.com> wrote:
> In the documentation for git notes [1] I read:
>
>     In principle, a note is a regular Git blob, and any kind of
>     (non-)format is accepted.
>
> Then, since the empty string is a valid regular Git blob, the empty
> string is also a valid git note.

Agreed.

> Therefore this behavior was unexpected for me:
>
>     > git notes --ref=foo add -m ''
>     Removing note for object 97b8860c071898d9e162678ea1035a8ced2f8b1f

Although I can understand your surprise, if you keep reading the
sentences following the above quote from the manual page, it says:

   You can binary-safely create notes from arbitrary files using
    git hash-object:

        $ cc *.c
        $ blob=$(git hash-object -w a.out)
        $ git notes --ref=built add -C "$blob" HEAD

This might indicate that even if "git notes add -m ''" does not create
an empty note, then at least the following command should:

    git notes add -C e69de29bb2d1d6434b8b29ae775ad8c2e48c5391

(that is the SHA1 sum of the empty blob)

However (and this is where I start to grow surprised as well), that
_also_ totally fails to create an empty note!

This is IMHO clearly a bug: If we advertise a binary-safe method for
creating git-notes, then that method should definitely also support
creating empty notes.

Whether or not "git notes add -m ''" should also create an empty note,
is not necessarily that clear to me. There are historical reasons for
the current behavior (see below), but there is also a certain element
of convenience for the user here; consider the following:

When you run "git commit", git opens your editor to let you write the
commit message. However, if you change your mind, and want to abort
the commit, you can simply write nothing (or exit without saving),
and git will respond with:

    Aborting commit due to empty commit message.

The same applies to "git commit -m ''".

This is the pattern on which "git notes add" is modeled: Empty input
is interpreted as an intent to abort, which, in the notes case is to
simply not create the note at all.

> I was surprised to see that this behavior was deliberately introduced:
>
>     > git log -1 a0b4dfa
>     commit a0b4dfa9b35a2ebac578ea5547b041bb78557238
>     Author: Johan Herland <johan@herland.net>
>     Date:   Sat Feb 13 22:28:24 2010 +0100
>
>         Teach builtin-notes to remove empty notes
>
>         When the result of editing a note is an empty string, the
>         associated note entry should be deleted from the notes tree.
>
>         This allows deleting notes by invoking either "git notes -m ''"
>         or "git notes -F /dev/null".
>
>         Signed-off-by: Johan Herland <johan@herland.net>
>         Signed-off-by: Junio C Hamano <gitster@pobox.com>
>
> 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.

Actually, if you take a closer look at the context for that commit,
you will see that things were in fact the other way around: auto-
removing empty notes was added _before_ "git notes remove", and if
you look at 92b3385f which introduces "git notes remove", you'll see
that it was initially implemented wholly in terms of editing the note
to become empty, which then caused its removal.

That said, these are unimportant historical details, and should not
serve as justification for continued incorrect behavior.

> 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.)

I do not disagre with your use case at all, and it is regrettable that
there is no simple workaround for creating empty notes. Assuming that
your lists of files use e.g. a newline to separate filenames, then I
guess you could use a note consisting of a single newline to signify
an empty list ("echo | git hash-object -w --stdin"). At least as a
temporary workaround until we get this fixed in Git.

> Does anyone know why we have the existing behavior? Is it for
> "technical reasons” or was it actually considered desirable?

I guess it's "historical reasons". But we should definitely fix it.

At least, we should fix

    git notes add -C e69de29bb2d1d6434b8b29ae775ad8c2e48c5391

Whether we should also change

    git notes add -m ''

to create an empty note, or leave it as-is, (i.e. similar in spirit to
"git commit -m ''"), I'll leave up to further discussion.


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

[RFC/PATCH] notes: Allow adding empty notes with -C

[Johan Herland]

Although the "git notes" man page advertises that we support binary-safe
notes addition with the -C option, we currently do not support adding the
empty note (i.e. using the empty blob to annotate an object). Instead,
an empty note is always treated as an intent to remove the note
altogether.

Introduce a flag to builtin/notes which indicates whether or not to
remove an empty note, and disable that flag for the -C option (leave it
enabled to preserve the current behavior for all other options).

Also add a test case that verifies that we can not indeed add empty notes
with "git notes add -C $empty_blob".

Reported-by: James H. Fisher <jhf@trifork.com>
Signed-off-by: Johan Herland <johan@herland.net>
---
 builtin/notes.c  | 16 +++++++++++-----
 notes.c          |  3 +--
 t/t3301-notes.sh | 19 +++++++++++++++++++
 3 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index 67d0bb1..f8ff590 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -95,6 +95,7 @@ static const char note_template[] =
 struct msg_arg {
 	int given;
 	int use_editor;
+	int remove_if_empty;
 	struct strbuf buf;
 };
 
@@ -202,7 +203,7 @@ static void create_note(const unsigned char *object, struct msg_arg *msg,
 		free(prev_buf);
 	}
 
-	if (!msg->buf.len) {
+	if (msg->remove_if_empty && !msg->buf.len) {
 		fprintf(stderr, _("Removing note for object %s\n"),
 			sha1_to_hex(object));
 		hashclr(result);
@@ -233,6 +234,7 @@ static int parse_msg_arg(const struct option *opt, const char *arg, int unset)
 	stripspace(&(msg->buf), 0);
 
 	msg->given = 1;
+	msg->remove_if_empty = 1;
 	return 0;
 }
 
@@ -250,6 +252,7 @@ static int parse_file_arg(const struct option *opt, const char *arg, int unset)
 	stripspace(&(msg->buf), 0);
 
 	msg->given = 1;
+	msg->remove_if_empty = 1;
 	return 0;
 }
 
@@ -266,7 +269,7 @@ static int parse_reuse_arg(const struct option *opt, const char *arg, int unset)
 
 	if (get_sha1(arg, object))
 		die(_("Failed to resolve '%s' as a valid ref."), arg);
-	if (!(buf = read_sha1_file(object, &type, &len)) || !len) {
+	if (!(buf = read_sha1_file(object, &type, &len))) {
 		free(buf);
 		die(_("Failed to read object '%s'."), arg);
 	}
@@ -278,14 +281,17 @@ static int parse_reuse_arg(const struct option *opt, const char *arg, int unset)
 	free(buf);
 
 	msg->given = 1;
+	msg->remove_if_empty = 0;
 	return 0;
 }
 
 static int parse_reedit_arg(const struct option *opt, const char *arg, int unset)
 {
+	int ret = parse_reuse_arg(opt, arg, unset);
 	struct msg_arg *msg = opt->value;
 	msg->use_editor = 1;
-	return parse_reuse_arg(opt, arg, unset);
+	msg->remove_if_empty = 1;
+	return ret;
 }
 
 static int notes_copy_from_stdin(int force, const char *rewrite_cmd)
@@ -403,7 +409,7 @@ static int add(int argc, const char **argv, const char *prefix)
 	unsigned char object[20], new_note[20];
 	char logmsg[100];
 	const unsigned char *note;
-	struct msg_arg msg = { 0, 0, STRBUF_INIT };
+	struct msg_arg msg = { 0, 0, 1, STRBUF_INIT };
 	struct option options[] = {
 		{ OPTION_CALLBACK, 'm', "message", &msg, N_("message"),
 			N_("note contents as a string"), PARSE_OPT_NONEG,
@@ -560,7 +566,7 @@ static int append_edit(int argc, const char **argv, const char *prefix)
 	const unsigned char *note;
 	char logmsg[100];
 	const char * const *usage;
-	struct msg_arg msg = { 0, 0, STRBUF_INIT };
+	struct msg_arg msg = { 0, 0, 1, STRBUF_INIT };
 	struct option options[] = {
 		{ OPTION_CALLBACK, 'm', "message", &msg, N_("message"),
 			N_("note contents as a string"), PARSE_OPT_NONEG,
diff --git a/notes.c b/notes.c
index 5fe691d..62bc6e1 100644
--- a/notes.c
+++ b/notes.c
@@ -1218,8 +1218,7 @@ static void format_note(struct notes_tree *t, const unsigned char *object_sha1,
 	if (!sha1)
 		return;
 
-	if (!(msg = read_sha1_file(sha1, &type, &msglen)) || !msglen ||
-			type != OBJ_BLOB) {
+	if (!(msg = read_sha1_file(sha1, &type, &msglen)) || type != OBJ_BLOB) {
 		free(msg);
 		return;
 	}
diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index cfd67ff..a6c399b 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -1239,4 +1239,23 @@ test_expect_success 'git notes get-ref (--ref)' '
 	test "$(GIT_NOTES_REF=refs/notes/bar git notes --ref=baz get-ref)" = "refs/notes/baz"
 '
 
+cat > expect << EOF
+commit 085b0d1309902c3148feb5a136515bdb9a1cd614
+Author: A U Thor <author@example.com>
+Date:   Thu Apr 7 15:28:13 2005 -0700
+
+    16th
+
+Notes (foo):
+EOF
+
+test_expect_success 'can create empty note with "git notes add -C $empty_blob"' '
+	test_commit 16th &&
+	blob=e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 &&
+	git notes add -C $blob &&
+	git log -1 > actual &&
+	test_cmp expect actual &&
+	test "$(git notes list HEAD)" = "$blob"
+'
+
 test_done
-- 
2.1.1.392.g062cc5d

Re: [RFC/PATCH] notes: Allow adding empty notes with -C

[Torsten Bögershausen]

On 2014-09-21 05.00, Johan Herland wrote:
[]
> diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
> index cfd67ff..a6c399b 100755
> --- a/t/t3301-notes.sh
> +++ b/t/t3301-notes.sh
> @@ -1239,4 +1239,23 @@ test_expect_success 'git notes get-ref (--ref)' '
>  	test "$(GIT_NOTES_REF=refs/notes/bar git notes --ref=baz get-ref)" = "refs/notes/baz"
>  '
>  
> +cat > expect << EOF
Git style for shell scripts: Plase put no space between < or > or >> and the file name:
cat >expect <<EOF

> +commit 085b0d1309902c3148feb5a136515bdb9a1cd614
> +Author: A U Thor <author@example.com>
> +Date:   Thu Apr 7 15:28:13 2005 -0700
> +
> +    16th
> +
> +Notes (foo):
> +EOF
> +
> +test_expect_success 'can create empty note with "git notes add -C $empty_blob"' '
> +	test_commit 16th &&
> +	blob=e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 &&
> +	git notes add -C $blob &&
> +	git log -1 > actual &&
git log -1 >actual &&

> +	test_cmp expect actual &&
> +	test "$(git notes list HEAD)" = "$blob"
> +'
> +
>  test_done
> 

Re: [RFC/PATCH] notes: Allow adding empty notes with -C

[Johan Herland]

On Sun, Sep 21, 2014 at 10:53 AM, Torsten Bögershausen <tboegi@web.de> wrote:
> On 2014-09-21 05.00, Johan Herland wrote:

[...]

>> +cat > expect << EOF
> Git style for shell scripts: Plase put no space between < or > or >> and the file name:
> cat >expect <<EOF

[...]

>> +     git log -1 > actual &&
> git log -1 >actual &&

Ok, will fix (although the rest of the file leans towards the
alternative style with space).


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: Bug/request: the empty string should be a valid git note

[Kyle J. McKay]

On Sep 20, 2014, at 18:44, Johan Herland wrote:

> At least, we should fix
>
>    git notes add -C e69de29bb2d1d6434b8b29ae775ad8c2e48c5391
>
> Whether we should also change
>
>    git notes add -m ''
>
> to create an empty note, or leave it as-is, (i.e. similar in spirit to
> "git commit -m ''"), I'll leave up to further discussion.

The help for git commit has this:

   --allow-empty-message
     Like --allow-empty this command is primarily for use by
     foreign SCM interface scripts. It allows you to create
     a commit with an empty commit message without using
     plumbing commands like git-commit-tree(1).

Why not add the same/similar option to git notes add?

So this:

   git notes add --allow-empty-message -m ''

creates an empty note.  (Perhaps --allow-empty-note should
be an alias?)

With your patch to allow -C e69de29bb2d1d6434b8b29ae775ad8c2e48c5391
there's already support for it, it just needs the option
parsing added.  :)

--Kyle

Re: Bug/request: the empty string should be a valid git note

[Junio C Hamano]

"Kyle J. McKay" <mackyle@gmail.com> writes:

> On Sep 20, 2014, at 18:44, Johan Herland wrote:
>
>> At least, we should fix
>>
>>    git notes add -C e69de29bb2d1d6434b8b29ae775ad8c2e48c5391
>>
>> Whether we should also change
>>
>>    git notes add -m ''
>>
>> to create an empty note, or leave it as-is, (i.e. similar in spirit to
>> "git commit -m ''"), I'll leave up to further discussion.
>
> The help for git commit has this:
>
>   --allow-empty-message
>     Like --allow-empty this command is primarily for use by
>     foreign SCM interface scripts. It allows you to create
>     a commit with an empty commit message without using
>     plumbing commands like git-commit-tree(1).
>
> Why not add the same/similar option to git notes add?

Sounds like a good direction to go.