Subject: Re: [PATCH 2/3] git-add--interactive: remove hunk coalescing

From: Junio C Hamano <[email-masked]>
Date: Wed, 02 Jul 2008 15:26:02 -0700

Thomas Rast <[email-masked]> writes:

> Current git-apply has no trouble at all applying chunks that have
> overlapping context, as produced by the splitting feature. So we can
> drop the manual coalescing.
>
> Signed-off-by: Thomas Rast <[email-masked]>
> ---
>  git-add--interactive.perl |   89 ---------------------------------------------
>  1 files changed, 0 insertions(+), 89 deletions(-)
>
> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index e1964a5..a4234d3 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -682,93 +682,6 @@ sub split_hunk {
>  	return @split;
>  }
>  
> -sub find_last_o_ctx {
> ...
> -}
> -
> -sub merge_hunk {
> ...
> -}
> -
> -sub coalesce_overlapping_hunks {
> ...
> -}
>  
>  sub help_patch_cmd {
>  	print colored $help_color, <<\EOF ;
> @@ -962,8 +875,6 @@ sub patch_update_file {
>  		}
>  	}
>  
> -	@hunk = coalesce_overlapping_hunks(@hunk);
> -
>  	my $n_lofs = 0;
>  	my @result = ();
>  	if ($mode->{USE}) {
> -- 
> 1.5.6.1.276.gde9a

I think [1/3] makes sense as we trust --recount anyway (and more
importantly if the user did not muck with the patch --recount would be a
no-op), but I am not sure about this one.  I suspect this change reduces
the precision and safety of the patch application, especially when the
user does not edit hunks.

When you "[s]plit" a hunk like this:

	@@ -n,7 +m,6 @@
         con
         text
        -deleted preimage
        +replaced postimage
	 more
	 line of context
        -deleted another
	 context

into two, we prepare these two hunks internally:

	@@ -n,5 +m,5 @@
         con
         text
        -deleted preimage
        +replaced postimage
	 more
         line of context

	@@ -l,4 +k,3 @@  -- l=n+5, k=m+5
	 more
         line of context
        -deleted another
	 context

So that applying only one piece without applying the other would still
have correct context to locate where to apply.  However, if the user says
"I want to apply both after all", we would need to remove the overlap when
merge them back.  If you don't, you would be feeding a nonsense patch to
"git apply" that goes back in context.

Blindly concatenating the above two and feeding them to "git apply" *may*
happen to work by accident, not by design.  This very much feels like a
hack of "This works most of the time for me, your mileage may vary" kind,
which we would want to avoid when we can.