| Message ID | f00a7596587bbf2d055dbf77a19506be1a6350fd.1673490953.git.dyroneteng@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | notes.c: introduce "--separator" optio | expand |
On Wed, Jan 11, 2023 at 9:48 PM Teng Long <dyroneteng@gmail.com> wrote: > 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. > > Signed-off-by: Teng Long <dyroneteng@gmail.com> > --- > diff --git a/builtin/notes.c b/builtin/notes.c > @@ -618,7 +618,6 @@ static int append_edit(int argc, const char **argv, const char *prefix) > 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"); Indeed, it's not clear why that was there in the first place. Digging through history doesn't shed any light on it. It was introduced by 2347fae50b (builtin-notes: Add "append" subcommand for appending to note objects, 2010-02-13)[1], but there's no explanation as to why it was coded that way. 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(). [1]: https://lore.kernel.org/git/1266096518-2104-26-git-send-email-johan@herland.net/
Eric Sunshine <sunshine@sunshineco.com> writes: > On Wed, Jan 11, 2023 at 9:48 PM Teng Long <dyroneteng@gmail.com> wrote: > > 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. > > > > Signed-off-by: Teng Long <dyroneteng@gmail.com> > > --- > > diff --git a/builtin/notes.c b/builtin/notes.c > > @@ -618,7 +618,6 @@ static int append_edit(int argc, const char **argv, const char *prefix) > > 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"); > > Indeed, it's not clear why that was there in the first place. Digging > through history doesn't shed any light on it. It was introduced by > 2347fae50b (builtin-notes: Add "append" subcommand for appending to > note objects, 2010-02-13)[1], but there's no explanation as to why it > was coded that way. 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(). Yes, I have the same opinion with you, maybe original idea to savesome array creation overhead? I'm not sure, but I think the deletion here is OK. Thanks.
diff --git a/builtin/notes.c b/builtin/notes.c index 80d9dfd25c..e57f024824 100644 --- a/builtin/notes.c +++ b/builtin/notes.c @@ -618,7 +618,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)