Re: [RFC PATCH 2/4] range-diff: don't remove funcname from inner diff

From: Thomas Gummerer <t.gummerer@gmail.com>
Date: Apr 15, 2019, 18:56 UTC
Message-ID: <20190415185627.GB1704@hank.intra.tgummerer.com>
In-Reply-To: <nycvar.QRO.7.76.6.1904151454010.44@tvgsbejvaqbjf.bet>

On 04/15, Johannes Schindelin wrote:

Show 27 quoted lines

> Hi Eric,
> 
> On Sun, 14 Apr 2019, Eric Sunshine wrote:
> 
> > On Sun, Apr 14, 2019 at 5:09 PM Thomas Gummerer <t.gummerer@gmail.com> 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@gmail.com>
> > > ---
> > > 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.

Show 6 quoted lines

> >
> > might be appropriate.
> >
> > > +                       strbuf_addch(&buf, ' ');
> > > +                       strbuf_addbuf(&buf, &line);
> >