Re: [PATCH] add -p: coalesce hunks before testing applicability
* Phillip Wood [2018-09-13 11:20]: Yes in the long term we want to be able to coalesce edited hunks, but I think it is confusing to call coalesce_overlapping_hunks() at the moment as it will not coalesce the edited hunks. I would see it as a first step into that direction. I think that if you split a hunk, edit the first subhunk, transforming a trailing context line to a deletion then try if you try to stage the second subhunk it will fail. With your patch the edit will succeed as the second subhunk is skipped when testing the edited patch. Then when you try to stage the second subhunk it will fail as it's leading context will contradict the trailing lines of the edited subhunk. With the old method the edit failed but didn't store up trouble for the future. Agreed. I guess the question is if you assume a hunk to be applied or skipped as the default. You can still find enough cases where neither the current nor the patched version works. I stumbled upon the one case where I wanted to stage only one part of a split hunk and that one worked after my patch. I leave it up to you if the added benefit overweights the stored up trouble. Cheers Jochen signature.asc Description: PGP signature
Re: [PATCH] add -p: coalesce hunks before testing applicability
Hi Phillip,
* Phillip Wood [2018-08-30 14:47]:
When $newhunk is created it is marked as dirty to prevent
coalesce_overlapping_hunks() from coalescing it. This patch does not
change that. What is happening is that by calling
coalesce_overlapping_hunks() the hunks that are not currently selected
are filtered out and any hunks that can be coalesced are (I think that
in the test that starts passing with this patch the only change is the
filtering as there's only a single hunk selected).
Agreed here. It would be enough to include the first hunk in the test to
make it fail again. Still I would see the patch as going in the right
direction as we need something like coalesce_overlapping_hunks() to make
the hunks applicable after the edit.
This is a subtle change to the test for the applicability of an edited
hunk. Previously when all the hunks were used to create the test patch
we could be certain that if the test patch applied then if the user
later selected any unselected hunk or deselected any selected hunk
then that operation would succeed. I'm not sure that is true now (but
I haven't thought about it for very long).
I'm not sure here. If we use the same test from t3701, do s(plit),
y(es), e(dit), it would fail later on. Can you come up with an example?
We could restore the old
test condition and coalesce the hunks by copying all the hunks and
setting $hunk->{USE}=1 when creating the test patch if that turns out
to be useful (it would be interesting to see if the test still passes
with that change).
We set USE=1 for $newhunk already, or where would you set it?
Cheers Jochen
signature.asc
Description: PGP signature
[PATCH] add -p: coalesce hunks before testing applicability
When a hunk was split before being edited manually, it does not apply
anymore cleanly. Apply coalesce_overlapping_hunks() first to make it
work. Enable test for it as well.
Signed-off-by: Jochen Sprickerhof
---
git-add--interactive.perl | 8
t/t3701-add-interactive.sh | 2 +-
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 36f38ced9..c9f434e4a 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1195,10 +1195,10 @@ sub edit_hunk_loop {
# delta from the original unedited hunk.
$hunk->{OFS_DELTA} and
$newhunk->{OFS_DELTA} += $hunk->{OFS_DELTA};
- if (diff_applies($head,
-@{$hunks}[0..$ix-1],
-$newhunk,
-@{$hunks}[$ix+1..$#{$hunks}])) {
+ my @hunk = @{$hunks};
+ splice (@hunk, $ix, 1, $newhunk);
+ @hunk = coalesce_overlapping_hunks(@hunk);
+ if (diff_applies($head, @hunk)) {
$newhunk->{DISPLAY} = [color_diff(@{$newtext})];
return $newhunk;
}
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index b170fb02b..b04806ad7 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -348,7 +348,7 @@ test_expect_success 'split hunk "add -p (edit)"' '
! grep "^+15" actual
'
-test_expect_failure 'split hunk "add -p (no, yes, edit)"' '
+test_expect_success 'split hunk "add -p (no, yes, edit)"' '
test_write_lines 5 10 20 21 30 31 40 50 60 >test &&
git reset &&
# test sequence is s(plit), n(o), y(es), e(dit)
--
2.18.0
