Re: [PATCH 1/4] add-patch: Fix type missmatch rom msvc

[Patrick Steinhardt <ps@pks.im>]

On Thu, Dec 26, 2024 at 01:33:12PM -0800, Junio C Hamano wrote:
> Sören Krecker <soekkle@freenet.de> writes:
> > @@ -1624,10 +1629,11 @@ static int patch_update_file(struct add_p_state *s,
> >  			else if (0 < response && response <= file_diff->hunk_nr)
> >  				hunk_index = response - 1;
> >  			else
> > -				err(s, Q_("Sorry, only %d hunk available.",
> > -					  "Sorry, only %d hunks available.",
> > -					  file_diff->hunk_nr),
> > -				    (int)file_diff->hunk_nr);
> > +				err(s,
> > +				    Q_("Sorry, only %"PRIuMAX" hunk available.",
> > +				       "Sorry, only %"PRIuMAX" hunks available.",
> > +				       (uintmax_t)file_diff->hunk_nr),
> > +				    (uintmax_t)file_diff->hunk_nr);
> 
> Again, this hunk may be needed, as the "hunk_nr" uses size_t, which
> probably is overly wide.
> 
> So, I do not mind too much to adjust the code around hunk_nr,
> hunk_alloc and other things that are already size_t (but before
> doing so, we probably should see if it makes more sense to use ulong
> for these members instead of size_t), hbut I am not sure if it is a
> sensible move to change old_offset, count, etc. that count in number
> of lines and use ulong (not bytes) to use size_t instead.
> 
> size_t _might_ be wider than some other forms of unsigned integers,
> but it is not necessarily the widest, so "because on msvc ulong is
> merely 32-bit and I want wider integer like everybody else!" is not
> a good excuse for such a change (if it were, we'd all coding with
> nothing but uintmax_t).
> 
> So I dunno.

In practice, the number of bytes and number of lines _can_ be the same
when the file consists of newlines, only. That is of course unlikely to
be the case in any sane input, but may be the case when processing input
that was specifically crafted to trigger such an edge case. So using a
type that can store the maximum size of a theoretically possible object
feels like a sensible safeguard to me and requires us to worry less
about such weird edge cases.

I also doubt that widening the type to `size_t` would have a meaningful
impact on performance, so I don't see a strong reason not to go there.
I tend to think that using `size_t` in such size-like-fields should be
our default unless there is a good reason not to pick it.

Patrick