On Wed, Jan 18, 2023 at 11:14 AM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > From: Seija Kijin <doremylover123@xxxxxxxxx> > > 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.
