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

On Mon, Oct 14, 2024 at 6:36 PM [off-list ref] wrote:

On 14/10/2024 17:26, Usman Akinyemi wrote:

quoted

On Mon, Oct 14, 2024 at 4:13 PM Usman Akinyemi

quoted

On Mon, Oct 14, 2024 at 2:55 PM Phillip Wood [off-list ref] 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

quoted

quoted

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