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

[Junio C Hamano <gitster@pobox.com>]

Jonathan Nieder <jrnieder@gmail.com> writes:

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

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

>> @@ -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().