threads / patch / 20604

patchgit add -p: demonstrate failure when staging both mode and hunk

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

## tl;dr

6 messages between Aug 15, 2009 and Aug 18, 2009. Diffs are folded; open one to read it.

replies: 5people: 4as markdown or json

Kirill Smelkov· Aug 15, 2009, 12:26 UTC · lore

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

Show changes to t/t3701-add-interactive.sh +11 −0

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

Thomas Rast· Aug 15, 2009, 13:56 UTC · re: Kirill Smelkov · lore

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

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

Show changes to 2 files +5 −1

git-add--interactive.perl, t/t3701-add-interactive.sh

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

Jeff King· Aug 15, 2009, 14:17 UTC · re: Thomas Rast · lore

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

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

Show 9 quoted lines

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

Show 13 quoted lines

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

---

Show changes to git-add--interactive.perl +8 −3

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;

Thomas Rast· Aug 15, 2009, 14:35 UTC · re: Jeff King · lore

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

Jeff King wrote:

Show 25 quoted lines

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

Junio C Hamano· Aug 15, 2009, 18:19 UTC · re: Thomas Rast · lore

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

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.

Kirill Smelkov· Aug 18, 2009, 07:52 UTC · re: Junio C Hamano · lore

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

On Sat, Aug 15, 2009 at 11:19:24AM -0700, Junio C Hamano wrote:

Show 8 quoted lines

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