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

From: Taylor Blau <me@ttaylorr.com>
Date: Oct 21, 2024, 16:34 UTC
Message-ID: <ZxaClmjrLAJiVxNJ@nand.local>
In-Reply-To: <CAPSxiM_BCz2n-uOOSRk3AsVp-Y7R+1XNfVRt6dH6=fWVBZ6cBw@mail.gmail.com>

On Mon, Oct 21, 2024 at 02:24:38PM +0000, Usman Akinyemi wrote:

Show 17 quoted lines

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

Show 13 quoted lines

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

Thanks, Taylor