On Mon, Oct 14, 2024 at 2:55 PM Phillip Wood <phillip.wood123@xxxxxxxxx> wrote: > > On 14/10/2024 15:00, Patrick Steinhardt wrote: > > On Mon, Oct 14, 2024 at 02:57:13PM +0100, Phillip Wood wrote: > >> On 14/10/2024 11:53, Patrick Steinhardt 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. > >> > >> Agreed, but I think in this case there is a common theme (converting atoi() > >> to a safer alternative) and the problem is with the commit message listing > >> which files have changed rather than unrelated code changes being grouped > >> together. This patch could be split up and if there were many more atoi() > >> conversions it would need to be split to prevent it being too long but I > >> don't think its essential to do so. > > > > In theory I agree. In practice I think we should have better > > explanations why the respective conversions are fine and whether this is > > fixing a bug or not. And if it is fixing bugs I'd also like to see tests > > added to the tree. > > I'm not sure if I would describe any of the changes as fixing bugs. The > option and config parsing code becomes stricter so I guess you could say > it was a bug to accept any old rubbish and treat it as zero before. The > imap code that's changed all rejected zero anyway apart from the tag > parsing so maybe accepting the changes to the tag parsing are fixing a bug. > > > And by the time we got there it makes sense to split up commits. > > Yes if we start adding tests then it is worth splitting them up, I'm not > sure we have anyway of testing the imap changes but it would be worth > testing the other changes though. > > Phillip > > > Patrick > > > I got this from a leftoverbit which the main issue was reported as bug. https://public-inbox.org/git/CAC4O8c-nuOTS=a0sVp1603KaM2bZjs+yNZzdAaa5CGTNGFE7hQ@xxxxxxxxxxxxxx/ For the test, I should have the test as another patch right ? Thanks.
