On Mon, Oct 14, 2024 at 10:53 AM Patrick Steinhardt <ps@xxxxxx> wrote: > > 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@xxxxxxxxx> wrote: > > > > > > From: Usman Akinyemi <usmanakinyemi202@xxxxxxxxx> > > > > > > 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@xxxxxxxxxxxxxx/ > > > > > > Signed-off-by: Usman Akinyemi <usmanakinyemi202@xxxxxxxxx> > > > > > > Cc: gitster@xxxxxxxxx > > > Cc: Patrick Steinhardt <ps@xxxxxx> > > > Cc: phillip.wood123@xxxxxxxxx > > > Cc: Christian Couder <christian.couder@xxxxxxxxx> > > > Cc: Eric Sunshine <sunshine@xxxxxxxxxxxxxx> > > > Cc: Taylor Blau <me@xxxxxxxxxxxx> > > 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. Thanks for this, I thought the gitgitgadget automatically use the Cc from the commit message. > > > > --- > > > 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. > I was going to add testcase, I sent this patch to ensure I am going in the right direction. > [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 Thanks Patrick. I actually created a new branch for this branch, my mistake was not basing it on the master branch. I was a little bit confused. But, now I understand better. Thanks.
