From: Thomas Gummerer <t.gummerer@gmail.com>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: Eric Sunshine <sunshine@sunshineco.com>,
Git List <git@vger.kernel.org>, Duy Nguyen <pclouds@gmail.com>,
Junio C Hamano <gitster@pobox.com>
Subject: Re: [RFC PATCH 2/4] range-diff: don't remove funcname from inner diff
Date: Mon, 15 Apr 2019 19:56:27 +0100 [thread overview]
Message-ID: <20190415185627.GB1704@hank.intra.tgummerer.com> (raw)
In-Reply-To: <nycvar.QRO.7.76.6.1904151454010.44@tvgsbejvaqbjf.bet>
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@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.
> >
> > might be appropriate.
> >
> > > + strbuf_addch(&buf, ' ');
> > > + strbuf_addbuf(&buf, &line);
> >
next prev parent reply other threads:[~2019-04-15 18:56 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-04-11 11:17 incorrect range-diff output? Duy Nguyen
2019-04-11 22:05 ` Thomas Gummerer
2019-04-12 8:41 ` Johannes Schindelin
2019-04-14 21:12 ` Thomas Gummerer
2019-04-12 9:21 ` Duy Nguyen
2019-04-12 15:02 ` Junio C Hamano
2019-04-14 21:20 ` Thomas Gummerer
2019-04-15 2:01 ` Junio C Hamano
2019-04-15 12:40 ` Johannes Schindelin
2019-04-14 21:09 ` [RFC PATCH 0/4] output improvements for git range-diff Thomas Gummerer
2019-04-14 21:09 ` [RFC PATCH 1/4] range-diff: fix function parameter indentation Thomas Gummerer
2019-04-14 21:09 ` [RFC PATCH 2/4] range-diff: don't remove funcname from inner diff Thomas Gummerer
2019-04-14 23:21 ` Eric Sunshine
2019-04-15 12:54 ` Johannes Schindelin
2019-04-15 18:56 ` Thomas Gummerer [this message]
2019-04-17 11:52 ` Johannes Schindelin
2019-04-24 18:15 ` Thomas Gummerer
2019-04-15 12:53 ` Johannes Schindelin
2019-04-15 18:57 ` Thomas Gummerer
2019-04-14 21:09 ` [RFC PATCH 3/4] range-diff: add section header instead of diff header Thomas Gummerer
2019-04-14 23:29 ` Eric Sunshine
2019-04-15 6:28 ` Johannes Sixt
2019-04-15 13:01 ` Johannes Schindelin
2019-04-15 19:09 ` Thomas Gummerer
2019-04-14 21:09 ` [RFC PATCH 4/4] range-diff: add section headers to the outer hunk header Thomas Gummerer
2019-04-15 12:44 ` Johannes Schindelin
2019-04-15 18:48 ` Thomas Gummerer
2019-04-15 12:47 ` [RFC PATCH 0/4] output improvements for git range-diff Johannes Schindelin
2019-04-15 19:25 ` Thomas Gummerer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190415185627.GB1704@hank.intra.tgummerer.com \
--to=t.gummerer@gmail.com \
--cc=Johannes.Schindelin@gmx.de \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=pclouds@gmail.com \
--cc=sunshine@sunshineco.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).