From a0870ab260eaf1fd1f872abaf44a24a4b192a68e Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 24 Oct 2019 10:31:44 +0200 Subject: [PATCH 1/5] fixup! built-in add -p: implement the hunk splitting feature When counting how many lines a hunk can be split into, we need to be careful to take line comments in the diff into account, i.e. lines starting with a backslash. Example: [...] -} \ No newline at end of file +} Logically, these line comments belong to the line preceding them, therefore we need to pretend that they have the same diff marker (space, minus or plus character). This fixes https://github.com/git-for-windows/git/issues/2364. Signed-off-by: Johannes Schindelin --- add-patch.c | 5 ++--- t/t3701-add-interactive.sh | 12 ++++++++++++ 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/add-patch.c b/add-patch.c index 7711fced844a6b..fa285078104a25 100644 --- a/add-patch.c +++ b/add-patch.c @@ -511,10 +511,9 @@ 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 == ' ' || *p == '\\')) + if ((marker == '-' || marker == '+') && *p == ' ') hunk->splittable_into++; - if (marker) + if (marker && *p != '\\') marker = *p; p = eol == pend ? pend : eol + 1; diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 5fcb0eee972db6..2915065fad47c1 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -472,6 +472,18 @@ test_expect_failure 'split hunk "add -p (no, yes, edit)"' ' ! grep "^+31" actual ' +test_expect_success 'split hunk with incomplete line at end' ' + git reset --hard && + printf "missing LF" >>test && + git add test && + test_write_lines before 10 20 30 40 50 60 70 >test && + git grep --cached missing && + test_write_lines s n y q | git add -p && + test_must_fail git grep --cached missing && + git grep before && + test_must_fail git grep --cached before +' + test_expect_failure 'edit, adding lines to the first hunk' ' test_write_lines 10 11 20 30 40 50 51 60 >test && git reset && From c59445a88284e66bb93143991e05bb529b6ce882 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 24 Oct 2019 12:12:50 +0200 Subject: [PATCH 2/5] fixup! built-in add -p: implement the hunk splitting feature Let's be a bit more defensive in the code that finds the next line: if we are already at the end of the buffer, we definitely do _not_ want to return an offset that is outside the buffer! Signed-off-by: Johannes Schindelin --- add-patch.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/add-patch.c b/add-patch.c index fa285078104a25..dd1fb0d644e84e 100644 --- a/add-patch.c +++ b/add-patch.c @@ -562,8 +562,13 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps) static size_t find_next_line(struct strbuf *sb, size_t offset) { - char *eol = memchr(sb->buf + offset, '\n', sb->len - offset); + char *eol; + if (offset >= sb->len) + BUG("looking for next line beyond buffer (%d >= %d)\n%s", + (int)offset, (int)sb->len, sb->buf); + + eol = memchr(sb->buf + offset, '\n', sb->len - offset); if (!eol) return sb->len; return eol - sb->buf + 1; From 098aa7740b05a8ad219aaa4573886020fd0fa077 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 24 Oct 2019 12:13:52 +0200 Subject: [PATCH 3/5] fixup! built-in add -p: implement the hunk splitting feature A recently-reported bug had its root cause in the `splittable_into` variable overcounting the number of potential hunks (the symptom was misleading, it said that there was an unhandled diff marker). Let's make sure to catch these things earlier, with a less misleading error message. Signed-off-by: Johannes Schindelin --- add-patch.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/add-patch.c b/add-patch.c index dd1fb0d644e84e..f9fe650d511102 100644 --- a/add-patch.c +++ b/add-patch.c @@ -853,6 +853,10 @@ static int split_hunk(struct add_p_state *s, struct file_diff *file_diff, while (splittable_into > 1) { ch = s->plain.buf[current]; + + if (!ch) + BUG("buffer overrun while splitting hunks"); + if ((marker == '-' || marker == '+') && ch == ' ') { first = 0; hunk[1].start = current; From a3ed9a398ec3793faa311cca754637c8a53641a8 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 24 Oct 2019 12:15:44 +0200 Subject: [PATCH 4/5] fixup! built-in add -p: implement the hunk splitting feature While it is true that the diffs Git generates can only contain comment lines at the end of a hunk, and really only when indicating that a line does not end in a Line Feed, comment lines _are_ a valid diff construct. Let's play it safe by handling those when splitting hunks. Signed-off-by: Johannes Schindelin --- add-patch.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/add-patch.c b/add-patch.c index f9fe650d511102..075a2961a805c7 100644 --- a/add-patch.c +++ b/add-patch.c @@ -867,6 +867,10 @@ static int split_hunk(struct add_p_state *s, struct file_diff *file_diff, if (marker != ' ' || (ch != '-' && ch != '+')) { next_hunk_line: + /* Comment lines are attached to the previous line */ + if (ch == '\\') + ch = marker ? marker : ' '; + /* current hunk not done yet */ if (ch == ' ') context_line_count++; From e10734de10a9706c624dd2f2b81ed84fca33c6a2 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 24 Oct 2019 12:17:24 +0200 Subject: [PATCH 5/5] fixup! built-in add -p: implement the hunk splitting feature While in the vicinity of this code, let's document what the rather convoluted conditions in the hunk-splitting loop actually _mean_: while they look simple enough, it took a bit of brain-twisting even for the developer who wrote this code to understand after a few months what is going on in this part. And if even the person who wrote this has a hard time understanding the code... then it needs commenting ;-) Signed-off-by: Johannes Schindelin --- add-patch.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/add-patch.c b/add-patch.c index 075a2961a805c7..aeff1e9af6c2a0 100644 --- a/add-patch.c +++ b/add-patch.c @@ -857,6 +857,10 @@ static int split_hunk(struct add_p_state *s, struct file_diff *file_diff, if (!ch) BUG("buffer overrun while splitting hunks"); + /* + * Is this the first context line after a chain of +/- lines? + * Then record the start of the next split hunk. + */ if ((marker == '-' || marker == '+') && ch == ' ') { first = 0; hunk[1].start = current; @@ -865,6 +869,16 @@ static int split_hunk(struct add_p_state *s, struct file_diff *file_diff, context_line_count = 0; } + /* + * Was the previous line a +/- one? Alternatively, is this the + * first line (and not a +/- one)? + * + * Then just increment the appropriate counter and continue + * with the next line. + * + * Otherwise this is the first of a chain of +/- lines. + * neither the first of a chain of context lines? + */ if (marker != ' ' || (ch != '-' && ch != '+')) { next_hunk_line: /* Comment lines are attached to the previous line */