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

[Zejun Zhao <jelly.zhao.42@gmail.com>]

Thank you for reviewing this patch.

On Wed, Feb 05, 2025 08:47:06 +0100, Patrick Steinhardt wrote:
> You should split up this patch into a series, as it is really hard to
> follow what's going on. There are a couple of things happening:
> 
>   - You change types in `struct apply_state`, which bubbles up.
> 
>   - You adapt `git_hdr_len()` to receive different inputs, which bubbles
>     up.
> 
>   - You perform small fixes in several places.
> 
> It might also be a good idea to split out the loop counters into a
> separate commit, as those are trivially correct.

Sure I'll come up with a v2 patch series, in which each kind of fixes 
will be put in a single commit and I'll state why I believe the type 
cast/change is safe for every single fix in the commit message.

> > @@ -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;
> 
> The change to `hexsz` shouldn't be needed, even if it makes us match the
> type of `hexsz` as declared in `git_hash_algo`.

Yes it's not necessary here to change the type. And for the `hexsz` stuff,
on Wed, Feb 05 2025 04:58:57 -0800, Junio C Hamano wrote,
> 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?

I found the discussion mentioned at [1]. It seems like the change here 
only makes things worse so I'll see if I'd leave it untouched or change 
the type of `.hexsz` member to `int` or something.

[1] https://lore.kernel.org/git/xmqqttaqw2eb.fsf@gitster.g/

> > @@ -2185,7 +2182,7 @@ static int parse_chunk(struct apply_state *state, char *buffer, unsigned long si
> >  			};
> >  			int i;
> 
> This may arguably be `size_t`, as well.

It's OK for me to use a `size_t` loop count everywhere but I tried to 
keep the changes in this patch minimal (forget about the `hexsz` thing).
I could apply this change if you insist.

> > @@ -2257,12 +2255,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);
> >  
> >  	/*
> 
> Do we _know_ that `max` fits into an `int`?

Yep we've set an upper bound for `max` before:
> /*
>  * "scale" the filename
>  */
> max = state->max_len;
> if (max > 50)
> 	      max = 50;
so it must fit into an `int`.