Skip to contentSkip to conversation

add-patch: handle splitting hunks with diff.suppressBlankEmpty

PATCH Superseded Phillip Wood via GitGitGadget 3 participants 6 messages Jul 18, 2024 – Jul 20, 2024

lore
Share & tools
lore

Phillip Wood via GitGitGadget

author
#0 lore raw

PATCH 1/1 add-patch: handle splitting hunks with diff.suppressBlankEmpty

From: Phillip Wood <[email protected]>

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 <[email protected]>
Helped-by Jeff King <[email protected]>
Signed-off-by Phillip Wood <[email protected]>

---
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(-)
add-patch.c +12 -5
index d8ea05ff108..13b2607f544 100644
@@ -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");
t/t3701-add-interactive.sh +11 -0
index 5d78868ac16..351dd2b4332 100755
@@ -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

Signature · 1 line
gitgitgadget
#1 lore raw
Quoted 20 lines · Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

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.

Quoted 4 lines
Reported-by Ilya Tumaykin <[email protected]>
Helped-by Jeff King <[email protected]>
Signed-off-by Phillip Wood <[email protected]>

---

Well written. Thanks for a pleasant read.

add-patch.c static int split_hunk(struct add_p_state *s, struct file_diff *file_diff, · line 963

Quoted 6 lines
add-patch.c +1 -1
@@ -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"),
#2 lore raw

Hi Junio

Quoted 22 lines · Junio C Hamano
Quoted 20 lines · Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

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.

Quoted 6 lines
Quoted 4 lines
Reported-by Ilya Tumaykin <[email protected]>
Helped-by Jeff King <[email protected]>
Signed-off-by Phillip Wood <[email protected]>

---

Well written. Thanks for a pleasant read.

Thanks to Peff for letting me steal his commit message

Quoted 9 lines

add-patch.c static int split_hunk(struct add_p_state *s, struct file_diff *file_diff, · line 963

Quoted 6 lines
add-patch.c +1 -1
@@ -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

Quoted 8 lines

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

author
#3 lore raw

v2 cover add-patch: handle splitting hunks with diff.suppressBlankEmpty

In reply to Phillip Wood via GitGitGadget

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

Signature · 1 line
gitgitgadget
author
#4 lore raw

PATCH v2 1/2 add-patch: handle splitting hunks with diff.suppressBlankEmpty

From: Phillip Wood <[email protected]>

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 <[email protected]>
Helped-by Jeff King <[email protected]>
Signed-off-by Phillip Wood <[email protected]>
---
 add-patch.c                | 19 +++++++++++++------
 t/t3701-add-interactive.sh | 19 +++++++++++++++++++
 2 files changed, 32 insertions(+), 6 deletions(-)
add-patch.c +13 -6
index d8ea05ff108..8feb719483f 100644
@@ -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(const 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;

@@ -813,7 +820,7 @@ 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),

@@ -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");
t/t3701-add-interactive.sh +19 -0
index 5d78868ac16..9a48933cecf 100755
@@ -1164,4 +1164,23 @@ 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 &&
+	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 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 D "" e G "" >expect &&
+	test_cmp expect actual
+'
+
 test_done
Signature · 1 line
gitgitgadget
author
#5 lore raw

PATCH v2 2/2 add-patch: use normalize_marker() when recounting edited hunk

In reply to Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

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 <[email protected]>
---
 add-patch.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
add-patch.c +2 -2
index 8feb719483f..2a72ad63d14 100644
@@ -1178,14 +1178,14 @@ static ssize_t recount_edited_hunk(struct add_p_state *s, struct hunk *hunk,
 
 	header->old_count = header->new_count = 0;
 	for (i = hunk->start; i < hunk->end; ) {
-		switch (s->plain.buf[i]) {
+		switch(normalize_marker(&s->plain.buf[i])) {
 		case '-':
 			header->old_count++;
 			break;
 		case '+':
 			header->new_count++;
 			break;
-		case ' ': case '\r': case '\n':
+		case ' ':
 			header->old_count++;
 			header->new_count++;
 			break;
Signature · 1 line
gitgitgadget