From: Teng Long <dyroneteng@gmail.com>
To: gitster@pobox.com
Cc: avarab@gmail.com, dyroneteng@gmail.com, git@vger.kernel.org,
sunshine@sunshineco.com, tenglong.tl@alibaba-inc.com
Subject: Re: [PATCH v8 4/6] notes.c: introduce '--separator=<paragraph-break>' option
Date: Thu, 27 Apr 2023 15:21:07 +0800 [thread overview]
Message-ID: <20230427072107.29821-1-tenglong.tl@alibaba-inc.com> (raw)
In-Reply-To: <xmqqsfcn27e0.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
>> + while (d->msg_nr) {
>> + --d->msg_nr;
>> + strbuf_release(&d->messages[d->msg_nr]->buf);
>> + free(d->messages[d->msg_nr]);
>> + }
>
> while (d->msg_nr--) {
> strbuf_relesae(...);
> free(d->messages[d->msg_nr]);
> }
Will fix or optimize.
>> +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.buf, d->messages[i]->buf.len);
>> + strbuf_addbuf(&d->buf, &msg);
>> + if (d->messages[i]->stripspace)
>> + strbuf_stripspace(&d->buf, 0);
>> + strbuf_reset(&msg);
>> + }
>> + strbuf_release(&msg);
>> +}
>
>Interesting.
>
>I would have expected that where we add to d->messages[] array a new
>note_msg structure and set its .stripspace member, we already knew
>if the associated strbuf needs to be stripspace'd and instead of
>maintaining the .stripspace member for each note_msg, we can just
>have it contain only a string (not even a strbuf).
That's because the "-C" option, it will not do stripspace for the
whole note, but support to use together with "-m" or "-F"(they will
do stripspace). So, for the consistent result with backforward
compatibility, we have to get the information about whether to do
stripspace when concating each message.
>The above loop, and the design to have .stripspace per each
>note_msg, smell iffy to me for one thing. The .stripspace member is
>used to flag "after adding this element, the whole thing need to be
>treated with stripspace". Is it intended? What it means is that if
>you have two elements in d->messages[] array, one of them with and
>the other one without the .stripspace bit set, depending on the
>order of the elements, the result would be vastly different. When
>the later element has the stripspace bit set, everything gets
>cleansed, but when the earlier element has it, only the earlier
>element gets cleansed and the later element is added with multiple
>blank lines and trailing whitespaces intact. It does not sound
>quite right.
In the previous patch, I actually followed your suggestion to only
stripspace message itself but not the whole concatenate message. But
actually, there's a problem with this, in fact it's also caused by
"-C", "-C" doesn't stripspace, means that when mixed with "-m" and
"-F", the order of the options matters.
I'm not sure if this is a design flaw or if it's intentional, but
it's special about the stripspace handling of "-C" right now, which
is why I've added two new test cases in [3/6], just to make sure my
subsequent patches don't break them:
* 'add notes with "-C" and "-m", "-m" will stripspace all together'
* 'add notes with "-m" and "-C", "-C" will not stripspace all together'
>I wonder if the semantics intended (not implemented) by this series
>is to concatenate if none of the elements want stripspace and cleanse
>the whitespaces if any of them want stripspace, in which case an
>obvious implementation would be to just concatenate everything in
>the loop with separators, while seeing if any of them have the
>stripspace bit set, and then after the loop finishes, run stripspace
>on the whole thing just once if any of d->messages[] asked for it?
>If that is the case, an even better design could be to move the
>stripspace bit out of d->messages[] and make it d->stripspace to
>apply to the whole thing.
I agree, but this seems to break the effect of "the order matters" because of
the "-C" argument.
If we think that the "-C" particularity is a case that should be optimized or
fixed(), then I strongly agree with this modification approach, and I think
that compared to this particularity, on the one hand, our implementation
is simple, on the other hand, users may find it easier to understand.
>Another possiblity is to just stripspace the elements taken from the
>source you want to stripspace (like "-m" and "-F" but not the ones
>taken from parse_reuse_arg()) before you even add them to
>d->messages[] array. That way, the only possible source of multiple
>blank lines and trailing whitespaces in the concatenation of
>elements you want stripspace would be from the separator, which is
>under control by the end user. ...
I think this approach is feasible, and add judgment logic when supporting
subsequent --[no-] stripspaces.
>... So your concatenation could just
>ignore running stripspace at all---if the user does (or does not)
>want multiple blank lines or trailing whitespaces on the separator
>line, that's under their control. That sounds like the simplest and
>cleanest design---we strip as we read from each source and make them
>into simple strings to be kept in d->messages[] without having to
>allocate and keep a strbuf per each source, and we concatenate just
>once, without worrying about stripspace.
The strbuf can record the "len" of the buf, to make sure the string
will be cut of if the content contains '\0' in the middle, like a binary
file. Maybe using a string could make that, but I‘m not sure how to solve
it.
>The simple approach may break when any of the elements ended up to
>be truly empty, but then we can solve that by refraining to push an
>empty result into d->messages[] array, or something? I dunno.
I will add some test cases about it and found it out.
Thanks.
next prev parent reply other threads:[~2023-04-27 7:22 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
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 [this message]
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=20230427072107.29821-1-tenglong.tl@alibaba-inc.com \
--to=dyroneteng@gmail.com \
--cc=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--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).