| Message ID | d5a6c74792c15e2f83c2ed0758fb99eac11a8174.1677136319.git.dyroneteng@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | notes.c: introduce "--separator" option | expand |
Teng Long <dyroneteng@gmail.com> writes: > So this commit > introduces a new '--separator' option for 'git-notes-add' and > 'git-notes-append', for example when execute: Introduce a new '--separator' option for 'git notes add' and 'git notes append'. > We will check the option value and if the value doesn't contail > a trailing '\n', will add it automatically, contail? A newline is added to the value given to --separator if it does not end with one already. > so execute > $ git notes add -m foo -m bar --separator="-" > and > $ export LF=" > " > $ git notes add -m foo -m bar --separator="-$LF" > > have the same behavour. Running A and B produces the same result. > + subcommand). If you specify multiple `-m` and `-F`, a blank > + line will be inserted between the messages. Use the `--separator` > + option to insert other delimiters. Concise and readable. Nice. > @@ -86,7 +88,13 @@ the command can read the input given to the `post-rewrite` hook.) > > append:: > Append to the notes of an existing object (defaults to HEAD). > - Creates a new notes object if needed. > + Creates a new notes object if needed. > + The default delimiter is a blank line, use the `--separator` Re-read the above three lines, pretending that it is already 6 months and you've forgotten about adding the feature yourself. Notice something? A reader with fresh mind would read and think along the description but - OK, append command is used to append to the note to an existing object; - OK, if the object does not have a note yet, we will create one; - The default delimiter? Delimiter for what? I am puzzled. at the third step, gets puzzled. The command takes the existing note's contents, adds a delimiter and then appends the new material given by the user, but because that is not clear after reading the first two lines, the sudden appearance of "delimiter" would confuse readers. > + option to insert other delimiters. More specifically, if the > + note and the message are not empty, the delimiter will be > + inserted between them. If you specify multiple `-m` and `-F` Again, the order of presentation is somewhat backwards and that is why we need to say "More specifically" here. > + options, the delimiter will be inserted between the messages > + too. Append new message(s) given by `-m` or `-F` options to an existing note, or add them as a new note if one does not exist, for the object (defaults to HEAD). When appending to an existing note, a blank line is added before each new message as an inter-paragraph separator. The separator can be customized with the `--separator` option. or something along that line, perhaps? > +--separator <paragraph-break>:: > + The '<paragraph-break>' inserted between paragraphs. > + A blank line by default. Here is where you need to say about promoting incomplete line separator more than the proposed log message. Specify a string used as a custom inter-paragraph separator (a newline is added at the end as needed). Defaults to a blank line. > diff --git a/builtin/notes.c b/builtin/notes.c > index 553ae2bd..e0ada862 100644 > --- a/builtin/notes.c > +++ b/builtin/notes.c > @@ -24,11 +24,12 @@ > ... > +static void insert_separator(struct strbuf *message, size_t pos) > +{ > + if (!separator) > + strbuf_insertstr(message, pos, "\n"); > + else if (separator[strlen(separator) - 1] == '\n') > + strbuf_insertstr(message, pos, separator); > + else > + strbuf_insertf(message, pos, "%s%s", separator, "\n"); > +} It looks like you are very fond of "insert", but aren't we always appending with the latest control flow? In other words, is it worth carrying 'pos' around? > +static void parse_messages(struct string_list *messages, struct note_data *d) > +{ > + size_t i; > + for (i = 0; i < messages->nr; i++) { > + if (d->buf.len) > + insert_separator(&d->buf, d->buf.len); > + strbuf_insertstr(&d->buf, d->buf.len, > + messages->items[i].string); > + strbuf_stripspace(&d->buf, 0); This is not a new problem, but if we get three 100-byte messages, we - add the first 100-byte message to d->buf and then run stripspace() over that 100-byte. - add separator and then the second 100-byte message to d->buf, and then run stripspace() over that 200-plus-byte. - add separator and then the third 100-byte message to d->buf, and then run stripspace() over that 300-plus-byte. Shouldn't we be doing better? > + d->given = 1; Do we understand what d->given flag represents? My understanding is that it becomes true only when any of the -m/-F/-c/-C options are given to tell the command what message to use, so that we can automatically open the editor to ask for the message when nothing is given. So, I suspect that d->given = !!messages->nr; at the beginning of the function, or d->given = !!d->buf.len; may be equivalent[*], instead of setting it once every iteration? Side note: The latter is slightly more strict, as giving a run of empty strings with the default separator would result in an empty d->buf and d->given will become false. > + } > +} This helper is not parsing, but just processing after the whole thing was parsed. "parse_messages" -> "concatenate_messages" or something, perhaps? Now d->given is set in parse_reuse_arg() and parse_reedit_arg(), because -c/-C is another source of a paragraph. Shouldn't the paragraph taken by these options go in the message list to be concatenated together with other messages, or are -c/-C incompatible with -m/-F and we are OK with two separate and distinct behaviour? > static int parse_msg_arg(const struct option *opt, const char *arg, int unset) > { > - struct note_data *d = opt->value; > + struct string_list *msg = opt->value; > > BUG_ON_OPT_NEG(unset); > > - if (d->buf.len) > - strbuf_addch(&d->buf, '\n'); > - strbuf_addstr(&d->buf, arg); > - strbuf_stripspace(&d->buf, 0); > - > - d->given = 1; > + string_list_append(msg, arg); > return 0; > } OK, this one now does not concatenate; just accumulates to the "msg" string list. > static int parse_file_arg(const struct option *opt, const char *arg, int unset) > { > - struct note_data *d = opt->value; > + struct string_list *msg = opt->value; > + struct strbuf buf = STRBUF_INIT; > > BUG_ON_OPT_NEG(unset); > > - if (d->buf.len) > - strbuf_addch(&d->buf, '\n'); > if (!strcmp(arg, "-")) { > - if (strbuf_read(&d->buf, 0, 1024) < 0) > + if (strbuf_read(&buf, 0, 1024) < 0) > die_errno(_("cannot read '%s'"), arg); > - } else if (strbuf_read_file(&d->buf, arg, 1024) < 0) > + } else if (strbuf_read_file(&buf, arg, 1024) < 0) > die_errno(_("could not open or read '%s'"), arg); > - strbuf_stripspace(&d->buf, 0); > > - d->given = 1; > + string_list_append(msg, buf.buf); > + strbuf_release(&buf); > return 0; > } Ditto. > @@ -418,6 +438,8 @@ static int add(int argc, const char **argv, const char *prefix) > OPT_BOOL(0, "allow-empty", &allow_empty, > N_("allow storing empty note")), > OPT__FORCE(&force, N_("replace existing notes"), PARSE_OPT_NOCOMPLETE), > + OPT_STRING(0, "separator", &separator, N_("separator"), > + N_("insert <paragraph-break> between paragraphs")), > OPT_END() > }; > > @@ -429,6 +451,7 @@ static int add(int argc, const char **argv, const char *prefix) > usage_with_options(git_notes_add_usage, options); > } > > + parse_messages(&messages, &d); Yup, this one is "concatenate_messages". We do not read the existing one, we accumulated everything the user gave us in messages, and concatenate them using the separator. The result is stored in d->buf. I wonder why separator is not a parameter into the helper function? In short, I think handling of -m/-F got vastly better from the previous rounds in this version, but I am not sure about the only other remaining "strbuf_addch(&d->buf, '\n')" in parse_reuse_arg(). I am inclined to say that that codepath should behave the same way, but I didn't think it as deeply as you did, so...? Thanks.
Teng Long <dyroneteng@gmail.com> writes: > +static void parse_messages(struct string_list *messages, struct note_data *d) > +{ > + size_t i; > + for (i = 0; i < messages->nr; i++) { > + if (d->buf.len) > + insert_separator(&d->buf, d->buf.len); > + strbuf_insertstr(&d->buf, d->buf.len, > + messages->items[i].string); > + strbuf_stripspace(&d->buf, 0); > + d->given = 1; > + } > +} The two callers of this function prepares the string_list, and have this function consume it by concatenating its contents to d->buf. After calling this function, neither of them talks about messages, which means we are leaking the strings kept in the string_list. I could eject the topic from today's integration run (because the topic is not ready to be merged to 'next' as-is; "-C/-c" codepaths need to be adjusted, at least), but as I took a look already, I'll queue this fix-up on top of the topic for now. Feel free to squash it in (or address the leaks in your own way) when sending in an update. Thanks. builtin/notes.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git c/builtin/notes.c w/builtin/notes.c index 97c18fc02f..cd7af76e2f 100644 --- c/builtin/notes.c +++ w/builtin/notes.c @@ -220,7 +220,8 @@ static void insert_separator(struct strbuf *message, size_t pos) strbuf_insertf(message, pos, "%s%s", separator, "\n"); } -static void parse_messages(struct string_list *messages, struct note_data *d) +/* Consume messages and append them into d->buf */ +static void concat_messages(struct string_list *messages, struct note_data *d) { size_t i; for (i = 0; i < messages->nr; i++) { @@ -231,6 +232,7 @@ static void parse_messages(struct string_list *messages, struct note_data *d) strbuf_stripspace(&d->buf, 0); d->given = 1; } + string_list_clear(messages, 0); } static int parse_msg_arg(const struct option *opt, const char *arg, int unset) @@ -451,7 +453,7 @@ static int add(int argc, const char **argv, const char *prefix) usage_with_options(git_notes_add_usage, options); } - parse_messages(&messages, &d); + concat_messages(&messages, &d); object_ref = argc > 1 ? argv[1] : "HEAD"; if (get_oid(object_ref, &object)) @@ -622,7 +624,7 @@ static int append_edit(int argc, const char **argv, const char *prefix) usage_with_options(usage, options); } - parse_messages(&messages, &d); + concat_messages(&messages, &d); if (d.given && edit) fprintf(stderr, _("The -m/-F/-c/-C options have been deprecated "
Junio C Hamano <gitster@pobox.com>: > > So this commit > > introduces a new '--separator' option for 'git-notes-add' and > > 'git-notes-append', for example when execute: > > Introduce a new '--separator' option for 'git notes add' and > 'git notes append'. Thanks, I will avoid this like 'git-subcommand' expression in the future. > > We will check the option value and if the value doesn't contail > > a trailing '\n', will add it automatically, > > contail? "contain", sorry. > A newline is added to the value given to --separator if it > does not end with one already. Will apply. > > so execute > > $ git notes add -m foo -m bar --separator="-" > > and > > $ export LF=" > > " > > $ git notes add -m foo -m bar --separator="-$LF" > > > > have the same behavour. > > Running A and B produces the same result. Will s/Running A and B produces the same result/have the same behavour > - The default delimiter? Delimiter for what? I am puzzled. > > at the third step, gets puzzled. The command takes the existing > note's contents, adds a delimiter and then appends the new material > given by the user, but because that is not clear after reading the > first two lines, the sudden appearance of "delimiter" would confuse > readers. > > > + option to insert other delimiters. More specifically, if the > > + note and the message are not empty, the delimiter will be > > + inserted between them. If you specify multiple `-m` and `-F` > > Again, the order of presentation is somewhat backwards and that is > why we need to say "More specifically" here. > > > + options, the delimiter will be inserted between the messages > > + too. > > Append new message(s) given by `-m` or `-F` options to an > existing note, or add them as a new note if one does not > exist, for the object (defaults to HEAD). When appending to > an existing note, a blank line is added before each new > message as an inter-paragraph separator. The separator can > be customized with the `--separator` option. Will apply. > Here is where you need to say about promoting incomplete line > separator more than the proposed log message. > > Specify a string used as a custom inter-paragraph separator > (a newline is added at the end as needed). Defaults to a > blank line. Will apply. > > +static void insert_separator(struct strbuf *message, size_t pos) > > +{ > > + if (!separator) > > + strbuf_insertstr(message, pos, "\n"); > > + else if (separator[strlen(separator) - 1] == '\n') > > + strbuf_insertstr(message, pos, separator); > > + else > > + strbuf_insertf(message, pos, "%s%s", separator, "\n"); > > +} > > It looks like you are very fond of "insert", but aren't we always > appending with the latest control flow? In other words, is it worth > carrying 'pos' around? Actually I can't find a more suitable verb. I think it worth, but I'm not sure whether there is a better way, that is, when we add separator which use to contact messages like multiple '-m' and '-F', the 'pos' is at the end. When we add separator which use to contact the prev_notes and the new notes , the 'pos' is at the head. > > +static void parse_messages(struct string_list *messages, struct note_data *d) > > +{ > > + size_t i; > > + for (i = 0; i < messages->nr; i++) { > > + if (d->buf.len) > > + insert_separator(&d->buf, d->buf.len); > > + strbuf_insertstr(&d->buf, d->buf.len, > > + messages->items[i].string); > > + strbuf_stripspace(&d->buf, 0); > > This is not a new problem, but if we get three 100-byte messages, we > > - add the first 100-byte message to d->buf and then run > stripspace() over that 100-byte. > > - add separator and then the second 100-byte message to d->buf, and > then run stripspace() over that 200-plus-byte. > > - add separator and then the third 100-byte message to d->buf, and > then run stripspace() over that 300-plus-byte. > > Shouldn't we be doing better? After I read the comments of 'strbuf_stripspace', it does : 1. remove empty lines from beginning and end. 2. turn multiple empty lines between paragraphs into just one empty line. 3. if the input has only empty lines and spaces, no output will be produced. 4. if the last line doesn't have a newline at the end, one is added. 5. skip every commentted line if skip_comments is enabled. I think above the five functions in 'strbuf_stripspace', we may just care about the fourth, the others just like the logic when we edit the commit message when doing 'git commit' or 'git merge', etc. My opinion is a "commit note" and a "commit message" maybe not be the same, and sometimes the user might want a blank line or something at the beginning of the notes. But after I read the tests I think they are just the same. So, let me just fix this by stripspace the message individually, thanks. > Do we understand what d->given flag represents? My understanding is > that it becomes true only when any of the -m/-F/-c/-C options are given > to tell the command what message to use, so that we can automatically > open the editor to ask for the message when nothing is given. > > So, I suspect that > > d->given = !!messages->nr; > > at the beginning of the function, or > > d->given = !!d->buf.len; > > may be equivalent[*], instead of setting it once every iteration? > > Side note: The latter is slightly more strict, as giving a > run of empty strings with the default separator would result > in an empty d->buf and d->given will become false. I agree, we should choose the latter and move "d->given = !!d->buf.len;" behind the place where we concat the messages. > Now d->given is set in parse_reuse_arg() and parse_reedit_arg(), > because -c/-C is another source of a paragraph. Shouldn't the > paragraph taken by these options go in the message list to be > concatenated together with other messages, or are -c/-C incompatible > with -m/-F and we are OK with two separate and distinct behaviour? Yes, my bad. -c/-C should be considered as well and dealing them with the same messages-contacting logic, I will fix these two place in next patch. > I wonder why separator is not a parameter into the helper function? In my opinion, your proposal is probably similar to the current approach, but your solution may be a bit more readable. I will consider changing it in the next patch. > other remaining "strbuf_addch(&d->buf, '\n')" in parse_reuse_arg(). > I am inclined to say that that codepath should behave the same way, > but I didn't think it as deeply as you did, so...? You really have a good eye, I didn't noticed them. There are two place remained, the first one is "parse_reuse_arg", second one is "prepare_note_data" it's made for separate the note and the commented lines (note template), I think they should both be replaced/impacted by "--separator". Thanks.
Junio C Hamano <gitster@pobox.com> writes: > The two callers of this function prepares the string_list, and have > this function consume it by concatenating its contents to d->buf. > After calling this function, neither of them talks about messages, > which means we are leaking the strings kept in the string_list. > > I could eject the topic from today's integration run (because the > topic is not ready to be merged to 'next' as-is; "-C/-c" codepaths > need to be adjusted, at least), but as I took a look already, I'll > queue this fix-up on top of the topic for now. Feel free to squash > it in (or address the leaks in your own way) when sending in an > update. > > Thanks. > > builtin/notes.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git c/builtin/notes.c w/builtin/notes.c > index 97c18fc02f..cd7af76e2f 100644 > --- c/builtin/notes.c > +++ w/builtin/notes.c > @@ -220,7 +220,8 @@ static void insert_separator(struct strbuf *message, size_t pos) > strbuf_insertf(message, pos, "%s%s", separator, "\n"); > } > > -static void parse_messages(struct string_list *messages, struct note_data *d) > +/* Consume messages and append them into d->buf */ > +static void concat_messages(struct string_list *messages, struct note_data *d) > { > size_t i; > for (i = 0; i < messages->nr; i++) { > @@ -231,6 +232,7 @@ static void parse_messages(struct string_list *messages, struct note_data *d) > strbuf_stripspace(&d->buf, 0); > d->given = 1; > } > + string_list_clear(messages, 0); > } > > static int parse_msg_arg(const struct option *opt, const char *arg, int unset) > @@ -451,7 +453,7 @@ static int add(int argc, const char **argv, const char *prefix) > usage_with_options(git_notes_add_usage, options); > } > > - parse_messages(&messages, &d); > + concat_messages(&messages, &d); > object_ref = argc > 1 ? argv[1] : "HEAD"; > > if (get_oid(object_ref, &object)) > @@ -622,7 +624,7 @@ static int append_edit(int argc, const char **argv, const char *prefix) > usage_with_options(usage, options); > } > > - parse_messages(&messages, &d); > + concat_messages(&messages, &d); > > if (d.given && edit) > fprintf(stderr, _("The -m/-F/-c/-C options have been deprecated " Thanks, will apply in next patch.
diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt index efbc10f0..53d63888 100644 --- a/Documentation/git-notes.txt +++ b/Documentation/git-notes.txt @@ -9,9 +9,9 @@ SYNOPSIS -------- [verse] 'git notes' [list [<object>]] -'git notes' add [-f] [--allow-empty] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>] +'git notes' add [-f] [--allow-empty] [--separator=<paragraph-break>] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>] 'git notes' copy [-f] ( --stdin | <from-object> [<to-object>] ) -'git notes' append [--allow-empty] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>] +'git notes' append [--allow-empty] [--separator=<paragraph-break>] [-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> @@ -65,7 +65,9 @@ add:: However, if you're using `add` interactively (using an editor to supply the notes contents), then - instead of aborting - the existing notes will be opened in the editor (like the `edit` - subcommand). + subcommand). If you specify multiple `-m` and `-F`, a blank + line will be inserted between the messages. Use the `--separator` + option to insert other delimiters. copy:: Copy the notes for the first object onto the second object (defaults to @@ -86,7 +88,13 @@ the command can read the input given to the `post-rewrite` hook.) append:: Append to the notes of an existing object (defaults to HEAD). - Creates a new notes object if needed. + Creates a new notes object if needed. + The default delimiter is a blank line, use the `--separator` + option to insert other delimiters. More specifically, if the + note and the message are not empty, the delimiter will be + inserted between them. If you specify multiple `-m` and `-F` + options, the delimiter will be inserted between the messages + too. edit:: Edit the notes for a given object (defaults to HEAD). @@ -159,6 +167,10 @@ OPTIONS Allow an empty note object to be stored. The default behavior is to automatically remove empty notes. +--separator <paragraph-break>:: + The '<paragraph-break>' inserted between paragraphs. + A blank line by default. + --ref <ref>:: Manipulate the notes tree in <ref>. This overrides `GIT_NOTES_REF` and the "core.notesRef" configuration. The ref diff --git a/builtin/notes.c b/builtin/notes.c index 553ae2bd..e0ada862 100644 --- a/builtin/notes.c +++ b/builtin/notes.c @@ -24,11 +24,12 @@ #include "notes-utils.h" #include "worktree.h" +static char *separator = NULL; static const char * const git_notes_usage[] = { N_("git notes [--ref <notes-ref>] [list [<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>] add [-f] [--allow-empty] [--separator=<paragraph-break>] [-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 [--allow-empty] [-m <msg> | -F <file> | (-c | -C) <object>] [<object>]"), + N_("git notes [--ref <notes-ref>] append [--allow-empty] [--separator=<paragraph-break>] [-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>"), @@ -209,37 +210,55 @@ static void write_note_data(struct note_data *d, struct object_id *oid) } } +static void insert_separator(struct strbuf *message, size_t pos) +{ + if (!separator) + strbuf_insertstr(message, pos, "\n"); + else if (separator[strlen(separator) - 1] == '\n') + strbuf_insertstr(message, pos, separator); + else + strbuf_insertf(message, pos, "%s%s", separator, "\n"); +} + +static void parse_messages(struct string_list *messages, struct note_data *d) +{ + size_t i; + for (i = 0; i < messages->nr; i++) { + if (d->buf.len) + insert_separator(&d->buf, d->buf.len); + strbuf_insertstr(&d->buf, d->buf.len, + messages->items[i].string); + strbuf_stripspace(&d->buf, 0); + d->given = 1; + } +} + static int parse_msg_arg(const struct option *opt, const char *arg, int unset) { - struct note_data *d = opt->value; + struct string_list *msg = opt->value; BUG_ON_OPT_NEG(unset); - if (d->buf.len) - strbuf_addch(&d->buf, '\n'); - strbuf_addstr(&d->buf, arg); - strbuf_stripspace(&d->buf, 0); - - d->given = 1; + string_list_append(msg, arg); return 0; } + static int parse_file_arg(const struct option *opt, const char *arg, int unset) { - struct note_data *d = opt->value; + struct string_list *msg = opt->value; + struct strbuf buf = STRBUF_INIT; BUG_ON_OPT_NEG(unset); - if (d->buf.len) - strbuf_addch(&d->buf, '\n'); if (!strcmp(arg, "-")) { - if (strbuf_read(&d->buf, 0, 1024) < 0) + if (strbuf_read(&buf, 0, 1024) < 0) die_errno(_("cannot read '%s'"), arg); - } else if (strbuf_read_file(&d->buf, arg, 1024) < 0) + } else if (strbuf_read_file(&buf, arg, 1024) < 0) die_errno(_("could not open or read '%s'"), arg); - strbuf_stripspace(&d->buf, 0); - d->given = 1; + string_list_append(msg, buf.buf); + strbuf_release(&buf); return 0; } @@ -402,11 +421,12 @@ static int add(int argc, const char **argv, const char *prefix) struct object_id object, new_note; const struct object_id *note; struct note_data d = { .buf = STRBUF_INIT }; + struct string_list messages = STRING_LIST_INIT_DUP; struct option options[] = { - OPT_CALLBACK_F('m', "message", &d, N_("message"), + OPT_CALLBACK_F('m', "message", &messages, N_("message"), N_("note contents as a string"), PARSE_OPT_NONEG, parse_msg_arg), - OPT_CALLBACK_F('F', "file", &d, N_("file"), + OPT_CALLBACK_F('F', "file", &messages, N_("file"), N_("note contents in a file"), PARSE_OPT_NONEG, parse_file_arg), OPT_CALLBACK_F('c', "reedit-message", &d, N_("object"), @@ -418,6 +438,8 @@ static int add(int argc, const char **argv, const char *prefix) OPT_BOOL(0, "allow-empty", &allow_empty, N_("allow storing empty note")), OPT__FORCE(&force, N_("replace existing notes"), PARSE_OPT_NOCOMPLETE), + OPT_STRING(0, "separator", &separator, N_("separator"), + N_("insert <paragraph-break> between paragraphs")), OPT_END() }; @@ -429,6 +451,7 @@ static int add(int argc, const char **argv, const char *prefix) usage_with_options(git_notes_add_usage, options); } + parse_messages(&messages, &d); object_ref = argc > 1 ? argv[1] : "HEAD"; if (get_oid(object_ref, &object)) @@ -568,11 +591,12 @@ static int append_edit(int argc, const char **argv, const char *prefix) char *logmsg; const char * const *usage; struct note_data d = { .buf = STRBUF_INIT }; + struct string_list messages = STRING_LIST_INIT_DUP; struct option options[] = { - OPT_CALLBACK_F('m', "message", &d, N_("message"), - N_("note contents as a string"), PARSE_OPT_NONEG, + OPT_CALLBACK_F('m', "message", &messages, N_("message"), + N_("note contents as a string"), PARSE_OPT_NONEG, parse_msg_arg), - OPT_CALLBACK_F('F', "file", &d, N_("file"), + OPT_CALLBACK_F('F', "file", &messages, N_("file"), N_("note contents in a file"), PARSE_OPT_NONEG, parse_file_arg), OPT_CALLBACK_F('c', "reedit-message", &d, N_("object"), @@ -583,6 +607,8 @@ static int append_edit(int argc, const char **argv, const char *prefix) parse_reuse_arg), OPT_BOOL(0, "allow-empty", &allow_empty, N_("allow storing empty note")), + OPT_STRING(0, "separator", &separator, N_("separator"), + N_("insert <paragraph-break> between paragraphs")), OPT_END() }; int edit = !strcmp(argv[0], "edit"); @@ -596,6 +622,8 @@ static int append_edit(int argc, const char **argv, const char *prefix) usage_with_options(usage, options); } + parse_messages(&messages, &d); + if (d.given && edit) fprintf(stderr, _("The -m/-F/-c/-C options have been deprecated " "for the 'edit' subcommand.\n" @@ -618,7 +646,7 @@ static int append_edit(int argc, const char **argv, const char *prefix) char *prev_buf = read_object_file(note, &type, &size); if (d.buf.len && prev_buf && size) - strbuf_insertstr(&d.buf, 0, "\n"); + insert_separator(&d.buf, 0); if (prev_buf && size) strbuf_insert(&d.buf, 0, prev_buf, size); free(prev_buf); diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh index 3288aaec..c2c09f32 100755 --- a/t/t3301-notes.sh +++ b/t/t3301-notes.sh @@ -362,6 +362,7 @@ test_expect_success 'do not create empty note with -m ""' ' ' test_expect_success 'create note with combination of -m and -F' ' + test_when_finished git notes remove HEAD && cat >expect-combine_m_and_F <<-EOF && foo @@ -380,6 +381,26 @@ test_expect_success 'create note with combination of -m and -F' ' test_cmp expect-combine_m_and_F actual ' +test_expect_success 'create note with combination of -m and -F and --separator' ' + cat >expect-combine_m_and_F <<-\EOF && + foo + ------- + xyzzy + ------- + bar + ------- + zyxxy + ------- + baz + EOF + echo "xyzzy" >note_a && + echo "zyxxy" >note_b && + git notes add -m "foo" -F note_a -m "bar" -F note_b -m "baz" --separator "-------" && + git notes show >actual && + test_cmp expect-combine_m_and_F actual + +' + test_expect_success 'remove note with "git notes remove"' ' git notes remove HEAD^ && git notes remove && @@ -521,6 +542,85 @@ test_expect_success 'listing non-existing notes fails' ' test_must_be_empty actual ' +test_expect_success 'append: specify an empty separator' ' + test_when_finished git notes remove HEAD && + cat >expect <<-\EOF && + notes-1 + + notes-2 + EOF + + git notes add -m "notes-1" && + git notes append --separator="" -m "notes-2" && + git notes show >actual && + test_cmp expect actual +' + +test_expect_success 'append: specify separator with line break' ' + test_when_finished git notes remove HEAD && + cat >expect <<-\EOF && + notes-1 + ------- + notes-2 + EOF + + git notes add -m "notes-1" && + git notes append --separator="-------$LF" -m "notes-2" && + git notes show >actual && + test_cmp expect actual +' + +test_expect_success 'append: specify separator without line break' ' + test_when_finished git notes remove HEAD && + cat >expect <<-\EOF && + notes-1 + ------- + notes-2 + EOF + + git notes add -m "notes-1" && + git notes append --separator="-------" -m "notes-2" && + git notes show >actual && + test_cmp expect actual +' + +test_expect_success 'append: specify separator with multiple messages' ' + test_when_finished git notes remove HEAD && + cat >expect <<-\EOF && + notes-1 + ------- + notes-2 + ------- + notes-3 + EOF + + git notes add -m "notes-1" && + git notes append --separator="-------" -m "notes-2" -m "notes-3" && + git notes show >actual && + test_cmp expect actual +' + +test_expect_success 'append note with combination of -m and -F and --separator' ' + test_when_finished git notes remove HEAD && + cat >expect-combine_m_and_F <<-\EOF && + m-notes-1 + ------- + f-notes-1 + ------- + m-notes-2 + ------- + f-notes-2 + ------- + m-notes-3 + EOF + + echo "f-notes-1" >note_a && + echo "f-notes-2" >note_b && + git notes append -m "m-notes-1" -F note_a -m "m-notes-2" -F note_b -m "m-notes-3" --separator "-------" && + git notes show >actual && + test_cmp expect-combine_m_and_F actual +' + test_expect_success 'append to existing note with "git notes append"' ' cat >expect <<-EOF && Initial set of notes