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

From: phillip.wood123@gmail.com <phillip.wood123@gmail.com>
Date: Oct 15, 2024, 18:28 UTC
Message-ID: <9952b3a7-0ebc-4555-b8b3-f50f6f383704@gmail.com>
In-Reply-To: <CAPSxiM8-C6DAE3nYqMUCs+UgHN1R41grwVE+S-cSi6gZGvCpYw@mail.gmail.com>

Hi Usman

On 15/10/2024 16:17, Usman Akinyemi wrote:

Show 38 quoted lines

> On Mon, Oct 14, 2024 at 6:36 PM <phillip.wood123@gmail.com> wrote:
>>
>> 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

It would be better to check for the error message with test_cmp or at least grep for a longer phrase so we're sure the error message is the one we think we should be getting.

> #
> 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 ?

The code you're changing parses the conflict-marker-size attribute so you need to set up a .gitattributes file with an invalid marker size and then run "git merge" or "git cherry-pick"

Best Wishes

Phillip

Show 20 quoted lines

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