Re: [PATCH v2 2/4] add -p: gracefully ignore unparseable hunk headers in colored diffs
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 29, 2022, 07:56 UTC
- Message-ID
- <xmqqbks3qy8o.fsf@gitster.g>
- In-Reply-To
- <b07f85a035954bb9145a0fe034e6f17aeb128f52.1661376112.git.gitgitgadget@gmail.com>
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 25 quoted lines
> 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.