patchadd-patch: handle splitting hunks with diff.suppressBlankEmpty

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>
---
    add-patch: handle splitting hunks with diff.suppressBlankEmpty
    
    This is an alternative to jk/add-patch-with-suppress-blank-empty which
    was recently discarded from next. I hope that normalizing the context
    marker will simplify any future changes to the code.
    
    While I was writing this I realized that we should be recalculating
    hunk->splittable_into when we re-count the hunk header of it is edited
    but I've left to for a future series.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1763%2Fphillipwood%2Fadd-p-suppress-blank-empty-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1763/phillipwood/add-p-suppress-blank-empty-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1763

 add-patch.c                | 17 ++++++++++++-----
 t/t3701-add-interactive.sh | 11 +++++++++++
 2 files changed, 23 insertions(+), 5 deletions(-)

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

Well written.  Thanks for a pleasant read.

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"),

Hi Junio

On 18/07/2024 17:29, Junio C Hamano wrote:

Thanks to Peff for letting me steal his commit message

Yes I think that would be better

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

This is an alternative to jk/add-patch-with-suppress-blank-empty which was recently discarded from next. I hope that normalizing the context marker will simplify any future changes to the code.

Changes since V1

 * Updated merge_hunks() to use normalize_marker() as spotted by Junio
 * Updated the test so it checks merge_hunks() as well.

Phillip Wood (2):
  add-patch: handle splitting hunks with diff.suppressBlankEmpty
  add-patch: use normalize_marker() when recounting edited hunk

 add-patch.c                | 23 +++++++++++++++--------
 t/t3701-add-interactive.sh | 19 +++++++++++++++++++
 2 files changed, 34 insertions(+), 8 deletions(-)

base-commit: 790a17fb19d6eadd16c52e5d284a5c6921744766
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1763%2Fphillipwood%2Fadd-p-suppress-blank-empty-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1763/phillipwood/add-p-suppress-blank-empty-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1763

Range-diff vs v1:

 1:  fd09e66727a ! 1:  34d8fd44a97 add-patch: handle splitting hunks with diff.suppressBlankEmpty
     @@ add-patch.c: static void complete_file(char marker, struct hunk *hunk)
       }
       
      +/* Empty context lines may omit the leading ' ' */
     -+static int normalize_marker(char *p)
     ++static int normalize_marker(const char *p)
      +{
      +	return p[0] == '\n' || (p[0] == '\r' && p[1] == '\n') ? ' ' : p[0];
      +}
     @@ add-patch.c: static int parse_diff(struct add_p_state *s, const struct pathspec
       
       		p = eol == pend ? pend : eol + 1;
       		hunk->end = p - plain->buf;
     +@@ add-patch.c: static int merge_hunks(struct add_p_state *s, struct file_diff *file_diff,
     + 					    (int)(hunk->end - hunk->start),
     + 					    plain + hunk->start);
     + 
     +-				if (plain[overlap_end] != ' ')
     ++				if (normalize_marker(&plain[overlap_end]) != ' ')
     + 					return error(_("expected context line "
     + 						       "#%d in\n%.*s"),
     + 						     (int)(j + 1),
      @@ add-patch.c: 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);
     ++		ch = normalize_marker(&s->plain.buf[current]);
       
       		if (!ch)
       			BUG("buffer overrun while splitting hunks");
     @@ t/t3701-add-interactive.sh: test_expect_success 'reset -p with unmerged files' '
       
      +test_expect_success 'hunk splitting works with diff.suppressBlankEmpty' '
      +	test_config diff.suppressBlankEmpty true &&
     -+	test_write_lines a b c "" d e f >file &&
     ++	write_script fake-editor.sh <<-\EOF &&
     ++	tr F G <"$1" >"$1.tmp" &&
     ++	mv "$1.tmp" "$1"
     ++	EOF
     ++
     ++	test_write_lines a b "" c d  "" e f "" >file &&
      +	git add file &&
     -+	test_write_lines p q r "" s t u >file &&
     -+	test_write_lines s n y q | git add -p &&
     ++	test_write_lines A b "" c D  "" e F "" >file &&
     ++	(
     ++		test_set_editor "$(pwd)/fake-editor.sh" &&
     ++		test_write_lines s n y e q | git add -p file
     ++	) &&
      +	git cat-file blob :file >actual &&
     -+	test_write_lines a b c "" s t u >expect &&
     ++	test_write_lines a b "" c D "" e G "" >expect &&
      +	test_cmp expect actual
      +'
      +
 -:  ----------- > 2:  7bdcd2df012 add-patch: use normalize_marker() when recounting edited hunk

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>
---
 add-patch.c                | 19 +++++++++++++------
 t/t3701-add-interactive.sh | 19 +++++++++++++++++++
 2 files changed, 32 insertions(+), 6 deletions(-)

From: Phillip Wood <phillip.wood@dunelm.org.uk>

After the user has edited a hunk the number of lines in the pre- and post- image lines is recounted the hunk header can be updated before passing the hunk to "git apply". The recounting code correctly handles empty context lines where the leading ' ' is omitted by treating '\n' and '\r' as context lines.

Update this code to use normalize_marker() so that the handling of empty context lines is consistent with the rest of the hunk parsing code. There is a small change in behavior as normalize_marker() only treats "\r\n" as an empty context line rather than any line starting with '\r'. This should not matter in practice as Macs have used Unix line endings since MacOs 10 was released in 2001 and if it transpires that someone is still using an earlier version of MacOs where lines end with '\r' then we will need to change the handling of '\r' in normalize_marker() anyway.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 add-patch.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)