[PATCH] add-patch: handle splitting hunks with diff.suppressBlankEmpty

From: Phillip Wood via GitGitGadget <gitgitgadget@gmail.com>
Date: Jul 18, 2024, 14:23 UTC
Message-ID: <pull.1763.git.1721312619822.gitgitgadget@gmail.com>

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(-)

diff --git a/add-patch.c b/add-patch.c
index d8ea05ff108..13b2607f544 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -400,6 +400,12 @@ static void complete_file(char marker, struct hunk *hunk)
 		hunk->splittable_into++;
 }
 
+/* Empty context lines may omit the leading ' ' */
+static int normalize_marker(char *p)
+{
+	return p[0] == '\n' || (p[0] == '\r' && p[1] == '\n') ? ' ' : p[0];
+}
+
 static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
 {
 	struct strvec args = STRVEC_INIT;
@@ -485,6 +491,7 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
 	while (p != pend) {
 		char *eol = memchr(p, '\n', pend - p);
 		const char *deleted = NULL, *mode_change = NULL;
+		char ch = normalize_marker(p);
 
 		if (!eol)
 			eol = pend;
@@ -532,7 +539,7 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
 			 * Start counting into how many hunks this one can be
 			 * split
 			 */
-			marker = *p;
+			marker = ch;
 		} else if (hunk == &file_diff->head &&
 			   starts_with(p, "new file")) {
 			file_diff->added = 1;
@@ -586,10 +593,10 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
 			    (int)(eol - (plain->buf + file_diff->head.start)),
 			    plain->buf + file_diff->head.start);
 
-		if ((marker == '-' || marker == '+') && *p == ' ')
+		if ((marker == '-' || marker == '+') && ch == ' ')
 			hunk->splittable_into++;
-		if (marker && *p != '\\')
-			marker = *p;
+		if (marker && ch != '\\')
+			marker = ch;
 
 		p = eol == pend ? pend : eol + 1;
 		hunk->end = p - plain->buf;
@@ -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);
 
 		if (!ch)
 			BUG("buffer overrun while splitting hunks");
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 5d78868ac16..351dd2b4332 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -1164,4 +1164,15 @@ test_expect_success 'reset -p with unmerged files' '
 	test_must_be_empty staged
 '
 
+test_expect_success 'hunk splitting works with diff.suppressBlankEmpty' '
+	test_config diff.suppressBlankEmpty true &&
+	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 &&
+	git cat-file blob :file >actual &&
+	test_write_lines a b c "" s t u >expect &&
+	test_cmp expect actual
+'
+
 test_done

base-commit: 790a17fb19d6eadd16c52e5d284a5c6921744766

-- 
gitgitgadget