Re: [PATCH] add-patch: handle splitting hunks with diff.suppressBlankEmpty
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 18, 2024, 16:29 UTC
- Message-ID
- <xmqqikx2wtn4.fsf@gitster.g>
- In-Reply-To
- <pull.1763.git.1721312619822.gitgitgadget@gmail.com>
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 24 quoted lines
> From: Phillip Wood <phillip.wood@dunelm.org.uk> > > When "add -p" parses diffs, it looks for context lines starting with a > single space. But when diff.suppressBlankEmpty is in effect, an empty > context line will omit the space, giving us a true empty line. This > confuses the parser, which is unable to split based on such a line. > > It's tempting to say that we should just make sure that we generate a > diff without that option. However, although we do not parse hunks that > the user has manually edited with parse_diff() we do allow the user > to split such hunks. As POSIX calls the decision of whether to print the > space here "implementation-defined" we need to handle edited hunks where > empty context lines omit the space. > > So let's handle both cases: a context line either starts with a space or > consists of a totally empty line by normalizing the first character to a > space when we parse them. Normalizing the first character rather than > changing the code to check for a space or newline will hopefully future > proof against introducing similar bugs if the code is changed. > > Reported-by: Ilya Tumaykin <itumaykin@gmail.com> > Helped-by: Jeff King <peff@peff.net> > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > ---
Well written. Thanks for a pleasant read.
Show 6 quoted lines
> @@ -953,7 +960,7 @@ static int split_hunk(struct add_p_state *s, struct file_diff *file_diff,
> context_line_count = 0;
>
> while (splittable_into > 1) {
> - ch = s->plain.buf[current];
> + ch = normalize_marker(s->plain.buf + current);I wondered if &s->plain.buf[current] is easier to grok, but what's written already is good enough ;-)
There is another explicit mention of ' ' in merge_hunks(). Unless we are normalizing the buffer here (which I do not think we do), wouldn't we have to make sure that the code over there also knows that an empty line in a patch is an unmodified empty line?
if (plain[overlap_end] != ' ')
return error(_("expected context line "
"#%d in\n%.*s"),