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

[Vasil Dimov <vd@freebsd.org>]

On Wed, Apr 15, 2020 at 10:20:35 -0600, Taylor Blau wrote:
> 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

Fixed in v2.

> > `"%.*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?

I believe that assigning it just before len is changed and that grouping
all usage of the new variable close to each other makes the code more
readable. Ideally it would also be defined below.

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

It was '\0' before we overwrote it with '\n':

 89                 len = find_end_of_line(line, size);
 90                 line[len - 1] = '\0';

`line` points to a buffer of the entire output of `git log`, with many
newlines in it. In the beginning of the loop we overwrite the first new
line char in the buffer with '\0', then we restore it to '\n' and
eventually advance the pointer to the start of the next line.

I think that the intention of this code is to print only one line (the
current one).

-- 
Vasil Dimov
gro.DSBeerF@dv
%
Success consists of going from failure to failure without loss of
enthusiasm.
                -- Winston Churchill