On Mon, Oct 14, 2024 at 4:13 PM Usman Akinyemi <usmanakinyemi202@xxxxxxxxx> wrote: > > 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. Also, do I need to add the reference which mentions the leftoverbit in the commit message?
