Teng Long <dyroneteng@xxxxxxxxx> 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.
