Re: [PATCH] add -p: do not attempt to coalesce mode changes

[Jeff King <peff@peff.net>]

On Sat, Aug 15, 2009 at 03:56:39PM +0200, Thomas Rast wrote:

> In 0392513 (add-interactive: refactor mode hunk handling, 2009-04-16),
> we merged the interaction loops for mode changes and hunk staging.
> This was fine at the time, because 0beee4c (git-add--interactive:
> remove hunk coalescing, 2008-07-02) removed hunk coalescing.
> 
> However, in 7a26e65 (Revert "git-add--interactive: remove hunk
> coalescing", 2009-05-16), we resurrected it.  Since then, the code
> would attempt in vain to merge mode changes with diff hunks,
> corrupting both in the process.

Thanks for the writeup; I bisected the problem to 7a26e65, as well, but
hadn't yet figured out what was going on. :)

> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -841,6 +841,10 @@
>  	my ($last_o_ctx, $last_was_dirty);
>  
>  	for (grep { $_->{USE} } @in) {
> +		if ($_->{TYPE} ne 'hunk') {
> +			push @out, $_;
> +			next;
> +		}
>  		my $text = $_->{TEXT};
>  		my ($o_ofs) = parse_hunk_header($text->[0]);
>  		if (defined $last_o_ctx &&

Hmm. I am not too familiar with the coalesce_overlapping_hunks code, but
it looks like we peek at $out[-1] based on $last_o_ctx, assuming that
$last_o_ctx comes from the last hunk pushed (either because we just
pushed it, or we merged into it).  So a non-hunk in the middle of some
coalescing hunks is going to violate that assumption.

As it is now, I think we always put the 'mode' hunk at the very
beginning, so that shouldn't happen (and IIRC, that order is preserved
throughout). So maybe it is not worth worrying about. But an alternate
patch is below.

---
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index a06172c..c859bb4 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -838,21 +838,26 @@ sub coalesce_overlapping_hunks {
 	my (@in) = @_;
 	my @out = ();
 
-	my ($last_o_ctx, $last_was_dirty);
+	my ($last_o_ctx, $last_was_dirty, $last_hunk);
 
 	for (grep { $_->{USE} } @in) {
+		if ($_->{TYPE} ne 'hunk') {
+			push @out, $_;
+			next;
+		}
 		my $text = $_->{TEXT};
 		my ($o_ofs) = parse_hunk_header($text->[0]);
 		if (defined $last_o_ctx &&
 		    $o_ofs <= $last_o_ctx &&
 		    !$_->{DIRTY} &&
 		    !$last_was_dirty) {
-			merge_hunk($out[-1], $_);
+			merge_hunk($last_hunk, $_);
 		}
 		else {
 			push @out, $_;
+			$last_hunk = $_;
 		}
-		$last_o_ctx = find_last_o_ctx($out[-1]);
+		$last_o_ctx = find_last_o_ctx($last_hunk);
 		$last_was_dirty = $_->{DIRTY};
 	}
 	return @out;