Hi Ævar
On 20/12/2021 19:06, Ævar Arnfjörð Bjarmason wrote:
On Mon, Dec 20 2021, Phillip Wood via GitGitGadget wrote:
From: Phillip Wood <phillip.wood@xxxxxxxxxxxxx>
To determine whether a hunk can be split a counter is incremented each
time a context line follows an insertion or deletion. If at the end of
the hunk the value of this counter is greater than one then the hunk
can be split into that number of smaller hunks. If the last hunk in a
file ends with an insertion or deletion then there is no following
context line and the counter will not be incremented. This case is
already handled at the end of the loop where counter is incremented if
the last hunk ended with an insertion or deletion. Unfortunately there
is no similar check between files (likely because the perl version
only ever parses one diff at a time). Fix this by checking if the last
hunk ended with an insertion or deletion when we see the diff header
of a new file and extend the existing regression test.
Reproted-by: SZEDER Gábor <szeder.dev@xxxxxxxxx>
Signed-off-by: Phillip Wood <phillip.wood@xxxxxxxxxxxxx>
---
add-patch.c | 7 ++++++
t/t3701-add-interactive.sh | 46 ++++++++++++++++++++++++++++++++++----
2 files changed, 49 insertions(+), 4 deletions(-)
diff --git a/add-patch.c b/add-patch.c
index 8c41cdfe39b..5cea70666e9 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -472,6 +472,13 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
eol = pend;
if (starts_with(p, "diff ")) {
+ if (marker == '-' || marker == '+')
+ /*
+ * Last hunk ended in non-context line (i.e. it
+ * appended lines to the file, so there are no
+ * trailing context lines).
+ */
+ hunk->splittable_into++;
I wondered if factoring out these several "marker == '-' || marker ==
'+'" cases in parse_diff() into a "is_plus_minus(marker)" was worth it,
but probably not.
Yeah in the end I just factored out this hunk into a new function but I
didn't add a function for "marker == '-' || marker ==
> '+'"
ALLOC_GROW_BY(s->file_diff, s->file_diff_nr, 1,
file_diff_alloc);
file_diff = s->file_diff + s->file_diff_nr - 1;
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 77de0029ba5..94537a6b40a 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -326,7 +326,9 @@ test_expect_success 'correct message when there is nothing to do' '
test_expect_success 'setup again' '
git reset --hard &&
test_chmod +x file &&
- echo content >>file
+ echo content >>file &&
+ test_write_lines A B C D>file2 &&
style nit: "cmd args >file2" not "cmd args>file2"
@@ -373,8 +411,8 @@ test_expect_success 'setup expected' '
test_expect_success 'add first line works' '
git commit -am "clear local changes" &&
git apply patch &&
- test_write_lines s y y | git add -p file 2>error >raw-output &&
- sed -n -e "s/^([1-2]\/[1-2]) Stage this hunk[^@]*\(@@ .*\)/\1/" \
+ test_write_lines s y y s y n y | git add -p 2>error >raw-output &&
+ sed -n -e "s/^([1-9]\/[1-9]) Stage this hunk[^@]*\(@@ .*\)/\1/" \
-e "/^[-+@ \\\\]"/p raw-output >output &&
test_must_be_empty error &&
git diff --cached >diff &&
style/diff nit: maybe worth it to in 1/2 do some version of:
test_write_lines ... >lines &&
git ... <lines .. &&
...
sed -n \
-e ... \
-e ... \
>output
Just to make the diff smaller, i.e. just the "test_write_lines" line
would be modified here.
In the end I decided to leave this as is, while refactoring slightly
simplifies this patch it makes the previous one bigger and means that
would need to be reviewed again.
The changes themselves & this series LGTM.
Thanks
Best Wishes
Phillip
