[BUG, PATCH] git add -p: demonstrate failure when staging both mode and hunk

[BUG, PATCH] git add -p: demonstrate failure when staging both mode and hunk

[Kirill Smelkov]

When trying to stage changes to file which has also pending `chmod +x`,
`git add -p` produces lots of 'Use of uninitialized value ...' warnings
and fails to do the job:

    $ echo content >> file
    $ chmod +x file
    $ git add -p
    diff --git a/file b/file
    index e69de29..d95f3ad
    --- a/file
    +++ b/file
    old mode 100644
    new mode 100755
    Stage mode change [y,n,q,a,d,/,j,J,g,?]? y
    @@ -0,0 +1 @@
    +content
    Stage this hunk [y,n,q,a,d,/,K,g,e,?]? y
    Use of uninitialized value $o_ofs in addition (+) at /home/kirr/local/git/libexec/git-core/git-add--interactive line 776.
    Use of uninitialized value $ofs in numeric le (<=) at /home/kirr/local/git/libexec/git-core/git-add--interactive line 806.
    Use of uninitialized value $o0_ofs in concatenation (.) or string at /home/kirr/local/git/libexec/git-core/git-add--interactive line 830.
    Use of uninitialized value $n0_ofs in concatenation (.) or string at /home/kirr/local/git/libexec/git-core/git-add--interactive line 830.
    Use of uninitialized value $o_ofs in addition (+) at /home/kirr/local/git/libexec/git-core/git-add--interactive line 776.
    fatal: corrupt patch at line 5
    diff --git a/file b/file
    index e69de29..d95f3ad
    --- a/file
    +++ b/file
    @@ -,0 + @@
    +content

Cc: Jeff King <peff@peff.net>
Cc: Thomas Rast <trast@student.ethz.ch>
Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
---
 t/t3701-add-interactive.sh |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index fd2a55a..d5e9351 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -163,6 +163,17 @@ test_expect_success FILEMODE 'stage mode but not hunk' '
 	git diff          file | grep "+content"
 '
 
+
+test_expect_failure FILEMODE 'stage mode and hunk' '
+	git reset --hard &&
+	echo content >>file &&
+	chmod +x file &&
+	printf "y\\ny\\n" | git add -p &&
+	git diff --cached file | grep "new mode" &&
+	git diff --cached file | grep "+content" &&
+	test -z "$(git diff file)"
+'
+
 # end of tests disabled when filemode is not usable
 
 test_expect_success 'setup again' '
-- 
1.6.4.134.gb2139

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

[Thomas Rast]

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.

We add a check to the coalescing loop to ensure it only looks at diff
hunks, thus skipping mode changes.

Noticed-by: Kirill Smelkov <kirr@mns.spb.ru>
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 git-add--interactive.perl  |    4 ++++
 t/t3701-add-interactive.sh |    2 +-
 2 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index df9f231..06f7060 100755
--- 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 &&
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index d5e9351..62fd65e 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -164,7 +164,7 @@ test_expect_success FILEMODE 'stage mode but not hunk' '
 '
 
 
-test_expect_failure FILEMODE 'stage mode and hunk' '
+test_expect_success FILEMODE 'stage mode and hunk' '
 	git reset --hard &&
 	echo content >>file &&
 	chmod +x file &&
-- 
1.6.4.288.gc754a.dirty

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

[Jeff King]

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;

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

[Thomas Rast]

Jeff King wrote:
> 
> > --- 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.

Hmm.  I briefly considered worrying about futureproofing, but then
decided it wasn't worth it since we also rely on
coalesce_overlapping_hunks only being run over the hunks of a single
file.  But since you already went to the lengths of doing it, feel
free to take the explanation in my commit message and add my Acked-by
:-)

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

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

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> writes:

> Hmm.  I briefly considered worrying about futureproofing, but then
> decided it wasn't worth it since we also rely on
> coalesce_overlapping_hunks only being run over the hunks of a single
> file.

Thanks, all.

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

[Kirill Smelkov]

On Sat, Aug 15, 2009 at 11:19:24AM -0700, Junio C Hamano wrote:
> Thomas Rast <trast@student.ethz.ch> writes:
> 
> > Hmm.  I briefly considered worrying about futureproofing, but then
> > decided it wasn't worth it since we also rely on
> > coalesce_overlapping_hunks only being run over the hunks of a single
> > file.
> 
> Thanks, all.

Thomas, Jeff, Junio,

Thanks for fixing this!