From: Phillip Wood Date: Fri, 19 Jul 2024 15:17:50 GMT Subject: Re: [PATCH] add-patch: handle splitting hunks with diff.suppressBlankEmpty Message-ID: <6c7dfa30-6d4e-4b87-a25a-c1f85c83a6b1@gmail.com> In-Reply-To: Hi Junio On 18/07/2024 17:29, Junio C Hamano wrote: > "Phillip Wood via GitGitGadget" writes: > >> From: Phillip Wood >> >> 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 >> Helped-by: Jeff King >> Signed-off-by: Phillip Wood >> --- > > Well written. Thanks for a pleasant read. Thanks to Peff for letting me steal his commit message >> @@ -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 ;-) Yes I think that would be better > 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"), Oh dear, I'm not sure how I missed that. I'll fix that and update the test to make sure it exercises that code path as well. Thanks for your comments Phillip