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