builtin-notes: Refactor handling of -F option to allow combining -m and -F

By moving the -F option handling into a separate function (parse_file_arg),
we can start allowing several -F options, and mixed usage of -m and -F
options. Each -m/-F given appends to the note message, in the order they are
given on the command-line.

The patch includes tests verifying correct behaviour of the new functionality.

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

diff --git a/builtin-notes.c b/builtin-notes.c
index 572b47746bcb3f98528417595988229e72fccbc4..190c46c3bebc8150a65460add564e5e6d87f1e3b 100644 (file)
--- a/builtin-notes.c
+++ b/builtin-notes.c
@@ -34,6 +34,11 @@ static const char note_template[] =
        "# Write/edit the notes for the following object:\n"
        "#\n";
 
+struct msg_arg {
+       int given;
+       struct strbuf buf;
+};
+
 static int list_each_note(const unsigned char *object_sha1,
                const unsigned char *note_sha1, char *note_path,
                void *cb_data)
@@ -93,15 +98,13 @@ static void write_commented_object(int fd, const unsigned char *object)
                    sha1_to_hex(object));
 }
 
-static void create_note(const unsigned char *object,
-                       struct strbuf *buf,
-                       int skip_editor, int append_only,
-                       const unsigned char *prev,
+static void create_note(const unsigned char *object, struct msg_arg *msg,
+                       int append_only, const unsigned char *prev,
                        unsigned char *result)
 {
        char *path = NULL;
 
-       if (!skip_editor) {
+       if (!msg->given) {
                int fd;
 
                /* write the template message before editing: */
@@ -118,34 +121,33 @@ static void create_note(const unsigned char *object,
 
                close(fd);
 
-               if (launch_editor(path, buf, NULL)) {
+               if (launch_editor(path, &(msg->buf), NULL)) {
                        die("Please supply the note contents using either -m" \
                            " or -F option");
                }
+               stripspace(&(msg->buf), 1);
        }
 
-       stripspace(buf, 1);
-
        if (prev && append_only) {
                /* Append buf to previous note contents */
                unsigned long size;
                enum object_type type;
                char *prev_buf = read_sha1_file(prev, &type, &size);
 
-               strbuf_grow(buf, size + 1);
-               if (buf->len && prev_buf && size)
-                       strbuf_insert(buf, 0, "\n", 1);
+               strbuf_grow(&(msg->buf), size + 1);
+               if (msg->buf.len && prev_buf && size)
+                       strbuf_insert(&(msg->buf), 0, "\n", 1);
                if (prev_buf && size)
-                       strbuf_insert(buf, 0, prev_buf, size);
+                       strbuf_insert(&(msg->buf), 0, prev_buf, size);
                free(prev_buf);
        }
 
-       if (!buf->len) {
+       if (!msg->buf.len) {
                fprintf(stderr, "Removing note for object %s\n",
                        sha1_to_hex(object));
                hashclr(result);
        } else {
-               if (write_sha1_file(buf->buf, buf->len, blob_type, result)) {
+               if (write_sha1_file(msg->buf.buf, msg->buf.len, blob_type, result)) {
                        error("unable to write note object");
                        if (path)
                                error("The note contents has been left in %s",
@@ -160,20 +162,39 @@ static void create_note(const unsigned char *object,
        }
 }
 
-struct msg_arg {
-       int given;
-       struct strbuf buf;
-};
-
 static int parse_msg_arg(const struct option *opt, const char *arg, int unset)
 {
        struct msg_arg *msg = opt->value;
 
        if (!arg)
                return -1;
+
+       strbuf_grow(&(msg->buf), strlen(arg) + 2);
        if (msg->buf.len)
-               strbuf_addstr(&(msg->buf), "\n\n");
+               strbuf_addstr(&(msg->buf), "\n");
        strbuf_addstr(&(msg->buf), arg);
+       stripspace(&(msg->buf), 0);
+
+       msg->given = 1;
+       return 0;
+}
+
+static int parse_file_arg(const struct option *opt, const char *arg, int unset)
+{
+       struct msg_arg *msg = opt->value;
+
+       if (!arg)
+               return -1;
+
+       if (msg->buf.len)
+               strbuf_addstr(&(msg->buf), "\n");
+       if (!strcmp(arg, "-")) {
+               if (strbuf_read(&(msg->buf), 0, 1024) < 0)
+                       die_errno("cannot read '%s'", arg);
+       } else if (strbuf_read_file(&(msg->buf), arg, 1024) < 0)
+               die_errno("could not open or read '%s'", arg);
+       stripspace(&(msg->buf), 0);
+
        msg->given = 1;
        return 0;
 }
@@ -220,7 +241,6 @@ int commit_notes(struct notes_tree *t, const char *msg)
 
 int cmd_notes(int argc, const char **argv, const char *prefix)
 {
-       struct strbuf buf = STRBUF_INIT;
        struct notes_tree *t;
        unsigned char object[20], new_note[20];
        const unsigned char *note;
@@ -230,13 +250,13 @@ int cmd_notes(int argc, const char **argv, const char *prefix)
        int list = 0, add = 0, append = 0, edit = 0, show = 0, remove = 0,
            prune = 0, force = 0;
        int given_object;
-       const char *msgfile = NULL;
        struct msg_arg msg = { 0, STRBUF_INIT };
        struct option options[] = {
                OPT_GROUP("Notes options"),
-               OPT_CALLBACK('m', "message", &msg, "msg",
+               OPT_CALLBACK('m', "message", &msg, "MSG",
                             "note contents as a string", parse_msg_arg),
-               OPT_FILENAME('F', "file", &msgfile, "note contents in a file"),
+               OPT_CALLBACK('F', "file", &msg, "FILE",
+                            "note contents in a file", parse_file_arg),
                OPT_BOOLEAN('f', "force", &force, "replace existing notes"),
                OPT_END()
        };
@@ -265,21 +285,17 @@ int cmd_notes(int argc, const char **argv, const char *prefix)
        if (list + add + append + edit + show + remove + prune != 1)
                usage_with_options(git_notes_usage, options);
 
-       if ((msg.given || msgfile) && !(add || append || edit)) {
+       if (msg.given && !(add || append || edit)) {
                error("cannot use -m/-F options with %s subcommand.", argv[0]);
                usage_with_options(git_notes_usage, options);
        }
 
-       if ((msg.given || msgfile) && edit) {
+       if (msg.given && edit) {
                fprintf(stderr, "The -m and -F options has been deprecated for"
                        " the 'edit' subcommand.\n"
                        "Please use 'git notes add -f -m/-F' instead.\n");
        }
 
-       if (msg.given && msgfile) {
-               error("mixing -m and -F options is not allowed.");
-               usage_with_options(git_notes_usage, options);
-       }
 
        if (force && !add) {
                error("cannot use -f option with %s subcommand.", argv[0]);
@@ -341,24 +357,16 @@ int cmd_notes(int argc, const char **argv, const char *prefix)
                }
        }
 
-       if (remove)
-               strbuf_reset(&buf);
-       else if (msg.given)
-               strbuf_addbuf(&buf, &(msg.buf));
-       else if (msgfile) {
-               if (!strcmp(msgfile, "-")) {
-                       if (strbuf_read(&buf, 0, 1024) < 0)
-                               die_errno("cannot read '%s'", msgfile);
-               } else if (strbuf_read_file(&buf, msgfile, 1024) < 0)
-                       die_errno("could not open or read '%s'", msgfile);
+       if (remove) {
+               msg.given = 1;
+               strbuf_reset(&(msg.buf));
        }
 
        if (prune) {
                hashclr(new_note);
                prune_notes(t);
        } else {
-               create_note(object, &buf, msg.given || msgfile || remove,
-                           append, note, new_note);
+               create_note(object, &msg, append, note, new_note);
                if (is_null_sha1(new_note))
                        remove_note(t, object);
                else
@@ -369,6 +377,6 @@ int cmd_notes(int argc, const char **argv, const char *prefix)
        commit_notes(t, logmsg);
 
        free_notes(t);
-       strbuf_release(&buf);
+       strbuf_release(&(msg.buf));
        return 0;
 }

diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index 290ed63d4e3048cdaae77d10ea13434beaea6c7f..07090e399730d8fa39737a60e232c3f96599583e 100755 (executable)
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -284,8 +284,29 @@ test_expect_success 'verify non-creation of note with -m ""' '
        ! git notes show
 '
 
+cat > expect-combine_m_and_F << EOF
+foo
+
+xyzzy
+
+bar
+
+zyxxy
+
+baz
+EOF
+
+test_expect_success 'create note with combination of -m and -F' '
+       echo "xyzzy" > note_a &&
+       echo "zyxxy" > note_b &&
+       git notes add -m "foo" -F note_a -m "bar" -F note_b -m "baz" &&
+       git notes show > output &&
+       test_cmp expect-combine_m_and_F output
+'
+
 test_expect_success 'remove note with "git notes remove" (setup)' '
-       git notes remove HEAD^
+       git notes remove HEAD^ &&
+       git notes remove
 '
 
 cat > expect-rm-remove << EOF