git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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"),
Previous: Phillip Wood via GitGitGadgetNext: Phillip Wood
Message 2 of 6 in “add-patch: handle splitting hunks with diff.suppressBlankEmpty”
  1. add-patch: handle splitting hunks with diff.suppressBlankEmptyPhillip Wood via GitGitGadget, Jul 18, 2024
  2. Junio C HamanoJul 18, 2024
  3. Phillip WoodJul 19, 2024
  4. 0/2 add-patch: handle splitting hunks with diff.suppressBlankEmptyPhillip Wood via GitGitGadget, Jul 20, 2024
  5. 1/2 add-patch: handle splitting hunks with diff.suppressBlankEmptyPhillip Wood via GitGitGadget, Jul 20, 2024
  6. 2/2 add-patch: use normalize_marker() when recounting edited hunkPhillip Wood via GitGitGadget, Jul 20, 2024

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.