Re: [GSOC][PATCH] apply: address -Wsign-comparison warnings

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

> @@ -1087,11 +1086,11 @@ static int gitdiff_index(struct gitdiff_data *state,
>  	 * and optional space with octal mode.
>  	 */
>  	const char *ptr, *eol;
> -	int len;
> -	const unsigned hexsz = the_hash_algo->hexsz;
> +	size_t len;
> +	const size_t hexsz = the_hash_algo->hexsz;

I thought that I already saw this discussed in another thread.

The .hexsz of any hash algorithm would never be larger than what a
platform natural "unsigned" integer type can hold, so using size_t
for the member _is_ the wrong thing to do and the fix may be the
other way around, no?

There are genuinely good changes (correcting assigned variable from
int to size_t when the value ultimately came from a system function
that returns size_t) in this patch, but there are other annoying
ones like these, so I am not sure.

>  	ptr = strchr(line, '.');
> -	if (!ptr || ptr[1] != '.' || hexsz < ptr - line)
> +	if (!ptr || ptr[1] != '.' || hexsz < (size_t) (ptr - line))

Is this about -Wsign-compare complaining about size_t vs ptrdiff_t?

> @@ -1207,7 +1206,7 @@ static char *git_header_name(int p_value,
>  		cp = skip_tree_prefix(p_value, second, line + llen - second);
>  		if (!cp)
>  			goto free_and_fail1;
> -		if (line + llen - cp != first.len ||
> +		if ((size_t) (line + llen - cp) != first.len ||

Ditto.

> -			if (len < second - name &&
> +			if (len < (size_t) (second - name) &&

Ditto.

I said this before, but I am not sure if being strict about
"-Wsign-compare" is really buying us much.  If we are getting so
many false positives that need to be squelched by churning the code
with so many typecasts in order to find a new and real problem,
is it really worth it?