Re: [PATCH] git: replace strbuf_addstr with strbuf_addch for all strings of length 2

[Eric Sunshine <sunshine@sunshineco.com>]

On Wed, Jan 18, 2023 at 11:14 AM Junio C Hamano <gitster@pobox.com> wrote:
> > 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.

> > -             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.