[PATCHv2 1/3] builtin/notes: Fix premature failure when trying to add the empty blob

[PATCHv2 1/3] builtin/notes: Fix premature failure when trying to add the empty blob

[Johan Herland]

This fixes a small buglet when trying to explicitly add the empty blob
as a note object using the -c or -C option to git notes add/append.
Instead of failing with a nonsensical error message indicating that the
empty blob does not exist, we should rather behave as if an empty notes
message was given (e.g. using -m "" or -F /dev/null).

The next patch contains a test that verifies the fixed behavior.

Found-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Johan Herland <johan@herland.net>
---
 builtin/notes.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index 68b6cd8..9ee6816 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -266,7 +266,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);
 	}
-- 
2.0.0.rc4.501.gdaf83ca

[PATCHv2 2/3] t3312-notes-empty: Test that 'git notes' removes empty notes by default

[Johan Herland]

Add test cases documenting the current behavior when trying to
add/append/edit empty notes. This is in preparation for adding
--allow-empty; to allow empty notes to be stored.

Improved-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Johan Herland <johan@herland.net>
---
 t/t3312-notes-empty.sh | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)
 create mode 100755 t/t3312-notes-empty.sh

diff --git a/t/t3312-notes-empty.sh b/t/t3312-notes-empty.sh
new file mode 100755
index 0000000..44074f6
--- /dev/null
+++ b/t/t3312-notes-empty.sh
@@ -0,0 +1,48 @@
+#!/bin/sh
+
+test_description='Test adding/editing of empty notes'
+. ./test-lib.sh
+
+write_script fake_editor <<\EOF
+	echo "$MSG" >"$1"
+EOF
+GIT_EDITOR=./fake_editor
+export GIT_EDITOR
+
+test_expect_success 'setup' '
+	test_commit one &&
+	git log -1 >expect_missing &&
+	empty_blob=$(git hash-object -w /dev/null)
+'
+
+cleanup_notes() {
+	git update-ref -d refs/notes/commits
+}
+
+verify_missing() {
+	git log -1 > actual &&
+	test_cmp expect_missing actual &&
+	! git notes list HEAD
+}
+
+for cmd in \
+	'add' \
+	'add -F /dev/null' \
+	'add -m ""' \
+	'add -c "$empty_blob"' \
+	'add -C "$empty_blob"' \
+	'append' \
+	'append -F /dev/null' \
+	'append -m ""' \
+	'append -c "$empty_blob"' \
+	'append -C "$empty_blob"' \
+	'edit'
+do
+	test_expect_success "'git notes $cmd' removes empty note" "
+		cleanup_notes &&
+		MSG= git notes $cmd &&
+		verify_missing
+	"
+done
+
+test_done
-- 
2.0.0.rc4.501.gdaf83ca

[PATCHv2 3/3] notes: Add --allow-empty, to allow storing empty notes

[Johan Herland]

Although the "git notes" man page advertises that we support binary-safe
notes addition (using 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 the --allow-empty option to the add/append/edit subcommands,
to explicitly allow an empty note to be stored into the notes tree.

Also update the documentation, and add test cases for the new option.

Reported-by: James H. Fisher <jhf@trifork.com>
Improved-by: Kyle J. McKay <mackyle@gmail.com>
Signed-off-by: Johan Herland <johan@herland.net>
---
 Documentation/git-notes.txt | 12 ++++++++----
 builtin/notes.c             | 23 ++++++++++++++---------
 notes.c                     |  3 +--
 t/t3312-notes-empty.sh      | 17 ++++++++++++++++-
 4 files changed, 39 insertions(+), 16 deletions(-)

diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
index 310f0a5..851518d 100644
--- a/Documentation/git-notes.txt
+++ b/Documentation/git-notes.txt
@@ -9,10 +9,10 @@ SYNOPSIS
 --------
 [verse]
 'git notes' [list [<object>]]
-'git notes' add [-f] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>]
+'git notes' add [-f] [--allow-empty] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>]
 'git notes' copy [-f] ( --stdin | <from-object> <to-object> )
-'git notes' append [-F <file> | -m <msg> | (-c | -C) <object>] [<object>]
-'git notes' edit [<object>]
+'git notes' append [--allow-empty] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>]
+'git notes' edit [--allow-empty] [<object>]
 'git notes' show [<object>]
 'git notes' merge [-v | -q] [-s <strategy> ] <notes-ref>
 'git notes' merge --commit [-v | -q]
@@ -155,6 +155,10 @@ OPTIONS
 	Like '-C', but with '-c' the editor is invoked, so that
 	the user can further edit the note message.
 
+--allow-empty::
+	Allow an empty note object to be stored. The default behavior is
+	to automatically remove empty notes.
+
 --ref <ref>::
 	Manipulate the notes tree in <ref>.  This overrides
 	'GIT_NOTES_REF' and the "core.notesRef" configuration.  The ref
@@ -287,7 +291,7 @@ arbitrary files using 'git hash-object':
 ------------
 $ cc *.c
 $ blob=$(git hash-object -w a.out)
-$ git notes --ref=built add -C "$blob" HEAD
+$ git notes --ref=built add --allow-empty -C "$blob" HEAD
 ------------
 
 (You cannot simply use `git notes --ref=built add -F a.out HEAD`
diff --git a/builtin/notes.c b/builtin/notes.c
index 9ee6816..038a419 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -22,10 +22,10 @@
 
 static const char * const git_notes_usage[] = {
 	N_("git notes [--ref <notes_ref>] [list [<object>]]"),
-	N_("git notes [--ref <notes_ref>] add [-f] [-m <msg> | -F <file> | (-c | -C) <object>] [<object>]"),
+	N_("git notes [--ref <notes_ref>] add [-f] [--allow-empty] [-m <msg> | -F <file> | (-c | -C) <object>] [<object>]"),
 	N_("git notes [--ref <notes_ref>] copy [-f] <from-object> <to-object>"),
-	N_("git notes [--ref <notes_ref>] append [-m <msg> | -F <file> | (-c | -C) <object>] [<object>]"),
-	N_("git notes [--ref <notes_ref>] edit [<object>]"),
+	N_("git notes [--ref <notes_ref>] append [--allow-empty] [-m <msg> | -F <file> | (-c | -C) <object>] [<object>]"),
+	N_("git notes [--ref <notes_ref>] edit [--allow-empty] [<object>]"),
 	N_("git notes [--ref <notes_ref>] show [<object>]"),
 	N_("git notes [--ref <notes_ref>] merge [-v | -q] [-s <strategy> ] <notes_ref>"),
 	N_("git notes merge --commit [-v | -q]"),
@@ -150,8 +150,8 @@ static void write_commented_object(int fd, const unsigned char *object)
 }
 
 static void create_note(const unsigned char *object, struct msg_arg *msg,
-			int append_only, const unsigned char *prev,
-			unsigned char *result)
+			int append_only, int allow_empty,
+			const unsigned char *prev, unsigned char *result)
 {
 	char *path = NULL;
 
@@ -202,7 +202,7 @@ static void create_note(const unsigned char *object, struct msg_arg *msg,
 		free(prev_buf);
 	}
 
-	if (!msg->buf.len) {
+	if (!allow_empty && !msg->buf.len) {
 		fprintf(stderr, _("Removing note for object %s\n"),
 			sha1_to_hex(object));
 		hashclr(result);
@@ -397,7 +397,7 @@ static int append_edit(int argc, const char **argv, const char *prefix);
 
 static int add(int argc, const char **argv, const char *prefix)
 {
-	int retval = 0, force = 0;
+	int retval = 0, force = 0, allow_empty = 0;
 	const char *object_ref;
 	struct notes_tree *t;
 	unsigned char object[20], new_note[20];
@@ -417,6 +417,8 @@ static int add(int argc, const char **argv, const char *prefix)
 		{ OPTION_CALLBACK, 'C', "reuse-message", &msg, N_("object"),
 			N_("reuse specified note object"), PARSE_OPT_NONEG,
 			parse_reuse_arg},
+		OPT_BOOL(0, "allow-empty", &allow_empty,
+			N_("allow storing empty note")),
 		OPT__FORCE(&force, N_("replace existing notes")),
 		OPT_END()
 	};
@@ -460,7 +462,7 @@ static int add(int argc, const char **argv, const char *prefix)
 			sha1_to_hex(object));
 	}
 
-	create_note(object, &msg, 0, note, new_note);
+	create_note(object, &msg, 0, allow_empty, note, new_note);
 
 	if (is_null_sha1(new_note))
 		remove_note(t, object);
@@ -554,6 +556,7 @@ out:
 
 static int append_edit(int argc, const char **argv, const char *prefix)
 {
+	int allow_empty = 0;
 	const char *object_ref;
 	struct notes_tree *t;
 	unsigned char object[20], new_note[20];
@@ -574,6 +577,8 @@ static int append_edit(int argc, const char **argv, const char *prefix)
 		{ OPTION_CALLBACK, 'C', "reuse-message", &msg, N_("object"),
 			N_("reuse specified note object"), PARSE_OPT_NONEG,
 			parse_reuse_arg},
+		OPT_BOOL(0, "allow-empty", &allow_empty,
+			N_("allow storing empty note")),
 		OPT_END()
 	};
 	int edit = !strcmp(argv[0], "edit");
@@ -600,7 +605,7 @@ static int append_edit(int argc, const char **argv, const char *prefix)
 	t = init_notes_check(argv[0]);
 	note = get_note(t, object);
 
-	create_note(object, &msg, !edit, note, new_note);
+	create_note(object, &msg, !edit, allow_empty, note, new_note);
 
 	if (is_null_sha1(new_note))
 		remove_note(t, object);
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/t3312-notes-empty.sh b/t/t3312-notes-empty.sh
index 44074f6..6fce25f 100755
--- a/t/t3312-notes-empty.sh
+++ b/t/t3312-notes-empty.sh
@@ -1,6 +1,6 @@
 #!/bin/sh
 
-test_description='Test adding/editing of empty notes'
+test_description='Test adding/editing of empty notes with/without --allow-empty'
 . ./test-lib.sh
 
 write_script fake_editor <<\EOF
@@ -12,6 +12,9 @@ export GIT_EDITOR
 test_expect_success 'setup' '
 	test_commit one &&
 	git log -1 >expect_missing &&
+	cp expect_missing expect_empty &&
+	echo >>expect_empty &&
+	echo "Notes:" >>expect_empty &&
 	empty_blob=$(git hash-object -w /dev/null)
 '
 
@@ -25,6 +28,12 @@ verify_missing() {
 	! git notes list HEAD
 }
 
+verify_empty() {
+	git log -1 > actual &&
+	test_cmp expect_empty actual &&
+	test "$(git notes list HEAD)" = "$empty_blob"
+}
+
 for cmd in \
 	'add' \
 	'add -F /dev/null' \
@@ -43,6 +52,12 @@ do
 		MSG= git notes $cmd &&
 		verify_missing
 	"
+
+	test_expect_success "'git notes $cmd --allow-empty' stores empty note" "
+		cleanup_notes &&
+		MSG= git notes $cmd --allow-empty &&
+		verify_empty
+	"
 done
 
 test_done
-- 
2.0.0.rc4.501.gdaf83ca

Re: [PATCHv2 2/3] t3312-notes-empty: Test that 'git notes' removes empty notes by default

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> Add test cases documenting the current behavior when trying to
> add/append/edit empty notes. This is in preparation for adding
> --allow-empty; to allow empty notes to be stored.
>
> Improved-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Johan Herland <johan@herland.net>
> ---
>  t/t3312-notes-empty.sh | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
>  create mode 100755 t/t3312-notes-empty.sh
>
> diff --git a/t/t3312-notes-empty.sh b/t/t3312-notes-empty.sh
> new file mode 100755
> index 0000000..44074f6
> --- /dev/null
> +++ b/t/t3312-notes-empty.sh
> @@ -0,0 +1,48 @@
> +#!/bin/sh
> +
> +test_description='Test adding/editing of empty notes'
> +. ./test-lib.sh
> +
> +write_script fake_editor <<\EOF
> +	echo "$MSG" >"$1"
> +EOF
> +GIT_EDITOR=./fake_editor
> +export GIT_EDITOR
> +
> +test_expect_success 'setup' '
> +	test_commit one &&
> +	git log -1 >expect_missing &&
> +	empty_blob=$(git hash-object -w /dev/null)
> +'

> +cleanup_notes() {
> +	git update-ref -d refs/notes/commits
> +}
> +
> +verify_missing() {
> +	git log -1 > actual &&

Hmph, it was unclear what exactly you are trying to check with this
one and the other "git log -1 >expect_missing".

Perhaps a comment that says "We are interested in the trailing
'Notes: ...' in the output" is necessary here, or (probably even
better) use the --format='%N' to make it crystal clear?

> +	test_cmp expect_missing actual &&
> +	! git notes list HEAD

Isn't this test_must_fail (i.e. if somebody screws up to make "git
notes list" segfault, the test should fail, not taking the dying
with SEGV as a sign of success)?

> +}
> +
> +for cmd in \
> +	'add' \
> +	'add -F /dev/null' \
> +	'add -m ""' \
> +	'add -c "$empty_blob"' \
> +	'add -C "$empty_blob"' \
> +	'append' \
> +	'append -F /dev/null' \
> +	'append -m ""' \
> +	'append -c "$empty_blob"' \
> +	'append -C "$empty_blob"' \
> +	'edit'
> +do
> +	test_expect_success "'git notes $cmd' removes empty note" "
> +		cleanup_notes &&
> +		MSG= git notes $cmd &&
> +		verify_missing
> +	"
> +done

Perhaps just a taste issue, but I would think

	while read cmd
        do
        	... that test eval with $cmd interpolation ...
	done <<-\EOF
        add
        add -F /dev/null
        ...
        EOF

would be easier to maintain and to read, without having to worry
about quoting and backslashing.

> +
> +test_done

Re: [PATCHv2 2/3] t3312-notes-empty: Test that 'git notes' removes empty notes by default

[Johan Herland]

On Wed, Nov 5, 2014 at 8:00 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Johan Herland <johan@herland.net> writes:
>> +verify_missing() {
>> +     git log -1 > actual &&
>
> Hmph, it was unclear what exactly you are trying to check with this
> one and the other "git log -1 >expect_missing".
>
> Perhaps a comment that says "We are interested in the trailing
> 'Notes: ...' in the output" is necessary here, or (probably even
> better) use the --format='%N' to make it crystal clear?

I'm rewriting the tests not to use 'git log' at all. We already have
other tests that check the formatting of notes, so we really only need
to use "git notes list HEAD" in these tests.

>> +     test_cmp expect_missing actual &&
>> +     ! git notes list HEAD
>
> Isn't this test_must_fail (i.e. if somebody screws up to make "git
> notes list" segfault, the test should fail, not taking the dying
> with SEGV as a sign of success)?

Yes, will fix.

>> +}
>> +
>> +for cmd in \
>> +     'add' \
>> +     'add -F /dev/null' \
>> +     'add -m ""' \
>> +     'add -c "$empty_blob"' \
>> +     'add -C "$empty_blob"' \
>> +     'append' \
>> +     'append -F /dev/null' \
>> +     'append -m ""' \
>> +     'append -c "$empty_blob"' \
>> +     'append -C "$empty_blob"' \
>> +     'edit'
>> +do
>> +     test_expect_success "'git notes $cmd' removes empty note" "
>> +             cleanup_notes &&
>> +             MSG= git notes $cmd &&
>> +             verify_missing
>> +     "
>> +done
>
> Perhaps just a taste issue, but I would think
>
>         while read cmd
>         do
>                 ... that test eval with $cmd interpolation ...
>         done <<-\EOF
>         add
>         add -F /dev/null
>         ...
>         EOF
>
> would be easier to maintain and to read, without having to worry
> about quoting and backslashing.

Thanks. That's much easier to read. Will fix.

On Wed, Nov 5, 2014 at 7:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
> By definition, an empty note is empty ;-) and devoid of useful
> information other than a single bit, its existence.  I would
> understand a handful of tests that check "oh by the way, we should
> also handle empty ones sensibly", but are you sure that a _new_
> separate test script, not addition to existing test script, is worth
> to check _only_ empty notes?

Will move the tests into t3301 in the re-roll.


...Johan

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