Jonathan Nieder <[email protected]> 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().
