* [PATCHv2 1/3] builtin/notes: Fix premature failure when trying to add the empty blob @ 2014-11-05 8:32 Johan Herland 2014-11-05 8:32 ` [PATCHv2 2/3] t3312-notes-empty: Test that 'git notes' removes empty notes by default Johan Herland 2014-11-05 8:32 ` [PATCHv2 3/3] notes: Add --allow-empty, to allow storing empty notes Johan Herland 0 siblings, 2 replies; 5+ messages in thread From: Johan Herland @ 2014-11-05 8:32 UTC (permalink / raw) To: gitster; +Cc: git, mackyle, jhf, Eric Sunshine, 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCHv2 2/3] t3312-notes-empty: Test that 'git notes' removes empty notes by default 2014-11-05 8:32 [PATCHv2 1/3] builtin/notes: Fix premature failure when trying to add the empty blob Johan Herland @ 2014-11-05 8:32 ` Johan Herland 2014-11-05 19:00 ` Junio C Hamano 2014-11-05 8:32 ` [PATCHv2 3/3] notes: Add --allow-empty, to allow storing empty notes Johan Herland 1 sibling, 1 reply; 5+ messages in thread From: Johan Herland @ 2014-11-05 8:32 UTC (permalink / raw) To: gitster; +Cc: git, mackyle, jhf, Eric Sunshine, 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCHv2 2/3] t3312-notes-empty: Test that 'git notes' removes empty notes by default 2014-11-05 8:32 ` [PATCHv2 2/3] t3312-notes-empty: Test that 'git notes' removes empty notes by default Johan Herland @ 2014-11-05 19:00 ` Junio C Hamano 2014-11-07 9:09 ` Johan Herland 0 siblings, 1 reply; 5+ messages in thread From: Junio C Hamano @ 2014-11-05 19:00 UTC (permalink / raw) To: Johan Herland; +Cc: git, mackyle, jhf, Eric Sunshine 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCHv2 2/3] t3312-notes-empty: Test that 'git notes' removes empty notes by default 2014-11-05 19:00 ` Junio C Hamano @ 2014-11-07 9:09 ` Johan Herland 0 siblings, 0 replies; 5+ messages in thread From: Johan Herland @ 2014-11-07 9:09 UTC (permalink / raw) To: Junio C Hamano Cc: Git mailing list, Kyle J. McKay, James H. Fisher, Eric Sunshine 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCHv2 3/3] notes: Add --allow-empty, to allow storing empty notes 2014-11-05 8:32 [PATCHv2 1/3] builtin/notes: Fix premature failure when trying to add the empty blob Johan Herland 2014-11-05 8:32 ` [PATCHv2 2/3] t3312-notes-empty: Test that 'git notes' removes empty notes by default Johan Herland @ 2014-11-05 8:32 ` Johan Herland 1 sibling, 0 replies; 5+ messages in thread From: Johan Herland @ 2014-11-05 8:32 UTC (permalink / raw) To: gitster; +Cc: git, mackyle, jhf, Eric Sunshine, 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-11-07 9:09 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-11-05 8:32 [PATCHv2 1/3] builtin/notes: Fix premature failure when trying to add the empty blob Johan Herland 2014-11-05 8:32 ` [PATCHv2 2/3] t3312-notes-empty: Test that 'git notes' removes empty notes by default Johan Herland 2014-11-05 19:00 ` Junio C Hamano 2014-11-07 9:09 ` Johan Herland 2014-11-05 8:32 ` [PATCHv2 3/3] notes: Add --allow-empty, to allow storing empty notes Johan Herland
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).