Re: [PATCH 3/3] parse: replace atoi() with strtoul_ui() and strtol_i()

From: Usman Akinyemi <usmanakinyemi202@gmail.com>
Date: Oct 17, 2024, 11:56 UTC
Message-ID: <CAPSxiM-Yw2H65+EHoDckU2N2hr+UrXRu5Y2JjXc+TEwEUKJT0Q@mail.gmail.com>
In-Reply-To: <84dbe9f1-976d-45f8-a49a-d0f942906686@gmail.com>

On Mon, Oct 14, 2024 at 6:36 PM <phillip.wood123@gmail.com> wrote:

Show 17 quoted lines

>
> On 14/10/2024 17:26, Usman Akinyemi wrote:
> > On Mon, Oct 14, 2024 at 4:13 PM Usman Akinyemi
> >> On Mon, Oct 14, 2024 at 2:55 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
> >> 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 ?
>
> In general you should add tests in the same commit as the code changes
> that they test. In this instance I think you want to split this patch
> into three, one patch for git-daemon, one for imap-send and one for the
> merge marker config changes. Each patch should have a commit message
> explaining the changes and whether they change the behavior of the code
> (for example rejecting non-numbers) and add some tests. Note that I
> don't think it is possible to test the imap-send changes but the other
> two should be easy enough. The tests should be added to one of the
> existing test files that are testing the code being changed.

Hello,
I am currently facing some issues while trying to write the test for
daemon.c, I need some help on it.
The start_git_daemon function inside lib-git-daemon.sh is made to
allow --init-timeout, --max-connections and
timeout as well as other arguments. The start_git_daemon function in
lib-git-daemon.sh is used at t5570-git-daemon.sh.
Basically this is my changes
                if (skip_prefix(arg, "--timeout=", &v)) {
-                       timeout = atoi(v);
+                       if (strtoul_ui(v, 10, &timeout))
+                               die("invalid timeout '%s', expecting a
non-negative integer", v);
                        continue;
                }
                if (skip_prefix(arg, "--init-timeout=", &v)) {
-                       init_timeout = atoi(v);
+                       if (strtoul_ui(v, 10, &init_timeout))
+                               die("invalid init-timeout '%s',
expecting a non-negative integer", v);
                        continue;
                }
                if (skip_prefix(arg, "--max-connections=", &v)) {
-                       max_connections = atoi(v);
+                       if (strtol_i(v, 10, &max_connections))
+                               die("invalid '--max-connections' '%s',
expecting an integer", v);
                        if (max_connections < 0)
-                               max_connections = 0;            /* unlimited */
+                               max_connections = 0;  /* unlimited */
                        continue;
                }
What happened is that the start_git_daemon will already fail and will
prevent the
t5570-git-daemon.sh from starting if there is any wrong starting
condition such as the new
changes I added. I am finding it hard to come up with an approach to
test the new change.

Thank you.

Show 14 quoted lines

>
> >> Thanks.
> > Also, do I need to add the reference which mentions the leftoverbit in
> > the commit message?
>
> I'm not sure that's necessary so long as you explain the reason for the
> changes in the commit message.
>
>
> Best Wishes
>
> Phillip
>
>