Re: [PATCH] git: replace strbuf_addstr with strbuf_addch for all strings of length 2
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Jan 18, 2023, 18:53 UTC
- Message-ID
- <CAPig+cTrc7V+P6h039EsPwwRE4rw-qYGKHgGnGmp0TohfzHykA@mail.gmail.com>
- In-Reply-To
- <xmqqr0vr6d80.fsf@gitster.g>
On Wed, Jan 18, 2023 at 11:14 AM Junio C Hamano <gitster@pobox.com> wrote:
Show 15 quoted lines
> > From: Seija Kijin <doremylover123@gmail.com> > > This helps reduce overhead of calculating the length > > > diff --git a/builtin/am.c b/builtin/am.c > > strbuf_addstr(&sb, "GIT_AUTHOR_NAME="); > > sq_quote_buf(&sb, state->author_name); > > - strbuf_addch(&sb, '\n'); > > > > - strbuf_addstr(&sb, "GIT_AUTHOR_EMAIL="); > > + strbuf_addstr(&sb, "\nGIT_AUTHOR_EMAIL="); > > This may reduce the number of lines, but markedly worsens the > readability of the resulting code. Each of the three-line blocks in > the original used to be logically complete and independent unit, but > now each of them depend on what the last block wants.
Very much agree with this and all your other review comments.
Show 7 quoted lines
> > - strbuf_addchars(dest, ' ', 2); > > - strbuf_addstr(dest, "From inner merge:"); > > + strbuf_addstr(dest, " From inner merge:"); > > strbuf_addchars(dest, ' ', opt->priv->call_depth * 2); > > Ditto, even though this is not as horrible as the change to builtin/am.c > we saw earlier.
Additionally, if this literal string ever gets wrapped in `_(...)`, then the above change is even more undesirable due to the extra burden it places on translators.