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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 18, 2018, 14:40 UTC
Message-ID: <xmqqmutjagc7.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180818060911.GC241538@aiede.svl.corp.google.com>

Jonathan Nieder <jrnieder@gmail.com> writes:

Show 8 quoted lines

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

Show 10 quoted lines

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

Show 9 quoted lines

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