Re: [PATCH 2/2] range-diff: avoid negative string precision

[Taylor Blau <me@ttaylorr.com>]

On Wed, Apr 15, 2020 at 02:28:41PM +0000, Vasil Dimov via GitGitGadget wrote:
> From: Vasil Dimov <vd@FreeBSD.org>
>
> If the supplied integer for "precisoin" is negative in

s/precisoin/precision

> `"%.*s", len, line` then it is ignored. So the current code is
> equivalent to just `"%s", line` because it is executed only if
> `len` is negative.
>
> Fix this by saving the value of `len` before overwriting it with the
> return value of `parse_git_diff_header()`.
>
> Signed-off-by: Vasil Dimov <vd@FreeBSD.org>
> ---
>  range-diff.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/range-diff.c b/range-diff.c
> index 5cc920be391..40af0862818 100644
> --- a/range-diff.c
> +++ b/range-diff.c
> @@ -123,16 +123,19 @@ static int read_patches(const char *range, struct string_list *list,
>  			struct patch patch = { 0 };
>  			struct strbuf root = STRBUF_INIT;
>  			int linenr = 0;
> +			int orig_len;

Any reason to not assign this to 'len' up here?
>
>  			in_header = 0;
>  			strbuf_addch(&buf, '\n');
>  			if (!util->diff_offset)
>  				util->diff_offset = buf.len;
>  			line[len - 1] = '\n';
> +			orig_len = len;
>  			len = parse_git_diff_header(&root, &linenr, 0, line,
>  						    len, size, &patch);

OK, so we cut up the line by placing a NL at len, and then feed it to
'parse_git_diff_header' which will tell us the length of the thing that
it parsed, or give a negative value if it couldn't parse...

>  			if (len < 0)
> -				die(_("could not parse git header '%.*s'"), (int)len, line);
> +				die(_("could not parse git header '%.*s'"),
> +				    orig_len, line);

...and then you restore the original length and print it out here. It
seems like this error is now misleading though, because the line is
already modified at the point that the newline was inserted.

>  			strbuf_addstr(&buf, " ## ");
>  			if (patch.is_new > 0)
>  				strbuf_addf(&buf, "%s (new)", patch.new_name);
> --
> gitgitgadget

Thanks,
Taylor