git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Patrick Steinhardt <ps@pks.im>
To: phillip.wood@dunelm.org.uk
Cc: Usman Akinyemi <usmanakinyemi202@gmail.com>,
	Usman Akinyemi via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org
Subject: Re: [PATCH 3/3] parse: replace atoi() with strtoul_ui() and strtol_i()
Date: Mon, 14 Oct 2024 16:00:56 +0200	[thread overview]
Message-ID: <Zw0kGLZ-mcYjb6Je@pks.im> (raw)
In-Reply-To: <2a937b6f-a3fb-4f2a-997b-5508f0e20e65@gmail.com>

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@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.
> 
> 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.

And by the time we got there it makes sense to split up commits.

Patrick

  reply	other threads:[~2024-10-14 14:01 UTC|newest]

Thread overview: 94+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-10-12 23:09 [PATCH 0/3] R atoi Usman Akinyemi via GitGitGadget
2024-10-12 23:09 ` [PATCH 1/3] t3404: avoid losing exit status with focus on `git show` and `git cat-file` Usman Akinyemi via GitGitGadget
2024-10-14 21:29   ` Taylor Blau
2024-10-12 23:09 ` [PATCH 2/3] t3404: replace test with test_line_count() Usman Akinyemi via GitGitGadget
2024-10-14 21:35   ` Taylor Blau
2024-10-12 23:09 ` [PATCH 3/3] parse: replace atoi() with strtoul_ui() and strtol_i() Usman Akinyemi via GitGitGadget
2024-10-13  9:42   ` Usman Akinyemi
2024-10-14  9:00     ` Phillip Wood
2024-10-14 15:56       ` Usman Akinyemi
2024-10-14 10:53     ` Patrick Steinhardt
2024-10-14 13:57       ` Phillip Wood
2024-10-14 14:00         ` Patrick Steinhardt [this message]
2024-10-14 14:55           ` Phillip Wood
2024-10-14 16:13             ` Usman Akinyemi
2024-10-14 16:26               ` Usman Akinyemi
2024-10-14 18:36                 ` phillip.wood123
2024-10-15 15:17                   ` Usman Akinyemi
2024-10-15 16:19                     ` Taylor Blau
2024-10-16 17:58                       ` Usman Akinyemi
2024-10-15 18:28                     ` phillip.wood123
2024-10-16  9:20                       ` Phillip Wood
2024-10-16 18:00                         ` Usman Akinyemi
2024-10-17 11:56                   ` Usman Akinyemi
2024-10-17 12:02                     ` Patrick Steinhardt
2024-10-17 12:13                       ` Usman Akinyemi
2024-10-14 16:03       ` Usman Akinyemi
2024-10-14  9:49   ` Phillip Wood
2024-10-14 10:06     ` Kristoffer Haugsbakk
2024-10-14 13:48       ` Phillip Wood
2024-10-14 18:20     ` Usman Akinyemi
2024-10-14 18:30       ` phillip.wood123
2024-10-17 11:16     ` Usman Akinyemi
2024-10-18 13:52 ` [PATCH v2 0/3] " Usman Akinyemi via GitGitGadget
2024-10-18 13:52   ` [PATCH v2 1/3] daemon: " Usman Akinyemi via GitGitGadget
2024-10-21 12:20     ` Patrick Steinhardt
2024-10-21 13:43       ` Usman Akinyemi
2024-10-21 16:24         ` Taylor Blau
2024-10-21 16:34           ` Usman Akinyemi
2024-10-18 13:52   ` [PATCH v2 2/3] merge: replace atoi() with strtol_i() for marker size validation Usman Akinyemi via GitGitGadget
2024-10-21 12:20     ` Patrick Steinhardt
2024-10-21 14:24       ` Usman Akinyemi
2024-10-21 16:34         ` Taylor Blau
2024-10-21 16:39           ` Usman Akinyemi
2024-10-21 18:00           ` Usman Akinyemi
2024-10-21 19:56             ` Taylor Blau
2024-10-30 15:20       ` Phillip Wood
2024-10-30 16:19         ` Usman Akinyemi
2024-10-31  9:58           ` Phillip Wood
2024-10-31 12:21             ` Usman Akinyemi
2024-11-06  6:05               ` Usman Akinyemi
2024-11-06 16:03               ` phillip.wood123
2024-10-18 13:53   ` [PATCH v2 3/3] imap: replace atoi() with strtol_i() for UIDVALIDITY and UIDNEXT parsing Usman Akinyemi via GitGitGadget
2024-10-21 12:20     ` Patrick Steinhardt
2024-10-21 12:27       ` Usman Akinyemi
2024-10-21 12:34         ` Patrick Steinhardt
2024-10-21 14:38           ` Usman Akinyemi
2024-10-21 16:35             ` Taylor Blau
2024-10-21 16:36               ` Usman Akinyemi
2024-10-22 13:43           ` Usman Akinyemi
2024-10-18 21:21   ` [PATCH v2 0/3] parse: replace atoi() with strtoul_ui() and strtol_i() Taylor Blau
2024-10-18 21:29     ` Usman Akinyemi
2024-10-18 21:35       ` Taylor Blau
2024-10-18 21:43         ` Usman Akinyemi
2024-10-22  5:23   ` [PATCH v3 " Usman Akinyemi via GitGitGadget
2024-10-22  5:23     ` [PATCH v3 1/3] daemon: " Usman Akinyemi via GitGitGadget
2024-10-22 16:21       ` Taylor Blau
2024-10-22 22:06         ` Usman Akinyemi
2024-10-22  5:23     ` [PATCH v3 2/3] merge: replace atoi() with strtol_i() for marker size validation Usman Akinyemi via GitGitGadget
2024-10-22  5:23     ` [PATCH v3 3/3] imap: replace atoi() with strtol_i() for UIDVALIDITY and UIDNEXT parsing Usman Akinyemi via GitGitGadget
2024-10-22 22:08     ` [PATCH v4 0/3] parse: replace atoi() with strtoul_ui() and strtol_i() Usman Akinyemi via GitGitGadget
2024-10-22 22:08       ` [PATCH v4 1/3] daemon: " Usman Akinyemi via GitGitGadget
2024-10-22 22:08       ` [PATCH v4 2/3] merge: replace atoi() with strtol_i() for marker size validation Usman Akinyemi via GitGitGadget
2024-10-22 22:08       ` [PATCH v4 3/3] imap: replace atoi() with strtol_i() for UIDVALIDITY and UIDNEXT parsing Usman Akinyemi via GitGitGadget
2024-10-23  6:05         ` Patrick Steinhardt
2024-10-23  7:40           ` Usman Akinyemi
2024-10-23  7:40       ` [PATCH v5 0/3] parse: replace atoi() with strtoul_ui() and strtol_i() Usman Akinyemi via GitGitGadget
2024-10-23  7:40         ` [PATCH v5 1/3] daemon: " Usman Akinyemi via GitGitGadget
2024-10-23 20:31           ` Taylor Blau
2024-10-24  0:23             ` Usman Akinyemi
2024-10-23  7:40         ` [PATCH v5 2/3] merge: replace atoi() with strtol_i() for marker size validation Usman Akinyemi via GitGitGadget
2024-10-23 20:32           ` Taylor Blau
2024-10-24  0:23             ` Usman Akinyemi
2024-10-23  7:40         ` [PATCH v5 3/3] imap: replace atoi() with strtol_i() for UIDVALIDITY and UIDNEXT parsing Usman Akinyemi via GitGitGadget
2024-10-23  8:52         ` [PATCH v5 0/3] parse: replace atoi() with strtoul_ui() and strtol_i() Patrick Steinhardt
2024-10-23 20:33           ` Taylor Blau
2024-10-24  0:25             ` Usman Akinyemi
2024-10-24  0:24         ` [PATCH v6 " Usman Akinyemi via GitGitGadget
2024-10-24  0:24           ` [PATCH v6 1/3] daemon: " Usman Akinyemi via GitGitGadget
2024-10-24  0:24           ` [PATCH v6 2/3] merge: replace atoi() with strtol_i() for marker size validation Usman Akinyemi via GitGitGadget
2024-10-24  0:24           ` [PATCH v6 3/3] imap: replace atoi() with strtol_i() for UIDVALIDITY and UIDNEXT parsing Usman Akinyemi via GitGitGadget
2024-10-24 18:03           ` [PATCH v6 0/3] parse: replace atoi() with strtoul_ui() and strtol_i() Taylor Blau
2024-10-25  5:06             ` Patrick Steinhardt
2024-10-25  6:11               ` Usman Akinyemi
2024-10-25 14:44                 ` Taylor Blau

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Zw0kGLZ-mcYjb6Je@pks.im \
    --to=ps@pks.im \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=phillip.wood@dunelm.org.uk \
    --cc=usmanakinyemi202@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).