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

From: Usman Akinyemi <usmanakinyemi202@gmail.com>
Date: Oct 15, 2024, 15:17 UTC
Message-ID: <CAPSxiM8-C6DAE3nYqMUCs+UgHN1R41grwVE+S-cSi6gZGvCpYw@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 18 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, thanks for this, I was working on this and I need help. For the
merge-ll.c,
I noticed that the check->items[0].value were already checked to
ensure they do not contain letters in them.
        if (check->items[1].value) {
                marker_size = atoi(check->items[1].value);
                if (strtol_i(check->items[1].value, 10, &marker_size))
                        die("invalid marker-size expecting an integer");
                if (marker_size <= 0)
                        marker_size = DEFAULT_CONFLICT_MARKER_SIZE

error: option `marker-size' expects a numerical value not ok 38 - merge without conflict wrong marker-size # # cp new1.txt test.txt && # test_must_fail git merge-file -p --marker-size=1a test.txt orig.txt new2.txt 2>error && # cat error && # grep "invalid" error # I grepped the error message and I noticed that the message is gotten from parse-options.c and it ensures that the arg is negative. How to proceed in such a case ?

Also, for the daemon.c I am finding it hard to get the exact test file to add the new test.

Thank you. Usman Akinyemi

Show 13 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
>
>