On Tue, Jul 31, 2018 at 10:21 PM Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote: > > On Tue, Jul 31, 2018 at 1:37 PM Han-Wen Nienhuys <hanwen@xxxxxxxxxx> wrote: > > Highlight keywords in remote sideband output. > > Prefix with the module you're touching, don't capitalize, and drop the > period. Perhaps: Done. > sideband: highlight keywords in remote sideband output > > > The highlighting is done on the client-side. Supported keywords are > > "error", "warning", "hint" and "success". > > > > The colorization is controlled with the config setting "color.remote". > > What's the motivation for this change? The commit message should say > something about that and give an explanation of why this is done > client-side rather than server-side. Added > > Co-authored-by: Duy Nguyen <pclouds@xxxxxxxxx> > > Helped-by: is more typical. Done. > > Signed-off-by: Han-Wen Nienhuys <hanwen@xxxxxxxxxx> > > --- > > diff --git a/Documentation/config.txt b/Documentation/config.txt > > @@ -1229,6 +1229,15 @@ color.push:: > > +color.remote:: > > + A boolean to enable/disable colored remote output. If unset, > > + then the value of `color.ui` is used (`auto` by default). > > If this is "boolean", what does "auto" mean? Perhaps update the > description to better match other color-related options. All other doc entries say "boolean" here too. I'm happy to fix phrasing of this file in a follow-on change, but let's keep it like this for consistency. > > diff --git a/sideband.c b/sideband.c > > @@ -1,6 +1,97 @@ > > +void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n) > > +{ > > + int i; > > + > > + load_sideband_colors(); > > + if (!want_color_stderr(sideband_use_color)) { > > + strbuf_add(dest, src, n); > > + return; > > + } > > Can load_sideband_colors() be moved below the !want_color_stderr() conditional? Reorganized this a bit. > > + > > + while (isspace(*src)) { > > + strbuf_addch(dest, *src); > > + src++; > > + n--; > > + } > > + > > + for (i = 0; i < ARRAY_SIZE(keywords); i++) { > > + struct kwtable* p = keywords + i; > > + int len = strlen(p->keyword); > > Would it make sense to precompute each keyword length so you don't > have to recompute them repeatedly, or is that premature optimization? That is premature optimization. The next line does a strncasecmp on the same data so the cost (loading the keywords into CPU cache) is the same, while precomputing the length makes the code more error prone. > > + if (!strncasecmp(p->keyword, src, len) && !isalnum(src[len])) { > > So, the strncasecmp() is checking if one of the recognized keywords is > at the 'src' position, and the !isalnum() ensures that you won't pick > up something of which the keyword is merely a prefix. For instance, > you won't mistakenly highlight "successful". It also works correctly > when 'len' happens to reference the end-of-string NUL. Okay. added comment. > > + strbuf_addstr(dest, p->color); > > + strbuf_add(dest, src, len); > > + strbuf_addstr(dest, GIT_COLOR_RESET); > > + n -= len; > > + src += len; > > + break; > > + } > > So, despite the explanation in the commit message, this function isn't > _generally_ highlighting keywords in the sideband. Instead, it is > highlighting a keyword only if it finds it at the start of string > (ignoring whitespace). Perhaps the commit message could be more clear > about that. updated message. > A natural follow-on question is whether strings are fed to this > function one line at a time or if the incoming string may have > embedded newlines (in which case, you might need to find a prefix > following a newline, as well?). added comment. -- Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Geschäftsführer: Paul Manicle, Halimah DeLaine Prado