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