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

[Patrick Steinhardt <ps@pks.im>]

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.

Patrick