On 04/15, Johannes Schindelin wrote:
> Hi Thomas,
>
>
> On Sun, 14 Apr 2019, Thomas Gummerer wrote:
> > diff --git a/range-diff.c b/range-diff.c
> > index 9242b8975f..f365141ade 100644
> > --- a/range-diff.c
> > +++ b/range-diff.c
> > @@ -102,9 +102,12 @@ static int read_patches(const char *range, struct string_list *list)
> > strbuf_addch(&buf, '\n');
> > }
> > continue;
> > - } else if (starts_with(line.buf, "@@ "))
> > - strbuf_addstr(&buf, "@@");
> > - else if (!line.buf[0] || starts_with(line.buf, "index "))
> > + } else if (starts_with(line.buf, "@@ ")) {
> > + char *skip_lineno = strstr(line.buf + 3, "@@");
>
> Rather than using the magic constant "3", it would probably make sense to
> declare `skip_lineno` outside of the `if` construct, and use
> `skip_prefix(line.buf, "@@ ", &skip_lineno)` instead of
> `starts_with(...)`.
I like this suggestion.
> We *will*, however, want to have a safeguard against `strstr()` not
> finding anything. Maybe re-use the `p` variable that we already have, and
> do this instead:
>
> } else if (skip_prefix(line.buf, "@@ ", &p) &&
> (p = strstr(p, "@@"))) {
>
> > + strbuf_remove(&line, 0, skip_lineno - line.buf);
> > + strbuf_addch(&buf, ' ');
>
> Shorter:
>
> strbuf_splice(&line, 0, p - line.buf, " ", 1);
>
> (assuming that you accept my suggestion to use `p` instead of
> `skip_lineno`...)
And this is also much nicer, thanks!
> Thanks,
> Dscho
>
> > + strbuf_addbuf(&buf, &line);
> > + } else if (!line.buf[0] || starts_with(line.buf, "index "))
> > /*
> > * A completely blank (not ' \n', which is context)
> > * line is not valid in a diff. We skip it
> > --
> > 2.21.0.593.g511ec345e1
> >
> >
