threads / discuss / 37881

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

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

## tl;dr

5 messages between Nov 5, 2014 and Nov 7, 2014.

replies: 4people: 2as markdown or json

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

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

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
Johan Herland· Nov 5, 2014, 08:32 UTC · re: Johan Herland · lore

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
Junio C Hamano· Nov 5, 2014, 19:00 UTC · re: Johan Herland · lore

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

Johan Herland <johan@herland.net> writes:
Show 33 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.
>
> 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)
> +'
Show 6 quoted lines
> +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)?

Show 21 quoted lines
> +}
> +
> +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
Johan Herland· Nov 7, 2014, 09:09 UTC · re: Junio C Hamano · lore

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

On Wed, Nov 5, 2014 at 8:00 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 10 quoted lines
> 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.

Show 6 quoted lines
>> +     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.
Show 35 quoted lines
>> +}
>> +
>> +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:
Show 6 quoted lines
> 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
Johan Herland· Nov 5, 2014, 08:32 UTC · re: Johan Herland · lore

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

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

← back to recent threads