Re: [PATCH 3/3] parse: replace atoi() with strtoul_ui() and strtol_i()

>
> On Sun, Oct 13, 2024 at 09:42:41AM +0000, Usman Akinyemi wrote:
> > On Sat, Oct 12, 2024 at 11:09 PM Usman Akinyemi via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> > >
> > > From: Usman Akinyemi <usmanakinyemi202@gmail.com>
> > >
> > > Replace unsafe uses of atoi() with strtoul_ui() for unsigned integers
> > > and strtol_i() for signed integers across multiple files. This change
> > > improves error handling and prevents potential integer overflow issues.
> > >
> > > The following files were updated:
> > > - daemon.c: Update parsing of --timeout, --init-timeout, and
> > >   --max-connections
> > > - imap-send.c: Improve parsing of UIDVALIDITY, UIDNEXT, APPENDUID, and
> > >   tags
> > > - merge-ll.c: Enhance parsing of marker size in ll_merge and
> > >   ll_merge_marker_size
>
> To me it's always an indicator that something should be split up across
> multiple commits once you have a bulleted list of changes in your commit
> message.
>
> > > This change allows for better error detection when parsing integer
> > > values from command-line arguments and IMAP responses, making the code
> > > more robust and secure.
> > >
> > > This is a #leftoverbit discussed here:
> > >  https://public-inbox.org/git/CAC4O8c-nuOTS=a0sVp1603KaM2bZjs+yNZzdAaa5CGTNGFE7hQ@mail.gmail.com/
> > >
> > > Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
> > >
> > > Cc: gitster@pobox.com
> > > Cc: Patrick Steinhardt <ps@pks.im>
> > > Cc: phillip.wood123@gmail.com
> > > Cc: Christian Couder <christian.couder@gmail.com>
> > > Cc: Eric Sunshine <sunshine@sunshineco.com>
> > > Cc: Taylor Blau <me@ttaylorr.com>
>
> The Cc annotations shouldn't be part of the commit message. If you want
> to Cc specific folks you should rather do it e.g. on the command line or
> whatever you use to send out the patches. Otherwise, these will all end
> up in our history.

>
> > > ---
> > >  daemon.c    | 14 +++++++++-----
> > >  imap-send.c | 13 ++++++++-----
> > >  merge-ll.c  |  6 ++----
> > >  3 files changed, 19 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/daemon.c b/daemon.c
> > > index cb946e3c95f..3fdb6e83c40 100644
> > > --- a/daemon.c
> > > +++ b/daemon.c
> > > @@ -1308,17 +1308,21 @@ int cmd_main(int argc, const char **argv)
> > >                         continue;
> > >                 }
> > >                 if (skip_prefix(arg, "--timeout=", &v)) {
> > > -                       timeout = atoi(v);
> > > +                       if (strtoul_ui(v, 10, &timeout) < 0) {
> > > +                               die("'%s': not a valid integer for --timeout", v);
> > > +                       }
> > >                         continue;
> > >                 }
>
> We don't use braces around single-line statements. It would also help to
> explain whether this is fixing a bug and, if it does, then it would be
> nice to have a testcase that demonstrates the behaviour. The same is
> true for the other sites that you convert.
>

> [snip]
> > I also want to ask if this is the right way to send another patch as I
> > noticed that it is showing my previous patch which is not related to
> > this. Thank you.
>
> You shouldn't ever include patches from another patch series. I guess
> tha problem here is that you created all of your work on the same
> branch. I'd recommend to use separate feature branches for every series
> you are working on. In general, these branches should start from the
> current "main" branch.
>
> Patrick