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>

diff --git a/add-patch.c b/add-patch.c
index 79eda168ebb7cdb739720c2c0d16a44484522822..1c28a7fa1f706d309c5733a51edb0c59e08eaecd 100644 (file)
--- 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(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");

diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 0b5339ac6ca8248582ce723e3d552a8d4513e294..4c3e2bc82f9724d2f6d9179528cfe4e723f77df1 100755 (executable)
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -1130,4 +1130,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