From: Samuel Abraham <abrahamadekunle50@gmail.com>
Date: Sat, 14 Feb 2026 10:14:08 GMT
Subject: Re: [PATCH v4 2/4] add-patch: modify patch_update_file() signature
Message-ID: <CADYq+fa=-V9_gTpPRUvCDwFDShrUuxBqojOM+JSo_AvfvAJR7Q@mail.gmail.com>
In-Reply-To: <xmqqms1ci5g8.fsf@gitster.g>

On Sat, Feb 14, 2026 at 12:34 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Abraham Samuel Adekunle <abrahamadekunle50@gmail.com> writes:
>
> > -static int patch_update_file(struct add_p_state *s,
> > -                          struct file_diff *file_diff)
> > +static ssize_t patch_update_file(struct add_p_state *s, size_t idx)
>
> Why ssize_t?  Are we going to handle that many hunks that we do not
> expect to fit in a platform natural "int" type?  If we are not doing
> anything about "idx" being more than half the type, which apparently
> is the case ...
>
> >  {
> >       size_t hunk_index = 0;
> >       ssize_t i, undecided_previous, undecided_next, rendered_hunk_index = -1;
> >       struct hunk *hunk;
> >       char ch;
> >       struct child_process cp = CHILD_PROCESS_INIT;
> > -     int colored = !!s->colored.len, quit = 0, use_pager = 0;
> > +     int colored = !!s->colored.len, use_pager = 0;
> >       enum prompt_mode_type prompt_mode_type;
> > +     struct file_diff *file_diff = s->file_diff + idx;
> > +     ssize_t patch_update_resp = (ssize_t)idx;
>
> ... with the cast that is not checked here, wouldn't it make sense
> to just use the platform natural "int" everywhere?  Your code is not
> "safe" either way.  I do not think we expect to handle 2 billion
> hunks, so even on 32-bit platforms, platform natural "int" should be
> plenty.  Instead of religiously using size_t and ssize_t to count
> things without extra care, I'd rather see us check the error
> condition for real, if that is what we really care about (and that
> can still be done while leaving the codebase cleaner by sticking to
> the platform natural "int").
>
> Enough ranting.  Anyway.
>
> If we really are bothered that we cannot handle 3 billion hunks, we
> could avoid losing half the number range by returning
> s->file_diff.file_diff_nr (which is one more than there are elements
> in s->file_diff[] array) or ((size_t)-1).  That would allow us to
> return size_t from here.  I care about this a bit more than "why use
> size_t when int is perfectly fine", because some platforms that are
> not quite POSIX can have ssize_t that is not as wide as size_t.

Hello Junio.
Thank you for the review.

I wanted to be able to return a negative value while also making sure to
return a type of the same size as "i" since that is what we use to
update the caller
for the next or previous "i".
But I know better now to think about platforms that are not quite POSIX.
I will return the s->file_diff_nr and check for that instead.

Thanks
Abraham