On Mon, Oct 21, 2024 at 4:34 PM Taylor Blau <me@xxxxxxxxxxxx> wrote:
>
> 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@xxxxxx> wrote:
> > >
> > > On Fri, Oct 18, 2024 at 01:52:59PM +0000, Usman Akinyemi via GitGitGadget wrote:
> > > > From: Usman Akinyemi <usmanakinyemi202@xxxxxxxxx>
> > > >
> > > > 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);
>
> Thanks,
> Taylor
Thank you, I will make the changes.
