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 v7 3/4] notes.c: introduce '--separator=<paragraph-break>' option
Date: Tue, 28 Mar 2023 08:37:58 -0700 [thread overview]
Message-ID: <xmqq5yakhoo9.fsf@gitster.g> (raw)
In-Reply-To: <d1febf86d8471c24c9a045d6962fd86f63f414d5.1680012650.git.dyroneteng@gmail.com> (Teng Long's message of "Tue, 28 Mar 2023 22:28:46 +0800")
Teng Long <dyroneteng@gmail.com> writes:
> From: Teng Long <dyroneteng@gmail.com>
>
> When adding new notes or appending to an existing notes, we will
> insert a blank line between the paragraphs, like:
>
> $ git notes add -m foo -m bar
> $ git notes show HEAD | cat
> foo
>
> bar
>
> The default behavour sometimes is not enough, the user may want
> to use a custom delimiter between paragraphs, like when
> specifiy '-m', '-F', '-C', '-c' options. So this commit
"like when specifying", you mean?
> introduce a new '--separator' option for 'git notes add' and
> 'git notes append', for example when execute:
"when executing"?
> $ git notes add -m foo -m bar --separator="-"
> $ git notes show HEAD | cat
> foo
> -
> bar
>
> A newline is added to the value given to --separator if it
"a newline is ...", because this is not starting a new sentence, but
continuing from the "for example, when ..." above.
> 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"
>
> Running A and B produces the same result.
Here, it is totally unclear what A and B refers to. "both
... and ... produce the same result" or something?
> @@ -85,8 +87,12 @@ corresponding <to-object>. (The optional `<rest>` is ignored so that
> 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.
> + 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.
Excellent.
> @@ -101,6 +102,9 @@ struct note_data {
> int use_editor;
> char *edit_path;
> struct strbuf buf;
> + struct strbuf **messages;
> + size_t msg_nr;
> + size_t msg_alloc;
> };
Hmph, OK. I'd think twice before allowing an array of strbufs,
though. strbuf is an excellent data structure to allow editing
string safely, and an array of strbufs may be very useful when these
strings need to be edited simultaneously, but such a use case is
very rare and I suspect this would not be it---rather, don't we take
one string at a time while processing each option, perhaps running
stripspace, and then after that aren't we done with the string until
when we finally have these N strings to loop over and concatenate
into a single string? It would be sensible to use a strbuf to
produce the concatenation, but the strings to be concatenated do not
have to come from strbufs. So my intuition, before reading the code
but after seeing the data structure, says that "const char **messages"
might be more appropriate here. It sends a strong message about the
code structure and "phases" of processing, i.e. "we read each piece
and store it in the array once we are done preprocessing; then in
the next phase we concatenate them".
> @@ -209,71 +213,110 @@ 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");
> +}
Do we need "insert" separator? The caller in "concat" certainly
shouldn't---all it needs is "append".
> +/* Consume messages and append them into d->buf, then free them */
> +static void concat_messages(struct note_data *d)
> +{
> + struct strbuf msg = STRBUF_INIT;
> +
> + size_t i;
> + for (i = 0; i < d->msg_nr ; i++) {
> + if (d->buf.len)
> + insert_separator(&d->buf, d->buf.len);
> + strbuf_add(&msg, d->messages[i]->buf, d->messages[i]->len);
> + strbuf_addbuf(&d->buf, &msg);
> + strbuf_reset(&msg);
> + strbuf_release(d->messages[i]);
> + free(d->messages[i]);
> + }
> + strbuf_release(&msg);
> + free(d->messages);
> +}
As I suspected earlier, with the way d->messages[i] are used, they
have no reason to be strbufs---they can and should be a simple
string "const char *".
> static int parse_msg_arg(const struct option *opt, const char *arg, int unset)
> {
> struct note_data *d = opt->value;
> + struct strbuf *buf;
>
> BUG_ON_OPT_NEG(unset);
>
> - if (d->buf.len)
> - strbuf_addch(&d->buf, '\n');
> - strbuf_addstr(&d->buf, arg);
> - strbuf_stripspace(&d->buf, 0);
> + buf = xmalloc(sizeof(*buf));
> + strbuf_init(buf, strlen(arg));
> + strbuf_addstr(buf, arg);
> + strbuf_stripspace(buf, 0);
>
> - d->given = 1;
> + ALLOC_GROW_BY(d->messages, d->msg_nr, 1, d->msg_alloc);
> + d->messages[d->msg_nr - 1] = buf;
> return 0;
> }
And this one can use an on-stack strbuf (initialized with STRBUF_INIT),
and strbuf_detach() the result into d->messages[].
> static int parse_file_arg(const struct option *opt, const char *arg, int unset)
> {
> struct note_data *d = opt->value;
> + struct strbuf *buf;
Likewise.
>
> BUG_ON_OPT_NEG(unset);
>
> - if (d->buf.len)
> - strbuf_addch(&d->buf, '\n');
> + buf = xmalloc(sizeof(*buf));
> + strbuf_init(buf, 0);
> +
> 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);
> + strbuf_stripspace(buf, 0);
>
> - d->given = 1;
> + // we will note stripspace the buf here, because the file maybe
> + // is a binary and it maybe contains multiple continuous line breaks.
No // comments in our codebase.
> + ALLOC_GROW_BY(d->messages, d->msg_nr, 1, d->msg_alloc);
> + d->messages[d->msg_nr - 1] = buf;
> return 0;
> }
> static int parse_reuse_arg(const struct option *opt, const char *arg, int unset)
> {
> ...
> }
Ditto.
> @@ -567,7 +617,8 @@ static int append_edit(int argc, const char **argv, const char *prefix)
> const struct object_id *note;
> char *logmsg;
> const char * const *usage;
> - struct note_data d = { .buf = STRBUF_INIT };
> + struct note_data d = { .buf = STRBUF_INIT, .messages = NULL };
Why explicitly initialize .messages to NULL when we are leaving
other members to zero initialized implicitly by using designated
initializers?
> @@ -618,7 +675,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);
Having to insert two things in front of d.buf feels somewhat
wasteful. I wonder if we can easily reorder the logic to first read
the previous one, and then append new stuff to it, perhaps?
It wouldn't be a huge deal. If this weren't the only reason why we
need to invent insertstr that takes "where", I wouldn't even be
mentioning it.
> diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
> index 3288aaec..716192b5 100755
> --- a/t/t3301-notes.sh
> +++ b/t/t3301-notes.sh
> @@ -5,6 +5,7 @@
>
> test_description='Test commit notes'
>
> +TEST_PASSES_SANITIZE_LEAK=true
> . ./test-lib.sh
This is a strange commit to add this. If the test and the code
involved in the test were leak-sanitizer clean before this commit,
then you should have been able to insert this without any other
change, and we should do it that way in a separate commit. If we
are fixing an existing leak that the sanitizer complains, then that
is a different matter. If that is the case, it makes perfect sense
to have this here, but the proposed commit log message should
explain that it is what is happening.
Other than that, looks very nicely done.
Thanks.
next prev parent reply other threads:[~2023-03-28 15:39 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
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 [this message]
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=xmqq5yakhoo9.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).