add-patch: handle splitting hunks with diff.suppressBlankEmpty

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> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Phillip Wood committed Jul 20, 2024 at 16:01 UTC 39bdd84eaf646aa73a3709b0eb8be3f47378708f
2 files changed +32 -6
add-patch.c
+13 -6
@@ -400,6 +400,12 @@ static void complete_file(char marker, struct hunk *hunk)
400 hunk->splittable_into++;
401 }
402
403 +/* Empty context lines may omit the leading ' ' */
404 +static int normalize_marker(const char *p)
405 +{
406 + return p[0] == '\n' || (p[0] == '\r' && p[1] == '\n') ? ' ' : p[0];
407 +}
408 +
409 static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
410 {
411 struct strvec args = STRVEC_INIT;
@@ -485,6 +491,7 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
491 while (p != pend) {
492 char *eol = memchr(p, '\n', pend - p);
493 const char *deleted = NULL, *mode_change = NULL;
494 + char ch = normalize_marker(p);
495
496 if (!eol)
497 eol = pend;
@@ -532,7 +539,7 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
539 * Start counting into how many hunks this one can be
540 * split
541 */
535 - marker = *p;
542 + marker = ch;
543 } else if (hunk == &file_diff->head &&
544 starts_with(p, "new file")) {
545 file_diff->added = 1;
@@ -586,10 +593,10 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
593 (int)(eol - (plain->buf + file_diff->head.start)),
594 plain->buf + file_diff->head.start);
595
589 - if ((marker == '-' || marker == '+') && *p == ' ')
596 + if ((marker == '-' || marker == '+') && ch == ' ')
597 hunk->splittable_into++;
591 - if (marker && *p != '\\')
592 - marker = *p;
598 + if (marker && ch != '\\')
599 + marker = ch;
600
601 p = eol == pend ? pend : eol + 1;
602 hunk->end = p - plain->buf;
@@ -813,7 +820,7 @@ static int merge_hunks(struct add_p_state *s, struct file_diff *file_diff,
820 (int)(hunk->end - hunk->start),
821 plain + hunk->start);
822
816 - if (plain[overlap_end] != ' ')
823 + if (normalize_marker(&plain[overlap_end]) != ' ')
824 return error(_("expected context line "
825 "#%d in\n%.*s"),
826 (int)(j + 1),
@@ -953,7 +960,7 @@ static int split_hunk(struct add_p_state *s, struct file_diff *file_diff,
960 context_line_count = 0;
961
962 while (splittable_into > 1) {
956 - ch = s->plain.buf[current];
963 + ch = normalize_marker(&s->plain.buf[current]);
964
965 if (!ch)
966 BUG("buffer overrun while splitting hunks");
t/t3701-add-interactive.sh
+19
@@ -1130,4 +1130,23 @@ test_expect_success 'reset -p with unmerged files' '
1130 test_must_be_empty staged
1131 '
1132
1133 +test_expect_success 'hunk splitting works with diff.suppressBlankEmpty' '
1134 + test_config diff.suppressBlankEmpty true &&
1135 + write_script fake-editor.sh <<-\EOF &&
1136 + tr F G <"$1" >"$1.tmp" &&
1137 + mv "$1.tmp" "$1"
1138 + EOF
1139 +
1140 + test_write_lines a b "" c d "" e f "" >file &&
1141 + git add file &&
1142 + test_write_lines A b "" c D "" e F "" >file &&
1143 + (
1144 + test_set_editor "$(pwd)/fake-editor.sh" &&
1145 + test_write_lines s n y e q | git add -p file
1146 + ) &&
1147 + git cat-file blob :file >actual &&
1148 + test_write_lines a b "" c D "" e G "" >expect &&
1149 + test_cmp expect actual
1150 +'
1151 +
1152 test_done