From: Junio C Hamano <gitster@pobox.com>
To: Teng Long <dyroneteng@gmail.com>
Cc: avarab@gmail.com, git@vger.kernel.org, sunshine@sunshineco.com,
tenglong.tl@alibaba-inc.com
Subject: Re: [PATCH v6 3/3] notes.c: introduce '--separator=<paragraph-break>' option
Date: Thu, 23 Feb 2023 10:21:27 -0800 [thread overview]
Message-ID: <xmqqcz608cpk.fsf@gitster.g> (raw)
In-Reply-To: <d5a6c74792c15e2f83c2ed0758fb99eac11a8174.1677136319.git.dyroneteng@gmail.com> (Teng Long's message of "Thu, 23 Feb 2023 15:29:47 +0800")
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.
next prev parent reply other threads:[~2023-02-23 18:21 UTC|newest]
Thread overview: 186+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-13 5:56 [RFC PATCH 0/2] notes.c: introduce "--no-blankline" option Teng Long
2022-10-13 5:56 ` [RFC PATCH 1/2] " Teng Long
2022-10-13 6:06 ` Junio C Hamano
2022-10-17 13:19 ` Teng Long
2022-10-13 9:31 ` Ævar Arnfjörð Bjarmason
2022-10-17 13:33 ` Teng Long
2022-10-13 5:56 ` [RFC PATCH 2/2] notes.c: fixed tip when target and append note are both empty Teng Long
2022-10-13 9:36 ` Ævar Arnfjörð Bjarmason
2022-10-13 10:10 ` Phillip Wood
2022-10-13 10:23 ` Ævar Arnfjörð Bjarmason
2022-10-15 19:40 ` Phillip Wood
2022-10-18 3:25 ` Teng Long
2022-10-18 8:08 ` Teng Long
2022-10-18 3:11 ` Teng Long
2022-10-18 9:23 ` Ævar Arnfjörð Bjarmason
2022-11-07 13:57 ` [PATCH v2 0/3] notes.c: introduce "--blank-line" option Teng Long
2022-11-07 13:57 ` [PATCH v2 1/3] " Teng Long
2022-11-07 14:45 ` Ævar Arnfjörð Bjarmason
2022-11-07 15:45 ` Eric Sunshine
2022-11-07 17:22 ` Ævar Arnfjörð Bjarmason
2022-11-07 21:46 ` Taylor Blau
2022-11-07 22:36 ` Ævar Arnfjörð Bjarmason
2022-11-08 0:32 ` Taylor Blau
2022-11-08 3:45 ` Teng Long
2022-11-08 13:06 ` Teng Long
2022-11-08 13:22 ` Ævar Arnfjörð Bjarmason
2022-11-09 6:35 ` Teng Long
2022-11-07 15:06 ` Ævar Arnfjörð Bjarmason
2022-11-08 6:32 ` Teng Long
2022-11-07 21:47 ` Taylor Blau
2022-11-08 7:36 ` Teng Long
2022-11-07 13:57 ` [PATCH v2 2/3] notes.c: fixed tip when target and append note are both empty Teng Long
2022-11-07 14:40 ` Ævar Arnfjörð Bjarmason
2022-11-07 21:51 ` Taylor Blau
2022-11-07 22:33 ` Ævar Arnfjörð Bjarmason
2022-11-07 22:45 ` Taylor Blau
2022-11-08 8:55 ` Teng Long
2022-11-07 13:57 ` [PATCH v2 3/3] notes.c: drop unreachable code in "append_edit()" Teng Long
2022-11-07 14:41 ` Ævar Arnfjörð Bjarmason
2022-11-07 14:57 ` [PATCH v2 0/3] notes.c: introduce "--blank-line" option Ævar Arnfjörð Bjarmason
2022-11-09 7:05 ` Teng Long
2022-11-09 7:06 ` Teng Long
2022-11-09 9:06 ` [PATCH v3 0/5] notes.c: introduce "--no-blank-line" option Teng Long
2022-11-09 9:06 ` [PATCH v3 1/5] notes.c: cleanup 'strbuf_grow' call in 'append_edit' Teng Long
2022-11-09 9:06 ` [PATCH v3 2/5] notes.c: cleanup for "designated init" and "char ptr init" Teng Long
2022-11-09 9:06 ` [PATCH v3 3/5] notes.c: drop unreachable code in 'append_edit()' Teng Long
2022-11-09 9:06 ` [PATCH v3 4/5] notes.c: provide tips when target and append note are both empty Teng Long
2022-11-09 9:06 ` [PATCH v3 5/5] notes.c: introduce "--no-blank-line" option Teng Long
2022-11-28 14:20 ` [PATCH v3 0/5] " Teng Long
2022-11-29 1:10 ` Junio C Hamano
2022-11-29 22:53 ` Taylor Blau
2022-11-29 12:57 ` Teng Long
2022-11-29 13:19 ` Junio C Hamano
2022-12-15 12:48 ` Teng Long
2022-12-19 3:03 ` Eric Sunshine
2022-12-21 9:16 ` Teng Long
2022-12-21 11:35 ` Junio C Hamano
2022-12-22 9:30 ` Teng Long
2022-12-23 1:36 ` Eric Sunshine
2023-01-12 2:48 ` [PATCH v4 0/5] notes.c: introduce "--separator" optio Teng Long
2023-01-12 2:48 ` [PATCH v4 1/5] notes.c: cleanup 'strbuf_grow' call in 'append_edit' Teng Long
2023-01-15 4:53 ` Eric Sunshine
2023-01-28 11:22 ` Teng Long
2023-01-12 2:48 ` [PATCH v4 2/5] notes.c: cleanup for "designated init" and "char ptr init" Teng Long
2023-01-12 9:51 ` Ævar Arnfjörð Bjarmason
2023-01-28 11:33 ` Teng Long
2023-01-12 2:48 ` [PATCH v4 3/5] notes.c: drop unreachable code in 'append_edit()' Teng Long
2023-01-15 20:59 ` Eric Sunshine
2023-01-15 21:10 ` Eric Sunshine
2023-01-28 11:50 ` Teng Long
2023-01-30 5:38 ` Eric Sunshine
2023-02-01 8:08 ` Teng Long
2023-01-12 2:48 ` [PATCH v4 4/5] notes.c: provide tips when target and append note are both empty Teng Long
2023-01-12 9:52 ` Ævar Arnfjörð Bjarmason
2023-01-15 21:28 ` Eric Sunshine
2023-01-12 2:48 ` [PATCH v4 5/5] notes.c: introduce "--separator" option Teng Long
2023-01-12 9:53 ` Ævar Arnfjörð Bjarmason
2023-01-15 22:04 ` Eric Sunshine
2023-01-15 22:15 ` Eric Sunshine
2023-02-16 13:05 ` [PATCH v5 0/3] " Teng Long
2023-02-16 13:05 ` [PATCH v5 1/3] notes.c: cleanup 'strbuf_grow' call in 'append_edit' Teng Long
2023-02-16 18:39 ` Junio C Hamano
2023-02-20 3:34 ` Teng Long
2023-02-16 13:05 ` [PATCH v5 2/3] notes.c: cleanup for "designated init" Teng Long
2023-02-16 18:39 ` Junio C Hamano
2023-02-16 13:05 ` [PATCH v5 3/3] notes.c: introduce "--separator" option Teng Long
2023-02-16 23:22 ` Junio C Hamano
2023-02-20 14:00 ` Teng Long
2023-02-21 21:31 ` Junio C Hamano
2023-02-22 8:17 ` Teng Long
2023-02-22 23:15 ` Junio C Hamano
2023-02-23 7:29 ` [PATCH v6 0/3] " Teng Long
2023-02-23 7:29 ` [PATCH v6 1/3] notes.c: cleanup 'strbuf_grow' call in 'append_edit' Teng Long
2023-02-23 7:29 ` [PATCH v6 2/3] notes.c: cleanup for "designated init" Teng Long
2023-02-23 7:29 ` [PATCH v6 3/3] notes.c: introduce '--separator=<paragraph-break>' option Teng Long
2023-02-23 18:21 ` Junio C Hamano [this message]
2023-02-28 14:11 ` Teng Long
2023-02-25 21:30 ` Junio C Hamano
2023-02-28 14:14 ` Teng Long
2023-03-27 13:13 ` [PATCH v6 0/3] notes.c: introduce "--separator" option Teng Long
2023-03-28 14:28 ` [PATCH v7 0/4] " Teng Long
2023-03-28 14:28 ` [PATCH v7 1/4] notes.c: cleanup 'strbuf_grow' call in 'append_edit' Teng Long
2023-03-28 14:28 ` [PATCH v7 2/4] notes.c: cleanup for "designated init" Teng Long
2023-03-29 22:17 ` Junio C Hamano
2023-03-28 14:28 ` [PATCH v7 3/4] notes.c: introduce '--separator=<paragraph-break>' option Teng Long
2023-03-28 15:37 ` Junio C Hamano
2023-03-29 14:15 ` Teng Long
2023-03-29 21:48 ` Junio C Hamano
2023-04-13 9:36 ` Teng Long
2023-03-28 14:28 ` [PATCH v7 4/4] notes.c: don't do stripespace when parse file arg Teng Long
2023-03-28 15:54 ` Junio C Hamano
2023-03-29 12:06 ` Teng Long
2023-03-29 16:21 ` Junio C Hamano
2023-04-25 13:34 ` [PATCH 0/6] notes.c: introduce "--separator" option Teng Long
2023-04-25 13:34 ` [PATCH v8 1/6] notes.c: cleanup 'strbuf_grow' call in 'append_edit' Teng Long
2023-04-25 13:34 ` [PATCH v8 2/6] notes.c: use designated initializers for clarity Teng Long
2023-04-25 13:34 ` [PATCH v8 3/6] t3321: add test cases about the notes stripspace behavior Teng Long
2023-04-25 16:25 ` Junio C Hamano
2023-04-27 3:47 ` Teng Long
2023-04-25 13:34 ` [PATCH v8 4/6] notes.c: introduce '--separator=<paragraph-break>' option Teng Long
2023-04-25 17:34 ` Junio C Hamano
2023-04-27 7:21 ` Teng Long
2023-04-27 18:21 ` Junio C Hamano
2023-04-25 17:35 ` Junio C Hamano
2023-04-25 13:34 ` [PATCH v8 5/6] notes.c: append separator instead of insert by pos Teng Long
2023-04-25 17:47 ` Junio C Hamano
2023-04-27 7:51 ` Teng Long
2023-04-25 13:34 ` [PATCH v8 6/6] notes.c: introduce "--[no-]stripspace" option Teng Long
2023-04-25 17:49 ` Junio C Hamano
2023-04-28 7:40 ` Teng Long
2023-04-28 18:21 ` Junio C Hamano
2023-04-28 9:23 ` [PATCH v9 0/6] notes.c: introduce "--separator" option Teng Long
2023-04-28 9:23 ` [PATCH v9 1/6] notes.c: cleanup 'strbuf_grow' call in 'append_edit' Teng Long
2023-04-28 9:23 ` [PATCH v9 2/6] notes.c: use designated initializers for clarity Teng Long
2023-04-28 9:23 ` [PATCH v9 3/6] t3321: add test cases about the notes stripspace behavior Teng Long
2023-04-28 9:23 ` [PATCH v9 4/6] notes.c: introduce '--separator=<paragraph-break>' option Teng Long
2023-04-28 20:44 ` Junio C Hamano
2023-05-06 9:12 ` Teng Long
2023-05-06 9:22 ` Teng Long
2023-05-10 19:19 ` Kristoffer Haugsbakk
2023-05-12 4:07 ` Teng Long
2023-05-12 7:29 ` Kristoffer Haugsbakk
2023-05-16 17:00 ` Junio C Hamano
2023-05-17 3:58 ` Teng Long
2023-05-17 15:32 ` Junio C Hamano
2023-06-14 1:02 ` Junio C Hamano
2023-06-14 1:10 ` [PATCH] notes: do not access before the beginning of an array Junio C Hamano
2023-06-14 1:41 ` [PATCH v9 4/6] notes.c: introduce '--separator=<paragraph-break>' option Eric Sunshine
2023-06-14 2:07 ` Junio C Hamano
2023-06-15 7:13 ` Jeff King
2023-06-15 19:15 ` Junio C Hamano
2023-06-19 6:08 ` Teng Long
2023-06-20 20:36 ` Junio C Hamano
2023-06-21 2:50 ` Teng Long
2023-04-28 9:23 ` [PATCH v9 5/6] notes.c: append separator instead of insert by pos Teng Long
2023-04-28 9:23 ` [PATCH v9 6/6] notes.c: introduce "--[no-]stripspace" option Teng Long
2023-04-28 20:46 ` [PATCH v9 0/6] notes.c: introduce "--separator" option Junio C Hamano
2023-05-01 22:29 ` Junio C Hamano
2023-05-18 12:02 ` [PATCH v10 " Teng Long
2023-05-18 12:02 ` [PATCH v10 1/6] notes.c: cleanup 'strbuf_grow' call in 'append_edit' Teng Long
2023-05-18 12:02 ` [PATCH v10 2/6] notes.c: use designated initializers for clarity Teng Long
2023-05-18 12:02 ` [PATCH v10 3/6] t3321: add test cases about the notes stripspace behavior Teng Long
2023-05-18 12:02 ` [PATCH v10 4/6] notes.c: introduce '[--[no-]separator|--separator=<paragraph-break>]' option Teng Long
2023-05-18 14:34 ` Kristoffer Haugsbakk
2023-05-20 10:41 ` Teng Long
2023-05-20 16:12 ` Kristoffer Haugsbakk
2023-05-19 0:54 ` Jeff King
2023-05-27 7:17 ` Teng Long
2023-05-27 17:19 ` Jeff King
2023-05-29 11:48 ` Teng Long
2023-05-18 12:02 ` [PATCH v10 5/6] notes.c: append separator instead of insert by pos Teng Long
2023-05-18 12:02 ` [PATCH v10 6/6] notes.c: introduce "--[no-]stripspace" option Teng Long
2023-05-18 13:56 ` [PATCH v10 0/6] notes.c: introduce "--separator" option Kristoffer Haugsbakk
2023-05-20 10:22 ` Teng Long
2023-05-18 15:17 ` Junio C Hamano
2023-05-20 10:59 ` Teng Long
2023-05-27 7:57 ` [PATCH v11 0/7] notes.c: introduce "--separator" Teng Long
2023-05-27 7:57 ` [PATCH v11 1/7] notes.c: cleanup 'strbuf_grow' call in 'append_edit' Teng Long
2023-05-27 7:57 ` [PATCH v11 2/7] notes.c: use designated initializers for clarity Teng Long
2023-05-27 7:57 ` [PATCH v11 3/7] t3321: add test cases about the notes stripspace behavior Teng Long
2023-05-27 7:57 ` [PATCH v11 4/7] notes.c: introduce '--separator=<paragraph-break>' option Teng Long
2023-05-27 7:57 ` [PATCH v11 5/7] notes.c: append separator instead of insert by pos Teng Long
2023-05-27 7:57 ` [PATCH v11 6/7] notes.c: introduce "--[no-]stripspace" option Teng Long
2023-05-27 7:57 ` [PATCH v11 7/7] notes: introduce "--no-separator" option Teng Long
2023-06-01 5:50 ` [PATCH v11 0/7] notes.c: introduce "--separator" Junio C Hamano
2023-06-03 10:01 ` Teng Long
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqcz608cpk.fsf@gitster.g \
--to=gitster@pobox.com \
--cc=avarab@gmail.com \
--cc=dyroneteng@gmail.com \
--cc=git@vger.kernel.org \
--cc=sunshine@sunshineco.com \
--cc=tenglong.tl@alibaba-inc.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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 read-only IMAP folder(s) and NNTP newsgroup(s).