git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
* [PATCH] add -p: coalesce hunks before testing applicability
@ 2018-08-28  8:58 Jochen Sprickerhof
  2018-08-28 18:07 ` Junio C Hamano
  0 siblings, 1 reply; 7+ messages in thread
From: Jochen Sprickerhof @ 2018-08-28  8:58 UTC (permalink / raw)
  To: git; +Cc: Jochen Sprickerhof

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@jochen.sprickerhof.de>
---
 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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] add -p: coalesce hunks before testing applicability
  2018-08-28  8:58 [PATCH] add -p: coalesce hunks before testing applicability Jochen Sprickerhof
@ 2018-08-28 18:07 ` Junio C Hamano
  2018-08-30 13:47   ` Phillip Wood
  0 siblings, 1 reply; 7+ messages in thread
From: Junio C Hamano @ 2018-08-28 18:07 UTC (permalink / raw)
  To: Jochen Sprickerhof; +Cc: git

Jochen Sprickerhof <git@jochen.sprickerhof.de> writes:

> 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@jochen.sprickerhof.de>
> ---
>  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;
>  		}

OK.  I think I understand how this may be needed in certain forms of
edit.  I do not think coalesce would reliably work against arbitrary
kind of edit, but the function is called at the end of the loop of
the caller of this function anyway, so it is not like this is making
anything worse at all.  Let's queue it and try it out.

Thanks.

All of the following is a tangent for future/further work, and
should not be done as part of your patch.  While this change may
work around the immediate issue of diff_applies() returning "no", it
makes it feel a bit iffy to keep the interface to return $newhunk.

With the current interface, the function is saying the caller "here
is a text that shows a new hunk, when spliced back into the @hunk
array and coalesced together with others, would apply cleanly to the
current index".  But the corrected code is already doing a trial
splice with trial coalescing anyway, so perhaps it may make more
sense to update the interface into this function for the caller to
say "the user asks to edit $ix'th hunk in @$hunks" and the function
to answer "Here is the edited result in @$hunks; I've made sure it
would cleanly apply".

Incidentally, that would make it possible in the future to allow
edit_hunk_loop to be told "the user wants to edit this $ix'th hunk",
allow the editor to split that hunk into multiple hunks, and return
the result by stuffing them (not just a single $newhunk) into the
@hunk array.  The caller's loop could then further select or join
these hunks---such an interaction is impossible with the current
interface that allows edit_hunk_loop to take a single hunk and
return a single newhunk.

But again, all of the above is a tangent for future/further work,
and should not be done as part of your patch.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] add -p: coalesce hunks before testing applicability
  2018-08-28 18:07 ` Junio C Hamano
@ 2018-08-30 13:47   ` Phillip Wood
  2018-08-30 14:51     ` Junio C Hamano
  2018-09-03 19:01     ` Jochen Sprickerhof
  0 siblings, 2 replies; 7+ messages in thread
From: Phillip Wood @ 2018-08-30 13:47 UTC (permalink / raw)
  To: Junio C Hamano, Jochen Sprickerhof; +Cc: git

Dear Jochen/Junio

On 28/08/18 19:07, Junio C Hamano wrote:
> Jochen Sprickerhof <git@jochen.sprickerhof.de> writes:
> 
>> 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@jochen.sprickerhof.de>
>> ---
>>   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;
>>   		}
> 
> OK.  I think I understand how this may be needed in certain forms of
> edit. 

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).

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). 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).

Best Wishes

Phillip

  I do not think coalesce would reliably work against arbitrary
> kind of edit, but the function is called at the end of the loop of
> the caller of this function anyway, so it is not like this is making
> anything worse at all.  Let's queue it and try it out.
> 
> Thanks.
> 
> All of the following is a tangent for future/further work, and
> should not be done as part of your patch.  While this change may
> work around the immediate issue of diff_applies() returning "no", it
> makes it feel a bit iffy to keep the interface to return $newhunk.
> 
> With the current interface, the function is saying the caller "here
> is a text that shows a new hunk, when spliced back into the @hunk
> array and coalesced together with others, would apply cleanly to the
> current index".  But the corrected code is already doing a trial
> splice with trial coalescing anyway, so perhaps it may make more
> sense to update the interface into this function for the caller to
> say "the user asks to edit $ix'th hunk in @$hunks" and the function
> to answer "Here is the edited result in @$hunks; I've made sure it
> would cleanly apply".
> 
> Incidentally, that would make it possible in the future to allow
> edit_hunk_loop to be told "the user wants to edit this $ix'th hunk",
> allow the editor to split that hunk into multiple hunks, and return
> the result by stuffing them (not just a single $newhunk) into the
> @hunk array.  The caller's loop could then further select or join
> these hunks---such an interaction is impossible with the current
> interface that allows edit_hunk_loop to take a single hunk and
> return a single newhunk.
> 
> But again, all of the above is a tangent for future/further work,
> and should not be done as part of your patch.
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] add -p: coalesce hunks before testing applicability
  2018-08-30 13:47   ` Phillip Wood
@ 2018-08-30 14:51     ` Junio C Hamano
  2018-09-03 19:01     ` Jochen Sprickerhof
  1 sibling, 0 replies; 7+ messages in thread
From: Junio C Hamano @ 2018-08-30 14:51 UTC (permalink / raw)
  To: Phillip Wood; +Cc: Jochen Sprickerhof, git

Phillip Wood <phillip.wood@talktalk.net> writes:

> 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).
>
> 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). 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).
>
> Best Wishes
>
> Phillip

OK, I marked the topic as "will merge to next" but unmark it for
now, as we are not in a hurry to graduate new topics to 'master'
anyway.  Hopefully between Jochen and you, perhaps others, can
explore the issues you raised and come to some conclusion before it
becomes necessary (i.e. when the next cycle begins).

Thanks.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] add -p: coalesce hunks before testing applicability
  2018-08-30 13:47   ` Phillip Wood
  2018-08-30 14:51     ` Junio C Hamano
@ 2018-09-03 19:01     ` Jochen Sprickerhof
  2018-09-13 10:20       ` Phillip Wood
  1 sibling, 1 reply; 7+ messages in thread
From: Jochen Sprickerhof @ 2018-09-03 19:01 UTC (permalink / raw)
  To: phillip.wood; +Cc: Junio C Hamano, git

[-- Attachment #1: Type: text/plain, Size: 1705 bytes --]

Hi Phillip,

* Phillip Wood <phillip.wood@talktalk.net> [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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] add -p: coalesce hunks before testing applicability
  2018-09-03 19:01     ` Jochen Sprickerhof
@ 2018-09-13 10:20       ` Phillip Wood
  2018-09-23 17:16         ` Jochen Sprickerhof
  0 siblings, 1 reply; 7+ messages in thread
From: Phillip Wood @ 2018-09-13 10:20 UTC (permalink / raw)
  To: Jochen Sprickerhof, phillip.wood; +Cc: Junio C Hamano, git

Hi Jochen

On 03/09/2018 20:01, Jochen Sprickerhof wrote:
> Hi Phillip,
> 
> * Phillip Wood <phillip.wood@talktalk.net> [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.

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.

>> 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?

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.

>> 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?

To match the old test it needs to be set on the hunks we've skipped or
haven't got to yet so they're all in the patch that's tested after
editing a hunk.

Best Wishes

Phillip

> 
> Cheers Jochen


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] add -p: coalesce hunks before testing applicability
  2018-09-13 10:20       ` Phillip Wood
@ 2018-09-23 17:16         ` Jochen Sprickerhof
  0 siblings, 0 replies; 7+ messages in thread
From: Jochen Sprickerhof @ 2018-09-23 17:16 UTC (permalink / raw)
  To: phillip.wood; +Cc: Junio C Hamano, git

[-- Attachment #1: Type: text/plain, Size: 1231 bytes --]

* Phillip Wood <phillip.wood@talktalk.net> [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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-28  8:58 [PATCH] add -p: coalesce hunks before testing applicability Jochen Sprickerhof
2018-08-28 18:07 ` Junio C Hamano
2018-08-30 13:47   ` Phillip Wood
2018-08-30 14:51     ` Junio C Hamano
2018-09-03 19:01     ` Jochen Sprickerhof
2018-09-13 10:20       ` Phillip Wood
2018-09-23 17:16         ` Jochen Sprickerhof

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox