From: phillip.wood123@gmail.com Date: Tue, 15 Oct 2024 18:28:20 GMT Subject: Re: [PATCH 3/3] parse: replace atoi() with strtoul_ui() and strtol_i() Message-ID: <9952b3a7-0ebc-4555-b8b3-f50f6f383704@gmail.com> In-Reply-To: Hi Usman On 15/10/2024 16:17, Usman Akinyemi wrote: > On Mon, Oct 14, 2024 at 6:36 PM 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 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 > 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 >> >>