threads / patch / 37880

patch, 2 partsnotes: Add --allow-empty, to allow storing empty notes

Subject: [PATCH 2/2] notes: Add --allow-empty, to allow storing empty notes

## tl;dr

5 messages between Nov 5, 2014 and Nov 5, 2014. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

Johan Herland· Nov 5, 2014, 01:32 UTC · lore

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

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.

Signed-off-by: Johan Herland <johan@herland.net>
---
 t/t3312-notes-empty.sh | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)
 create mode 100755 t/t3312-notes-empty.sh
Show changes to t/t3312-notes-empty.sh +58 −0
diff --git a/t/t3312-notes-empty.sh b/t/t3312-notes-empty.sh
new file mode 100755
index 0000000..2806d27
--- /dev/null
+++ b/t/t3312-notes-empty.sh
@@ -0,0 +1,58 @@
+#!/bin/sh
+
+test_description='Test adding/editing of empty notes'
+. ./test-lib.sh
+
+cat >fake_editor.sh <<\EOF
+#!/bin/sh
+echo "$MSG" >"$1"
+echo "$MSG" >& 2
+EOF
+chmod a+x fake_editor.sh
+GIT_EDITOR=./fake_editor.sh
+export GIT_EDITOR
+
+test_expect_success 'setup' '
+	test_commit one &&
+	empty_blob=$(git hash-object -w /dev/null)
+'
+
+cleanup_notes() {
+	git update-ref -d refs/notes/commits
+}
+
+cat >expect_missing <<\EOF
+commit d79ce1670bdcb76e6d1da2ae095e890ccb326ae9
+Author: A U Thor <author@example.com>
+Date:   Thu Apr 7 15:13:13 2005 -0700
+
+    one
+EOF
+
+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
Johan Herland· Nov 5, 2014, 01:32 UTC · re: Johan Herland · lore

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             | 25 +++++++++++++++----------
 notes.c                     |  3 +--
 t/t3312-notes-empty.sh      | 20 +++++++++++++++++++-
 4 files changed, 43 insertions(+), 17 deletions(-)
Show changes to 4 files +43 −17

Documentation/git-notes.txt, builtin/notes.c, notes.c, t/t3312-notes-empty.sh

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 68b6cd8..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);
@@ -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);
 	}
@@ -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 2806d27..f89fbc9 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
 
 cat >fake_editor.sh <<\EOF
@@ -35,6 +35,18 @@ verify_missing() {
 	! git notes list HEAD
 }
 
+cp expect_missing expect_empty
+cat >>expect_empty <<\EOF
+
+Notes:
+EOF
+
+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' \
@@ -53,6 +65,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
Eric Sunshine· Nov 5, 2014, 04:10 UTC · re: Johan Herland · lore

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

On Tue, Nov 4, 2014 at 8:32 PM, Johan Herland <johan@herland.net> wrote:
Show 27 quoted lines
> 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.
>
> Signed-off-by: Johan Herland <johan@herland.net>
> ---
>  t/t3312-notes-empty.sh | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 58 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..2806d27
> --- /dev/null
> +++ b/t/t3312-notes-empty.sh
> @@ -0,0 +1,58 @@
> +#!/bin/sh
> +
> +test_description='Test adding/editing of empty notes'
> +. ./test-lib.sh
> +
> +cat >fake_editor.sh <<\EOF
> +#!/bin/sh
> +echo "$MSG" >"$1"
> +echo "$MSG" >& 2
> +EOF
> +chmod a+x fake_editor.sh
write_script() would allow you to drop the #!/bin/sh and chmod lines.
Show 19 quoted lines
> +GIT_EDITOR=./fake_editor.sh
> +export GIT_EDITOR
> +
> +test_expect_success 'setup' '
> +       test_commit one &&
> +       empty_blob=$(git hash-object -w /dev/null)
> +'
> +
> +cleanup_notes() {
> +       git update-ref -d refs/notes/commits
> +}
> +
> +cat >expect_missing <<\EOF
> +commit d79ce1670bdcb76e6d1da2ae095e890ccb326ae9
> +Author: A U Thor <author@example.com>
> +Date:   Thu Apr 7 15:13:13 2005 -0700
> +
> +    one
> +EOF

Rather than hard-coding this output, generating it would make the test script less fragile:

    git log -1 >expect_missing
Show 25 quoted lines
> +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
Each -c/-C case fails for me when trying to read $empty_object. For example:

fatal: Failed to read object 'e69de29bb2d1d6434b8b29ae775ad8c2e48c5391'. not ok 5 - 'git notes add -c "$empty_blob"' removes empty note

> +
> +test_done
> --
> 2.0.0.rc4.501.gdaf83ca
Johan Herland· Nov 5, 2014, 08:32 UTC · re: Eric Sunshine · lore

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

On Wed, Nov 5, 2014 at 5:10 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
[...]
> write_script() would allow you to drop the #!/bin/sh and chmod lines.
[...]
> Rather than hard-coding this output, generating it would make the test
> script less fragile:
>
>     git log -1 >expect_missing
[...]
> Each -c/-C case fails for me when trying to read $empty_object. For example:
>
> fatal: Failed to read object 'e69de29bb2d1d6434b8b29ae775ad8c2e48c5391'.
> not ok 5 - 'git notes add -c "$empty_blob"' removes empty note
These are all fixed in the re-roll.
Thanks for the feedback!
...Johan
-- 
Johan Herland, <johan@herland.net>
www.herland.net
Junio C Hamano· Nov 5, 2014, 18:36 UTC · re: Johan Herland · lore

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

Johan Herland <johan@herland.net> writes:
Show 9 quoted lines
> 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.
>
> Signed-off-by: Johan Herland <johan@herland.net>
> ---
>  t/t3312-notes-empty.sh | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 58 insertions(+)
>  create mode 100755 t/t3312-notes-empty.sh

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?

Show 64 quoted lines
> diff --git a/t/t3312-notes-empty.sh b/t/t3312-notes-empty.sh
> new file mode 100755
> index 0000000..2806d27
> --- /dev/null
> +++ b/t/t3312-notes-empty.sh
> @@ -0,0 +1,58 @@
> +#!/bin/sh
> +
> +test_description='Test adding/editing of empty notes'
> +. ./test-lib.sh
> +
> +cat >fake_editor.sh <<\EOF
> +#!/bin/sh
> +echo "$MSG" >"$1"
> +echo "$MSG" >& 2
> +EOF
> +chmod a+x fake_editor.sh
> +GIT_EDITOR=./fake_editor.sh
> +export GIT_EDITOR
> +
> +test_expect_success 'setup' '
> +	test_commit one &&
> +	empty_blob=$(git hash-object -w /dev/null)
> +'
> +
> +cleanup_notes() {
> +	git update-ref -d refs/notes/commits
> +}
> +
> +cat >expect_missing <<\EOF
> +commit d79ce1670bdcb76e6d1da2ae095e890ccb326ae9
> +Author: A U Thor <author@example.com>
> +Date:   Thu Apr 7 15:13:13 2005 -0700
> +
> +    one
> +EOF
> +
> +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

← back to recent threads