On 04/15, Johannes Schindelin wrote:
> Hi Eric,
>
> On Sun, 14 Apr 2019, Eric Sunshine wrote:
>
> > On Sun, Apr 14, 2019 at 5:09 PM Thomas Gummerer <t.gummerer@xxxxxxxxx> wrote:
> > > [...]
> > > However it can still be useful to have the function name that 'git
> > > diff' extracts as additional context for the change.
> > > [...]
> > > Signed-off-by: Thomas Gummerer <t.gummerer@xxxxxxxxx>
> > > ---
> > > diff --git a/range-diff.c b/range-diff.c
> > > @@ -102,9 +102,12 @@ static int read_patches(const char *range, struct string_list *list)
> > > + } else if (starts_with(line.buf, "@@ ")) {
> > > + char *skip_lineno = strstr(line.buf + 3, "@@");
> > > + strbuf_remove(&line, 0, skip_lineno - line.buf);
> >
> > It makes me a bit uncomfortable that this is not checking for NULL
> > return from strstr() before doing pointer arithmetic (even though the
> > input is presumably machine-generated).
> >
> > if (!skip_lineno)
> > BUG(...);
>
> Good point, but maybe we should not go so far as to declare this a bug,
> and fall back to removing everything bug the initial two `at` characters
> instead?
I like declaring this a bug. We are after all parsing
machine-generated output, that does come from git (which is why I
neglected the NULL checking in the first place). If that second "@@"
is not there it's definitely a bug somewhere in the diff machinery,
I'd say.
Note that the "@@" also couldn't come from anywhere else, the diff
header has a well defined format and so does the metadata. The diff
itself is prefixed with '<', '>' and '#' in this case, and the commit
message is also prefixed with four spaces. So if this breaks
somewhere I'd rather hear about it loudly, than let users potentially
get wrong output because we missed something somewhere.
> >
> > might be appropriate.
> >
> > > + strbuf_addch(&buf, ' ');
> > > + strbuf_addbuf(&buf, &line);
> >
