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

[Usman Akinyemi <usmanakinyemi202@gmail.com>]

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.
>
> >
> > diff --git a/merge-ll.c b/merge-ll.c
> > index 8e63071922b..52870226816 100644
> > --- a/merge-ll.c
> > +++ b/merge-ll.c
> > @@ -427,7 +427,8 @@ enum ll_merge_result ll_merge(mmbuffer_t *result_buf,
> >       git_check_attr(istate, path, check);
> >       ll_driver_name = check->items[0].value;
> >       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 '%s', expecting an integer", check->items[1].value);
> >               if (marker_size <= 0)
> >                       marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
> >       }
> > @@ -454,7 +455,8 @@ int ll_merge_marker_size(struct index_state *istate, const char *path)
> >               check = attr_check_initl("conflict-marker-size", NULL);
> >       git_check_attr(istate, path, check);
> >       if (check->items[0].value) {
> > -             marker_size = atoi(check->items[0].value);
> > +             if (strtol_i(check->items[0].value, 10, &marker_size))
> > +                     die("invalid marker-size '%s', expecting an integer", check->items[0].value);
> >               if (marker_size <= 0)
> >                       marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
> >       }
>
> 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 ?

Thank you.
Usman Akinyemi.
> Patrick