From: Eric Sunshine <sunshine@sunshineco.com>
Date: Wed, 18 Jan 2023 18:53:07 GMT
Subject: Re: [PATCH] git: replace strbuf_addstr with strbuf_addch for all strings of length 2
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:
> > 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.