Re: [PATCH 2/2] diff-filter: be more careful when looking for negative bits

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 26, 2022, 23:36 UTC
Message-ID: <xmqqtudqnk7y.fsf@gitster.g>
In-Reply-To: <YfHRTrdwZVwxcPBK@nand.local>

Taylor Blau <me@ttaylorr.com> writes:

Show 15 quoted lines

>> +		int has_positive = 0;
>> +		int has_negative = 0;
>> +
>> +		for (i = 0; (optch = optarg[i]) != '\0'; i++) {
>> +			if (optch < 'a' || 'z' < optch)
>> +				has_positive++;
>> +			else
>> +				has_negative++;
>> +		}
>> +
>> +		if (!has_positive && has_negative) {
>
> (One small nit that I absolutely do not care about is using a variable
> that starts with 'has_'--which I would expect to be a boolean--to count
> the number of positive/negative filters. Perhaps calling these

These variables are indeed used in the boolean sense (see how they are used in the condition in the "if" statement).

If we wanted to short-circuit, we could do this:

	for (i = 0;
	     !has_positive && !has_negative &&
	     (optch = optarg[i]) != '\0';
	     i++) {
		if (isupper(optch))
			has_negative++;
		else
			has_positive++;
	}

and thanks to their names, nobody would be confused to find it is a bug that these do not count when the loop exits early. We shouldn't name these positive_nr or negative_nr because we are not interested in counting them.

It is just that "bool++" is more idiomatic than repeated assignment "bool = 1" when setter and getter both know it is merely used as a Boolean, and that is why they named as has_X, which clearly is a name for a Boolean, not a counter.

Having said that, I do not mind if an assignment is used instead of post-increment in new code. I just think it is waste of time to go find increments that toggle a Boolean to true and changing them to assignments, and it is even more waste having to review such a code churn, so let's not go there ;-)

But as I wrote in the big NEEDSWORK comment, this loop should disappear when the code is properly rewritten to correct the interaction between two or more "--diff-filter=" options, so it would not matter too much.

Thanks.