On Mon, Oct 14, 2024 at 2:55 PM Phillip Wood [off-list ref] wrote:

On 14/10/2024 15:00, Patrick Steinhardt wrote:

quoted

On Mon, Oct 14, 2024 at 02:57:13PM +0100, Phillip Wood wrote:

quoted

On 14/10/2024 11:53, Patrick Steinhardt wrote:

quoted

On Sun, Oct 13, 2024 at 09:42:41AM +0000, Usman Akinyemi wrote:

quoted

On Sat, Oct 12, 2024 at 11:09 PM Usman Akinyemi via GitGitGadget
[off-list ref] wrote:

quoted

From: Usman Akinyemi <redacted>

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.

quoted

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

quoted

Patrick

I got this from a leftoverbit which the main issue was reported as
bug. https://public-inbox.org/git/CAC4O8c-nuOTS=a0sVp1603KaM2bZjs+yNZzdAaa5CGTNGFE7hQ@mail.gmail.com/

For the test, I should have the test as another patch right ?
Thanks.