{"thread":{"id":"20604","subject":"[BUG, PATCH] git add -p: demonstrate failure when staging both mode and hunk","startedAt":"2009-08-15T12:26:49Z","lastAt":"2009-08-18T07:52:58Z","messageCount":6,"participants":["Kirill Smelkov","Thomas Rast","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"120709","messageId":"1250339209-27962-1-git-send-email-kirr@mns.spb.ru","threadId":"20604","inReplyTo":null,"subject":"[BUG, PATCH] git add -p: demonstrate failure when staging both mode and hunk","fromName":"Kirill Smelkov","fromEmail":"kirr@mns.spb.ru","sentAt":"2009-08-15T12:26:49Z","receivedAt":"2009-08-15T12:26:49Z","isPatch":true,"sender":{"key":"kirr@navytux.spb.ru","avatar":"https://gravatar.com/avatar/cf3445fdad1849941e17ab25bf1ee7c5ea1be2deb444be25ff5f36b0e50a985f?d=mp&s=160"},"body":"When trying to stage changes to file which has also pending `chmod +x`,\n`git add -p` produces lots of 'Use of uninitialized value ...' warnings\nand fails to do the job:\n\n $ echo content >> file\n $ chmod +x file\n $ git add -p\n diff --git a/file b/file\n index e69de29..d95f3ad\n --- a/file\n +++ b/file\n old mode 100644\n new mode 100755\n Stage mode change [y,n,q,a,d,/,j,J,g,?]? y\n @@ -0,0 +1 @@\n +content\n Stage this hunk [y,n,q,a,d,/,K,g,e,?]? y\n Use of uninitialized value $o_ofs in addition (+) at /home/kirr/local/git/libexec/git-core/git-add--interactive line 776.\n Use of uninitialized value $ofs in numeric le (<=) at /home/kirr/local/git/libexec/git-core/git-add--interactive line 806.\n Use of uninitialized value $o0_ofs in concatenation (.) or string at /home/kirr/local/git/libexec/git-core/git-add--interactive line 830.\n Use of uninitialized value $n0_ofs in concatenation (.) or string at /home/kirr/local/git/libexec/git-core/git-add--interactive line 830.\n Use of uninitialized value $o_ofs in addition (+) at /home/kirr/local/git/libexec/git-core/git-add--interactive line 776.\n fatal: corrupt patch at line 5\n diff --git a/file b/file\n index e69de29..d95f3ad\n --- a/file\n +++ b/file\n @@ -,0 + @@\n +content\n\nCc: Jeff King \nCc: Thomas Rast \nSigned-off-by: Kirill Smelkov \n---\n t/t3701-add-interactive.sh | 11 +++++++++++\n 1 files changed, 11 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex fd2a55a..d5e9351 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -163,6 +163,17 @@ test_expect_success FILEMODE 'stage mode but not hunk' '\n \tgit diff file | grep \"+content\"\n '\n \n+\n+test_expect_failure FILEMODE 'stage mode and hunk' '\n+\tgit reset --hard &&\n+\techo content >>file &&\n+\tchmod +x file &&\n+\tprintf \"y\\\\ny\\\\n\" | git add -p &&\n+\tgit diff --cached file | grep \"new mode\" &&\n+\tgit diff --cached file | grep \"+content\" &&\n+\ttest -z \"$(git diff file)\"\n+'\n+\n # end of tests disabled when filemode is not usable\n \n test_expect_success 'setup again' '\n-- \n1.6.4.134.gb2139\n"},{"id":"120710","messageId":"770693df8f416615f57423141fb59f3d6eccc915.1250344341.git.trast@student.ethz.ch","threadId":"20604","inReplyTo":"1250339209-27962-1-git-send-email-kirr@mns.spb.ru","subject":"[PATCH] add -p: do not attempt to coalesce mode changes","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2009-08-15T13:56:39Z","receivedAt":"2009-08-15T13:56:39Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"In 0392513 (add-interactive: refactor mode hunk handling, 2009-04-16),\nwe merged the interaction loops for mode changes and hunk staging.\nThis was fine at the time, because 0beee4c (git-add--interactive:\nremove hunk coalescing, 2008-07-02) removed hunk coalescing.\n\nHowever, in 7a26e65 (Revert \"git-add--interactive: remove hunk\ncoalescing\", 2009-05-16), we resurrected it. Since then, the code\nwould attempt in vain to merge mode changes with diff hunks,\ncorrupting both in the process.\n\nWe add a check to the coalescing loop to ensure it only looks at diff\nhunks, thus skipping mode changes.\n\nNoticed-by: Kirill Smelkov \nSigned-off-by: Thomas Rast \n---\n git-add--interactive.perl | 4 ++++\n t/t3701-add-interactive.sh | 2 +-\n 2 files changed, 5 insertions(+), 1 deletions(-)\n\ndiff --git a/git-add--interactive.perl b/git-add--interactive.perl\nindex df9f231..06f7060 100755\n--- a/git-add--interactive.perl\n+++ b/git-add--interactive.perl\n@@ -841,6 +841,10 @@\n \tmy ($last_o_ctx, $last_was_dirty);\n \n \tfor (grep { $_->{USE} } @in) {\n+\t\tif ($_->{TYPE} ne 'hunk') {\n+\t\t\tpush @out, $_;\n+\t\t\tnext;\n+\t\t}\n \t\tmy $text = $_->{TEXT};\n \t\tmy ($o_ofs) = parse_hunk_header($text->[0]);\n \t\tif (defined $last_o_ctx &&\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex d5e9351..62fd65e 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -164,7 +164,7 @@ test_expect_success FILEMODE 'stage mode but not hunk' '\n '\n \n \n-test_expect_failure FILEMODE 'stage mode and hunk' '\n+test_expect_success FILEMODE 'stage mode and hunk' '\n \tgit reset --hard &&\n \techo content >>file &&\n \tchmod +x file &&\n-- \n1.6.4.288.gc754a.dirty\n"},{"id":"120712","messageId":"20090815141710.GA15978@sigill.intra.peff.net","threadId":"20604","inReplyTo":"770693df8f416615f57423141fb59f3d6eccc915.1250344341.git.trast@student.ethz.ch","subject":"Re: [PATCH] add -p: do not attempt to coalesce mode changes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-08-15T14:17:11Z","receivedAt":"2009-08-15T14:17:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Aug 15, 2009 at 03:56:39PM +0200, Thomas Rast wrote:\n\n> In 0392513 (add-interactive: refactor mode hunk handling, 2009-04-16),\n> we merged the interaction loops for mode changes and hunk staging.\n> This was fine at the time, because 0beee4c (git-add--interactive:\n> remove hunk coalescing, 2008-07-02) removed hunk coalescing.\n> \n> However, in 7a26e65 (Revert \"git-add--interactive: remove hunk\n> coalescing\", 2009-05-16), we resurrected it. Since then, the code\n> would attempt in vain to merge mode changes with diff hunks,\n> corrupting both in the process.\n\nThanks for the writeup; I bisected the problem to 7a26e65, as well, but\nhadn't yet figured out what was going on. :)\n\n> --- a/git-add--interactive.perl\n> +++ b/git-add--interactive.perl\n> @@ -841,6 +841,10 @@\n> \tmy ($last_o_ctx, $last_was_dirty);\n> \n> \tfor (grep { $_->{USE} } @in) {\n> +\t\tif ($_->{TYPE} ne 'hunk') {\n> +\t\t\tpush @out, $_;\n> +\t\t\tnext;\n> +\t\t}\n> \t\tmy $text = $_->{TEXT};\n> \t\tmy ($o_ofs) = parse_hunk_header($text->[0]);\n> \t\tif (defined $last_o_ctx &&\n\nHmm. I am not too familiar with the coalesce_overlapping_hunks code, but\nit looks like we peek at $out[-1] based on $last_o_ctx, assuming that\n$last_o_ctx comes from the last hunk pushed (either because we just\npushed it, or we merged into it). So a non-hunk in the middle of some\ncoalescing hunks is going to violate that assumption.\n\nAs it is now, I think we always put the 'mode' hunk at the very\nbeginning, so that shouldn't happen (and IIRC, that order is preserved\nthroughout). So maybe it is not worth worrying about. But an alternate\npatch is below.\n\n---\ndiff --git a/git-add--interactive.perl b/git-add--interactive.perl\nindex a06172c..c859bb4 100755\n--- a/git-add--interactive.perl\n+++ b/git-add--interactive.perl\n@@ -838,21 +838,26 @@ sub coalesce_overlapping_hunks {\n \tmy (@in) = @_;\n \tmy @out = ();\n \n-\tmy ($last_o_ctx, $last_was_dirty);\n+\tmy ($last_o_ctx, $last_was_dirty, $last_hunk);\n \n \tfor (grep { $_->{USE} } @in) {\n+\t\tif ($_->{TYPE} ne 'hunk') {\n+\t\t\tpush @out, $_;\n+\t\t\tnext;\n+\t\t}\n \t\tmy $text = $_->{TEXT};\n \t\tmy ($o_ofs) = parse_hunk_header($text->[0]);\n \t\tif (defined $last_o_ctx &&\n \t\t $o_ofs <= $last_o_ctx &&\n \t\t !$_->{DIRTY} &&\n \t\t !$last_was_dirty) {\n-\t\t\tmerge_hunk($out[-1], $_);\n+\t\t\tmerge_hunk($last_hunk, $_);\n \t\t}\n \t\telse {\n \t\t\tpush @out, $_;\n+\t\t\t$last_hunk = $_;\n \t\t}\n-\t\t$last_o_ctx = find_last_o_ctx($out[-1]);\n+\t\t$last_o_ctx = find_last_o_ctx($last_hunk);\n \t\t$last_was_dirty = $_->{DIRTY};\n \t}\n \treturn @out;\n"},{"id":"120715","messageId":"200908151635.24341.trast@student.ethz.ch","threadId":"20604","inReplyTo":"20090815141710.GA15978@sigill.intra.peff.net","subject":"Re: [PATCH] add -p: do not attempt to coalesce mode changes","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2009-08-15T14:35:23Z","receivedAt":"2009-08-15T14:35:23Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Jeff King wrote:\n> \n> > --- a/git-add--interactive.perl\n> > +++ b/git-add--interactive.perl\n> > @@ -841,6 +841,10 @@\n> > \tmy ($last_o_ctx, $last_was_dirty);\n> > \n> > \tfor (grep { $_->{USE} } @in) {\n> > +\t\tif ($_->{TYPE} ne 'hunk') {\n> > +\t\t\tpush @out, $_;\n> > +\t\t\tnext;\n> > +\t\t}\n> > \t\tmy $text = $_->{TEXT};\n> > \t\tmy ($o_ofs) = parse_hunk_header($text->[0]);\n> > \t\tif (defined $last_o_ctx &&\n> \n> Hmm. I am not too familiar with the coalesce_overlapping_hunks code, but\n> it looks like we peek at $out[-1] based on $last_o_ctx, assuming that\n> $last_o_ctx comes from the last hunk pushed (either because we just\n> pushed it, or we merged into it). So a non-hunk in the middle of some\n> coalescing hunks is going to violate that assumption.\n> \n> As it is now, I think we always put the 'mode' hunk at the very\n> beginning, so that shouldn't happen (and IIRC, that order is preserved\n> throughout). So maybe it is not worth worrying about. But an alternate\n> patch is below.\n\nHmm. I briefly considered worrying about futureproofing, but then\ndecided it wasn't worth it since we also rely on\ncoalesce_overlapping_hunks only being run over the hunks of a single\nfile. But since you already went to the lengths of doing it, feel\nfree to take the explanation in my commit message and add my Acked-by\n:-)\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"120723","messageId":"7vtz09lz2r.fsf@alter.siamese.dyndns.org","threadId":"20604","inReplyTo":"200908151635.24341.trast@student.ethz.ch","subject":"Re: [PATCH] add -p: do not attempt to coalesce mode changes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-08-15T18:19:24Z","receivedAt":"2009-08-15T18:19:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast writes:\n\n> Hmm. I briefly considered worrying about futureproofing, but then\n> decided it wasn't worth it since we also rely on\n> coalesce_overlapping_hunks only being run over the hunks of a single\n> file.\n\nThanks, all.\n"},{"id":"121063","messageId":"20090818075258.GA4501@tugrik.mns.mnsspb.ru","threadId":"20604","inReplyTo":"7vtz09lz2r.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] add -p: do not attempt to coalesce mode changes","fromName":"Kirill Smelkov","fromEmail":"kirr@mns.spb.ru","sentAt":"2009-08-18T07:52:58Z","receivedAt":"2009-08-18T07:52:58Z","isPatch":true,"sender":{"key":"kirr@navytux.spb.ru","avatar":"https://gravatar.com/avatar/cf3445fdad1849941e17ab25bf1ee7c5ea1be2deb444be25ff5f36b0e50a985f?d=mp&s=160"},"body":"On Sat, Aug 15, 2009 at 11:19:24AM -0700, Junio C Hamano wrote:\n> Thomas Rast writes:\n> \n> > Hmm. I briefly considered worrying about futureproofing, but then\n> > decided it wasn't worth it since we also rely on\n> > coalesce_overlapping_hunks only being run over the hunks of a single\n> > file.\n> \n> Thanks, all.\n\nThomas, Jeff, Junio,\n\nThanks for fixing this!\n"}]}