| Message ID | 9a4506692239db52c2632346c079d9ac82946c8d.1676551077.git.dyroneteng@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | notes.c: introduce "--separator" option | expand |
Teng Long <dyroneteng@gmail.com> writes: > Let's cleanup the unnecessary 'strbuf_grow' call in 'append_edit'. This > "strbuf_grow(&d.buf, size + 1);" is prepared for insert a blank line if > needed, but actually when inserting, "strbuf_insertstr(&d.buf, 0, > "\n");" will do the "grow" for us. Correct. I think the code ends up trying to exactly size the strbuf as multiple "-m message" options are encountered on the command line, which is probably pointless (see below). > Best guess may be that the author originally inserted "\n" manually by > direct manipulation of the strbuf rather than employing a strbuf > function, but then switched to strbuf_insert() before submitting the > series and forgot to remove the now-unnecessary strbuf_grow(). Please do not speculate when you do not have to. "git blame" is your friend. 348f199b (builtin-notes: Refactor handling of -F option to allow combining -m and -F, 2010-02-13) added these to mimic the code introduced by 2347fae5 (builtin-notes: Add "append" subcommand for appending to note objects, 2010-02-13) that reads in previous note before the message. And the resulting code with explicit sizing is carried to this day. In the context of reading an existing note in, exact sizing may have made sense, but because the resulting note needs cleansing with stripspace() when appending with this option, such an exact sizing does not buy us all that much in practice. It may help avoiding overallocation due to ALLOC_GROW() slop, but nobody can feed so many long messages for it to matter from the command line. > Signed-off-by: Teng Long <dyroneteng@gmail.com> > Helped-by: Eric Sunshine <sunshine@sunshineco.com> > --- > builtin/notes.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/builtin/notes.c b/builtin/notes.c > index 80d9dfd2..23cb6f0d 100644 > --- a/builtin/notes.c > +++ b/builtin/notes.c > @@ -215,7 +215,6 @@ static int parse_msg_arg(const struct option *opt, const char *arg, int unset) > > BUG_ON_OPT_NEG(unset); > > - strbuf_grow(&d->buf, strlen(arg) + 2); > if (d->buf.len) > strbuf_addch(&d->buf, '\n'); > strbuf_addstr(&d->buf, arg); > @@ -618,7 +617,6 @@ static int append_edit(int argc, const char **argv, const char *prefix) > enum object_type type; > char *prev_buf = read_object_file(note, &type, &size); > > - strbuf_grow(&d.buf, size + 1); > if (d.buf.len && prev_buf && size) > strbuf_insertstr(&d.buf, 0, "\n"); > if (prev_buf && size)
Junio C Hamano <gitster@pobox.com> wrote on Thu, 16 Feb 2023 10:39:05 -0800: > > Best guess may be that the author originally inserted "\n" manually by > > direct manipulation of the strbuf rather than employing a strbuf > > function, but then switched to strbuf_insert() before submitting the > > series and forgot to remove the now-unnecessary strbuf_grow(). > > Please do not speculate when you do not have to. "git blame" is > your friend. My bad, I checked with "git blame" but did not describe in detail. > 348f199b (builtin-notes: Refactor handling of -F option to allow > combining -m and -F, 2010-02-13) added these to mimic the code > introduced by 2347fae5 (builtin-notes: Add "append" subcommand for > appending to note objects, 2010-02-13) that reads in previous note > before the message. And the resulting code with explicit sizing is > carried to this day. > > In the context of reading an existing note in, exact sizing may have > made sense, but because the resulting note needs cleansing with > stripspace() when appending with this option, such an exact sizing > does not buy us all that much in practice. > > It may help avoiding overallocation due to ALLOC_GROW() slop, but > nobody can feed so many long messages for it to matter from the > command line. Will apply by your content, thanks very much.
diff --git a/builtin/notes.c b/builtin/notes.c index 80d9dfd2..23cb6f0d 100644 --- a/builtin/notes.c +++ b/builtin/notes.c @@ -215,7 +215,6 @@ static int parse_msg_arg(const struct option *opt, const char *arg, int unset) BUG_ON_OPT_NEG(unset); - strbuf_grow(&d->buf, strlen(arg) + 2); if (d->buf.len) strbuf_addch(&d->buf, '\n'); strbuf_addstr(&d->buf, arg); @@ -618,7 +617,6 @@ static int append_edit(int argc, const char **argv, const char *prefix) enum object_type type; char *prev_buf = read_object_file(note, &type, &size); - strbuf_grow(&d.buf, size + 1); if (d.buf.len && prev_buf && size) strbuf_insertstr(&d.buf, 0, "\n"); if (prev_buf && size)
Let's cleanup the unnecessary 'strbuf_grow' call in 'append_edit'. This "strbuf_grow(&d.buf, size + 1);" is prepared for insert a blank line if needed, but actually when inserting, "strbuf_insertstr(&d.buf, 0, "\n");" will do the "grow" for us. Best guess may be that the author originally inserted "\n" manually by direct manipulation of the strbuf rather than employing a strbuf function, but then switched to strbuf_insert() before submitting the series and forgot to remove the now-unnecessary strbuf_grow(). Signed-off-by: Teng Long <dyroneteng@gmail.com> Helped-by: Eric Sunshine <sunshine@sunshineco.com> --- builtin/notes.c | 2 -- 1 file changed, 2 deletions(-)