Re: [PATCH v2 2/3] merge: replace atoi() with strtol_i() for marker size validation

From: Usman Akinyemi <usmanakinyemi202@gmail.com>
Date: Oct 21, 2024, 18:00 UTC
Message-ID: <CAPSxiM9W+YcJqxnkFmOpBcHuHab6V_vn+ibwgq-vCNWZUXPw=w@mail.gmail.com>
In-Reply-To: <ZxaClmjrLAJiVxNJ@nand.local>

On Mon, Oct 21, 2024 at 4:34 PM Taylor Blau <me@ttaylorr.com> wrote:

Show 47 quoted lines

>
> On Mon, Oct 21, 2024 at 02:24:38PM +0000, Usman Akinyemi wrote:
> > On Mon, Oct 21, 2024 at 2:01 PM Patrick Steinhardt <ps@pks.im> wrote:
> > >
> > > On Fri, Oct 18, 2024 at 01:52:59PM +0000, Usman Akinyemi via GitGitGadget wrote:
> > > > From: Usman Akinyemi <usmanakinyemi202@gmail.com>
> > > >
> > > > Replaced atoi() with strtol_i() for parsing conflict-marker-size to
> > > > improve error handling. Invalid values, such as those containing letters
> > > > now trigger a clear error message.
> > > > Updated the test to verify invalid input handling.
> > >
> > > When starting a new paragraph we typically have an empty line between
> > > the paragraphs. We also tend to write commit messages as if instructing
> > > the code to change. So instead of "Replaced atoi() with..." you'd say
> > > "Replace atoi() with", and instead of "Updated the test...", you'd say
> > > "Update the test ...".
> > >
> > > The same applies to your other commits, as well.
>
> Thanks for noting, Patrick.
>
> > > These are a bit curious. As your test demonstrates, we retrieve the
> > > values from the "gitattributes" file. And given that the file tends to be
> > > checked into the repository, you can now basically break somebody elses
> > > commands by having an invalid value in there.
> > >
> > > That makes me think that we likely shouldn't die here. We may print a
> > > warning, but other than that we should likely continue and use the
> > > DEFAULT_CONFLICT_MARKER_SIZE.
> > >
> >
> > Ohh, I understand. Philip suggested this. For the warning, will I just
> > use printf statement or what function to print the statement ?
> > Also, how do I test the print warning statement ?
>
> You can use warning() instead of die(), which will also print the
> message to stderr. You can redirect stderr to a separate file in your
> test, and then grep or test_grep that to ensure that you see the warning
> message.
>
> These messages should also be marked for translation (with `_()`), so
> the result will look something like:
>
>     if (strtol_i(check->items[0].value, 10, &marker_size))
>             warning(_("invalid marker-size '%s', expecting an integer"),
>                     check->items[0].value);

Hi Taylor, when I try to use this warning(_, I was getting some error In the editor warning(_("invalid marker-size '%s', expecting an integer"), check->items[1].value); Incompatible integer to pointer conversion passing 'int' to parameter of type 'const char *' while I tried run make

erge-ll.c: In function ‘ll_merge’:
merge-ll.c:432:33: error: implicit declaration of function ‘_’
[-Wimplicit-function-declaration]
  432 |                         warning(_("invalid marker-size '%s',
expecting an integer"), check->items[1].value);
      |                                 ^
merge-ll.c:432:33: error: passing argument 1 of ‘warning’ makes
pointer from integer without a cast [-Wint-conversion]
  432 |                         warning(_("invalid marker-size '%s',
expecting an integer"), check->items[1].value);
      |
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |                                 |
      |                                 int
In file included from merge-ll.c:9:
git-compat-util.h:691:26: note: expected ‘const char *’ but argument
is of type ‘int’
  691 | void warning(const char *err, ...) __attribute__((format
(printf, 1, 2)));
      |              ~~~~~~~~~~~~^~~

>
> Thanks,
> Taylor