Re: [PATCH v2 2/4] add -p: gracefully ignore unparseable hunk headers in colored diffs

[Junio C Hamano <gitster@pobox.com>]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> diff --git a/add-patch.c b/add-patch.c
> index 509ca04456b..f2fffe1af02 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -357,16 +357,13 @@ static int parse_hunk_header(struct add_p_state *s, struct hunk *hunk)
>  	eol = memchr(line, '\n', s->colored.len - hunk->colored_start);
>  	if (!eol)
>  		eol = s->colored.buf + s->colored.len;
> -	p = memmem(line, eol - line, "@@ -", 4);
> -	if (!p)
> -		return error(_("could not parse colored hunk header '%.*s'"),
> -			     (int)(eol - line), line);
> -	p = memmem(p + 4, eol - p - 4, " @@", 3);
> -	if (!p)
> -		return error(_("could not parse colored hunk header '%.*s'"),
> -			     (int)(eol - line), line);
>  	hunk->colored_start = eol - s->colored.buf + (*eol == '\n');
> -	header->colored_extra_start = p + 3 - s->colored.buf;
> +	p = memmem(line, eol - line, "@@ -", 4);
> +	if (p && (p = memmem(p + 4, eol - p - 4, " @@", 3)))
> +		header->colored_extra_start = p + 3 - s->colored.buf;
> +	else
> +		/* could not parse colored hunk header, showing nothing */
> +		header->colored_extra_start = hunk->colored_start;
>  	header->colored_extra_end = hunk->colored_start;

OK.  We keep two copies s->plain and s->colored of the line in the
patch, and the real information on the hunk header has already been
parsed out of the "plain" version.

Here, w want to find the end of "@@ -<range>, +<range> @@"" on the
colored variant, presumably because that is where the "funcname"
may appear and more useful pieces of information can be gleaned.

The original code assumed that we should be able to find such a
place (after all, we successfully parsed the "corresponding" plain
version to find it) and threw an error otherwise.  Otherwise,

 * hunk->colored_start is set to the beginning of the next line.

 * header->colored_extra_start is set to the byte after the " @@",
   i.e. where the "funcname" ought to begin.

 * header->colored_extra_end is set to the beginning of the next
   line (i.e. "funcname" is the remainder of the line starting at
   "colored_extra_start").

The new code loosens the requirement that colored one has the same
hunk header on the line (presumably prefixed by some color codes).
When the colored line does not look like a hunk header and we cannot
tell where the "funcname" part begins, we can pretend that the entire
line was just the hunk header without "funcname" part by setting the
extra_start and extra_end both at the end of the line.

There is nothing "structurally" important on the "funcname" part, so
presumably we will only use the information to show to the display
and nothing else, so I think this is perfectly OK to do.

Another obvious possibility is to show everything on that
unparseable line.  We know it corresponds to the hunk header in the
"real" patch (i.e. s->plain), and it may have something human can
understand.  Perhaps that may be changed in the later part of this
series (I haven't looked, but if that happens "showing nothing"
comment needs to be updated).

So far, looking good.