Re: Re* [PATCH v7 1/1] sideband: highlight keywords in remote sideband output

Jonathan Nieder [off-list ref] writes:

quoted

--- a/sideband.c
+++ b/sideband.c

@@ -75,7 +75,7 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)

Not about this patch: should the 'n' parameter be a size_t instead of
an int?  It doesn't matter in practice (since the caller has an int,
it can never be more than INT_MAX) but it might make the intent
clearer.

I tend to agree, but I think a separate "clean-up" patch to do so is
more appropriate than rolling it into this fix.

quoted

 		/*
 		 * Match case insensitively, so we colorize output from existing
 		 * servers regardless of the case that they use for their
		 * messages. We only highlight the word precisely, so
		 * "successful" stays uncolored.
		 */
		if (!strncasecmp(p->keyword, src, len) && !isalnum(src[len])) {

Not about this patch: should this check "&& src[len] == ':'" instead,
as discussed upthread?

I originally was of an opinion that we should take only lowercase
keyword followed by a colon, primarily because that is what we
produce.  Then "the real world need" told us that we are better off
catching the keyword case-insensitively.  Recalling that lesson, I
am not sure I would support "let's limit to the colon, rejecting any
other punctionation letter".

In any case, we should make such a policy decision outside a patch
like this one that is about fixing a behaviour which all users would
consider as a bug regardless of the policy they support.

quoted

@@ -100,8 +103,8 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
 		}
 	}
 
-	strbuf_add(dest, src, n);
+	if (0 < n)
+		strbuf_add(dest, src, n);

This check seems unnecessary.  strbuf_add can cope fine with !n.

I was primarily interested in catching negatives, and !n was a mere
optimization, but you are correct to point out that negative n at
this point in the codeflow is a BUG().