Re: [GSOC][PATCH v2 1/6] apply: change fields in `apply_state` to unsigned

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

Karthik Nayak <karthik.188@gmail.com> writes:

> Zejun Zhao <jelly.zhao.42@gmail.com> writes:
>
>> `.max_change` and `.max_len` of `apply_state` are only used as unsigned
>> integers. Misuse of `int` type would cause -Wsign-comparison warnings.
>>
>> Fix this by
>>
>>   - change `.max_change`'s type to `unsigned` since it's just a counter
>>
>
> Looking at `.max_change` it seems like this is only assigned in
> `patch_stats()` where we do
>
>   int lines = patch->lines_added + patch->lines_deleted;
>
>   if (lines > state->max_change)
>      state->max_change = lines;
>
> In this case shouldn't we first convert `.lines_added` `.lines_deleted`
> to also be 'unsigned int' in the first place?

Surely.  Or if any of the internal API uses a calling convention
that yields number of lines on success and negative number to signal
errors, we could also unify them to signed integer instead.  In
either case, using types consistently is good, and thanks for sharp
eyes spotting this instance.

>> @@ -2257,12 +2258,12 @@ static void show_stats(struct apply_state *state, struct patch *patch)
>>  	}
>>
>>  	if (patch->is_binary) {
>> -		printf(" %-*s |  Bin\n", max, qname.buf);
>> +		printf(" %-*s |  Bin\n", (int) max, qname.buf);
>>  		strbuf_release(&qname);
>>  		return;
>>  	}
>>
>> -	printf(" %-*s |", max, qname.buf);
>> +	printf(" %-*s |", (int) max, qname.buf);
>>  	strbuf_release(&qname);
>>

This is the kind of fallout that makes the resulting code harder to
read.  How bad would the code churn be if we instead unify to the
signed integer type, instead of using size_t, and making sure we
use the range-checking versions of arithmetic when needed, I have to
wonder?