Re: [PATCH] add -p: coalesce hunks before testing applicability

[Junio C Hamano <gitster@pobox.com>]

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.