{"thread":{"id":"19367","subject":"[PATCH] new test fails \"add -p\" for adds on the top line","startedAt":"2009-05-16T03:10:19Z","lastAt":"2009-05-16T19:51:16Z","messageCount":8,"participants":["Matt Graham","Nanako Shiraishi","Thomas Rast","Junio C Hamano","Sverre Rabbelier"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"114058","messageId":"1c5969370905152010m486a8b85s96334e99e6c54ad5@mail.gmail.com","threadId":"19367","inReplyTo":null,"subject":"[PATCH] new test fails \"add -p\" for adds on the top line","fromName":"Matt Graham","fromEmail":"mdg149@gmail.com","sentAt":"2009-05-16T03:10:19Z","receivedAt":"2009-05-16T03:10:19Z","isPatch":true,"sender":{"key":"mdg149@gmail.com","avatar":"https://gravatar.com/avatar/a1f130a60a6550f75e8d7d3849e58e46494f36bfaf764a38cfd695ac85de8576?d=mp&s=160"},"body":"add -p doesn't work for some diffs. diffs adding a new line at the top of\nthe file with other adds later in the file are one way to trigger the problem.\n\nduring add -p, split the diff and then answer y for all segments. the file\nwon't have been added to the index.\n\nSigned-off-by: Matthew Graham \n---\n t/t3701-add-interactive.sh | 32 ++++++++++++++++++++++++++++++++\n 1 files changed, 32 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex dfc6560..45da6c8 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -163,6 +163,38 @@ test_expect_success FILEMODE 'stage mode but not hunk' '\n \tgit diff file | grep \"+content\"\n '\n\n+# Write the patch file with a new line at the top and bottom\n+cat >patch <expected < diff &&\n+\ttest_cmp expected diff\n+'\n+\n # end of tests disabled when filemode is not usable\n\n test_done\n-- \n1.6.3.9.g6345\n"},{"id":"114090","messageId":"20090516192529.6117@nanako3.lavabit.com","threadId":"19367","inReplyTo":"1c5969370905152010m486a8b85s96334e99e6c54ad5@mail.gmail.com","subject":"Re: [PATCH] new test fails \"add -p\" for adds on the top line","fromName":"Nanako Shiraishi","fromEmail":"nanako3@lavabit.com","sentAt":"2009-05-16T10:25:29Z","receivedAt":"2009-05-16T10:25:29Z","isPatch":true,"sender":{"key":"nanako3@lavabit.com","avatar":"https://gravatar.com/avatar/3777b9e201c5883a62b1a6fdf7c53f2d712d1d80989146063ea861e33aad72a8?d=mp&s=160"},"body":"Quoting Matt Graham :\n\n> add -p doesn't work for some diffs. diffs adding a new line at the top of\n> the file with other adds later in the file are one way to trigger the problem.\n>\n> during add -p, split the diff and then answer y for all segments. the file\n> won't have been added to the index.\n>\n> Signed-off-by: Matthew Graham \n\nI tried \"git-add -p\" from different versions and I found out that versions before the commit 0beee4c6dec15292415e3d56075c16a76a22af54 doesn't have this problem.\n\ncommit 0beee4c6dec15292415e3d56075c16a76a22af54\nAuthor: Thomas Rast \nDate: Wed Jul 2 23:59:44 2008 +0200\n\n git-add--interactive: remove hunk coalescing\n \n Current git-apply has no trouble at all applying chunks that have\n overlapping context, as produced by the splitting feature. So we can\n drop the manual coalescing.\n \n Signed-off-by: Thomas Rast \n Signed-off-by: Junio C Hamano \n\n-- \nNanako Shiraishi\nhttp://ivory.ap.teacup.com/nanako3/\n"},{"id":"114100","messageId":"200905161612.30911.trast@student.ethz.ch","threadId":"19367","inReplyTo":"20090516192529.6117@nanako3.lavabit.com","subject":"Re: [PATCH] new test fails \"add -p\" for adds on the top line","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2009-05-16T14:12:22Z","receivedAt":"2009-05-16T14:12:22Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Nanako Shiraishi wrote:\n> Quoting Matt Graham :\n> \n> > add -p doesn't work for some diffs. diffs adding a new line at the top of\n> > the file with other adds later in the file are one way to trigger the problem.\n> >\n> > during add -p, split the diff and then answer y for all segments. the file\n> > won't have been added to the index.\n> >\n> > Signed-off-by: Matthew Graham \n> \n> I tried \"git-add -p\" from different versions and I found out that versions before the commit 0beee4c6dec15292415e3d56075c16a76a22af54 doesn't have this problem.\n> \n> commit 0beee4c6dec15292415e3d56075c16a76a22af54\n> Author: Thomas Rast \n> Date: Wed Jul 2 23:59:44 2008 +0200\n> \n> git-add--interactive: remove hunk coalescing\n> \n> Current git-apply has no trouble at all applying chunks that have\n> overlapping context, as produced by the splitting feature. So we can\n> drop the manual coalescing.\n> \n> Signed-off-by: Thomas Rast \n> Signed-off-by: Junio C Hamano \n\nThe above commit still reverts cleanly, but AFAICS merge_hunk blindly\ntrusts the hunk headers, an assumption that is no longer valid due to\nthe 'edit' feature. So either we need to recount the hunk headers\nprior to merging (which was rejected back in the 'edit' feature\ndiscussion due to code complexity) or find some other solution.\n\nPassing either --unidiff-zero or -C1 with the failing patch fixes the\nproblem, but oddly (to me at least) -C2 does not. The generated error\nlooks like\n\n $ git apply --check -v -C2 < patch\n Checking patch file...\n error: while searching for:\n baseline\n content\n\n error: patch failed: file:1\n error: file: patch does not apply\n\nThe corresponding call (builtin-apply.c:2093) is\n\n\t\t\terror(\"while searching for:\\n%.*s\",\n\t\t\t (int)(old - oldlines), oldlines);\n\nso it does not seem to insert the extra newline. Is it actually\nlooking for a blank line in the context? If so, wouldn't that be a\ngit-apply bug?\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"114106","messageId":"7viqk1ndlk.fsf@alter.siamese.dyndns.org","threadId":"19367","inReplyTo":"200905161612.30911.trast@student.ethz.ch","subject":"Re: [PATCH] new test fails \"add -p\" for adds on the top line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-05-16T17:48:23Z","receivedAt":"2009-05-16T17:48:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast writes:\n\n> Nanako Shiraishi wrote:\n>> Quoting Matt Graham :\n>> \n>> > add -p doesn't work for some diffs. diffs adding a new line at the top of\n>> > the file with other adds later in the file are one way to trigger the problem.\n>> >\n>> > during add -p, split the diff and then answer y for all segments. the file\n>> > won't have been added to the index.\n>> >\n>> > Signed-off-by: Matthew Graham \n>> \n>> I tried \"git-add -p\" from different versions and I found out that versions before the commit 0beee4c6dec15292415e3d56075c16a76a22af54 doesn't have this problem.\n>> \n>> commit 0beee4c6dec15292415e3d56075c16a76a22af54\n>> Author: Thomas Rast \n>> Date: Wed Jul 2 23:59:44 2008 +0200\n>> \n>> git-add--interactive: remove hunk coalescing\n>> \n>> Current git-apply has no trouble at all applying chunks that have\n>> overlapping context, as produced by the splitting feature. So we can\n>> drop the manual coalescing.\n>> \n>> Signed-off-by: Thomas Rast \n>> Signed-off-by: Junio C Hamano \n>\n> The above commit still reverts cleanly, but AFAICS merge_hunk blindly\n> trusts the hunk headers, an assumption that is no longer valid due to\n> the 'edit' feature.\n\nHeh, here is my \"I told you so\" moment ;-).\n\nWe could also remove that \"edit\" thing; I do not use nor trust it\n(fundamentally you cannot trust it).\n\nBut how about doing it this way?\n\n-- >8 --\nSubject: Revert \"git-add--interactive: remove hunk coalescing\"\n\nThis reverts commit 0beee4c6dec15292415e3d56075c16a76a22af54 but with a\nbit of twist, as we have added \"edit hunk manually\" hack and we cannot\nrely on the original line numbers of the hunks that were manually edited.\n---\n git-add--interactive.perl | 96 +++++++++++++++++++++++++++++++++++++++++++-\n t/t3701-add-interactive.sh | 2 +-\n 2 files changed, 96 insertions(+), 2 deletions(-)\n\ndiff --git a/git-add--interactive.perl b/git-add--interactive.perl\nindex f6e536e..a06172c 100755\n--- a/git-add--interactive.perl\n+++ b/git-add--interactive.perl\n@@ -767,6 +767,96 @@ sub split_hunk {\n \treturn @split;\n }\n \n+sub find_last_o_ctx {\n+\tmy ($it) = @_;\n+\tmy $text = $it->{TEXT};\n+\tmy ($o_ofs, $o_cnt) = parse_hunk_header($text->[0]);\n+\tmy $i = @{$text};\n+\tmy $last_o_ctx = $o_ofs + $o_cnt;\n+\twhile (0 < --$i) {\n+\t\tmy $line = $text->[$i];\n+\t\tif ($line =~ /^ /) {\n+\t\t\t$last_o_ctx--;\n+\t\t\tnext;\n+\t\t}\n+\t\tlast;\n+\t}\n+\treturn $last_o_ctx;\n+}\n+\n+sub merge_hunk {\n+\tmy ($prev, $this) = @_;\n+\tmy ($o0_ofs, $o0_cnt, $n0_ofs, $n0_cnt) =\n+\t parse_hunk_header($prev->{TEXT}[0]);\n+\tmy ($o1_ofs, $o1_cnt, $n1_ofs, $n1_cnt) =\n+\t parse_hunk_header($this->{TEXT}[0]);\n+\n+\tmy (@line, $i, $ofs, $o_cnt, $n_cnt);\n+\t$ofs = $o0_ofs;\n+\t$o_cnt = $n_cnt = 0;\n+\tfor ($i = 1; $i < @{$prev->{TEXT}}; $i++) {\n+\t\tmy $line = $prev->{TEXT}[$i];\n+\t\tif ($line =~ /^\\+/) {\n+\t\t\t$n_cnt++;\n+\t\t\tpush @line, $line;\n+\t\t\tnext;\n+\t\t}\n+\n+\t\tlast if ($o1_ofs <= $ofs);\n+\n+\t\t$o_cnt++;\n+\t\t$ofs++;\n+\t\tif ($line =~ /^ /) {\n+\t\t\t$n_cnt++;\n+\t\t}\n+\t\tpush @line, $line;\n+\t}\n+\n+\tfor ($i = 1; $i < @{$this->{TEXT}}; $i++) {\n+\t\tmy $line = $this->{TEXT}[$i];\n+\t\tif ($line =~ /^\\+/) {\n+\t\t\t$n_cnt++;\n+\t\t\tpush @line, $line;\n+\t\t\tnext;\n+\t\t}\n+\t\t$ofs++;\n+\t\t$o_cnt++;\n+\t\tif ($line =~ /^ /) {\n+\t\t\t$n_cnt++;\n+\t\t}\n+\t\tpush @line, $line;\n+\t}\n+\tmy $head = (\"@@ -$o0_ofs\" .\n+\t\t (($o_cnt != 1) ? \",$o_cnt\" : '') .\n+\t\t \" +$n0_ofs\" .\n+\t\t (($n_cnt != 1) ? \",$n_cnt\" : '') .\n+\t\t \" @@\\n\");\n+\t@{$prev->{TEXT}} = ($head, @line);\n+}\n+\n+sub coalesce_overlapping_hunks {\n+\tmy (@in) = @_;\n+\tmy @out = ();\n+\n+\tmy ($last_o_ctx, $last_was_dirty);\n+\n+\tfor (grep { $_->{USE} } @in) {\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}\n+\t\telse {\n+\t\t\tpush @out, $_;\n+\t\t}\n+\t\t$last_o_ctx = find_last_o_ctx($out[-1]);\n+\t\t$last_was_dirty = $_->{DIRTY};\n+\t}\n+\treturn @out;\n+}\n \n sub color_diff {\n \treturn map {\n@@ -878,7 +968,8 @@ sub edit_hunk_loop {\n \t\tmy $newhunk = {\n \t\t\tTEXT => $text,\n \t\t\tTYPE => $hunk->[$ix]->{TYPE},\n-\t\t\tUSE => 1\n+\t\t\tUSE => 1,\n+\t\t\tDIRTY => 1,\n \t\t};\n \t\tif (diff_applies($head,\n \t\t\t\t @{$hunk}[0..$ix-1],\n@@ -1210,6 +1301,8 @@ sub patch_update_file {\n \t\t}\n \t}\n \n+\t@hunk = coalesce_overlapping_hunks(@hunk);\n+\n \tmy $n_lofs = 0;\n \tmy @result = ();\n \tfor (@hunk) {\n@@ -1224,6 +1317,7 @@ sub patch_update_file {\n \t\topen $fh, '| git apply --cached --recount';\n \t\tfor (@{$head->{TEXT}}, @result) {\n \t\t\tprint $fh $_;\n+\t\t\tprint STDERR $_;\n \t\t}\n \t\tif (!close $fh) {\n \t\t\tfor (@{$head->{TEXT}}, @result) {\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 45da6c8..c5220a1 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -187,7 +187,7 @@ index b6f2c08..61b9053 100755\n +lastline\n EOF\n # Test splitting the first patch, then adding both\n-test_expect_failure 'add first line works' '\n+test_expect_success 'add first line works' '\n \tgit commit -am \"clear local changes\" &&\n \tgit apply patch &&\n \t(echo s; echo y; echo y) | git add -p file &&\n-- \n1.6.3.1.9.g95405b\n"},{"id":"114110","messageId":"fabb9a1e0905161055q89b9e6ei77a922749ed8cd5e@mail.gmail.com","threadId":"19367","inReplyTo":"7viqk1ndlk.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] new test fails \"add -p\" for adds on the top line","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2009-05-16T17:55:35Z","receivedAt":"2009-05-16T17:55:35Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Sat, May 16, 2009 at 19:48, Junio C Hamano wrote:\n> We could also remove that \"edit\" thing; I do not use nor trust it\n> (fundamentally you cannot trust it).\n\nBut it is very useful to split up patches! Of course, I always verify\nthe end result (usually by diffing against HEAD@{...}), but it is\ndefinitely a very useful tool that I would hate to see removed :(.\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"114113","messageId":"7vr5yon9ny.fsf@alter.siamese.dyndns.org","threadId":"19367","inReplyTo":"fabb9a1e0905161055q89b9e6ei77a922749ed8cd5e@mail.gmail.com","subject":"Re: [PATCH] new test fails \"add -p\" for adds on the top line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-05-16T19:13:21Z","receivedAt":"2009-05-16T19:13:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sverre Rabbelier writes:\n\n> Heya,\n>\n> On Sat, May 16, 2009 at 19:48, Junio C Hamano wrote:\n>> We could also remove that \"edit\" thing; I do not use nor trust it\n>> (fundamentally you cannot trust it).\n>\n> But it is very useful to split up patches! Of course, I always verify\n> the end result (usually by diffing against HEAD@{...}), but it is\n> definitely a very useful tool that I would hate to see removed :(.\n\nSorry, forgot a smiley ;-).\n"},{"id":"114114","messageId":"fabb9a1e0905161214o66736a56y5660c576095a3cb5@mail.gmail.com","threadId":"19367","inReplyTo":"7vr5yon9ny.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] new test fails \"add -p\" for adds on the top line","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2009-05-16T19:14:45Z","receivedAt":"2009-05-16T19:14:45Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Sat, May 16, 2009 at 21:13, Junio C Hamano wrote:\n> Sorry, forgot a smiley ;-).\n\nPhew, what would be without those :).\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"114116","messageId":"7vab5cn7wr.fsf@alter.siamese.dyndns.org","threadId":"19367","inReplyTo":"7viqk1ndlk.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] new test fails \"add -p\" for adds on the top line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-05-16T19:51:16Z","receivedAt":"2009-05-16T19:51:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Thomas Rast writes:\n> ...\n>> The above commit still reverts cleanly, but AFAICS merge_hunk blindly\n>> trusts the hunk headers, an assumption that is no longer valid due to\n>> the 'edit' feature.\n>\n> Heh, here is my \"I told you so\" moment ;-).\n\nIt never blindly trusted before the edit 'feature'; it counted carefully\nand it could do so because it had all the necessary information.\n\nI told you that 'edit' could remember the line offset and line numbers\nbefore giving the buffer to the end user, and then recount and adjust the\ncount after getting the edited results back, to update the offset and\ncount with the same carefulness. You (and I think there was somebody else\nwho was helping) didn't listen.\n\nFundamentally, after you remove some hunks (and worse yet, you modify\nsome) from the patch and feed that to \"git apply --recount\", it can never\ndo as thorough a job as you could do inside \"add -p\" itself. The latter\nhas more information (the omitted hunks, and the hunks before/after the\nuser edited) necessary to reconstruct the line numbers and hunk size. To\nkeep the whole process more robust and trustworthy, you must do the\nnecessary computation while you still have all the information about the\nhunks you are not feeding to the downstream.\n\nThat was what the \"I told you so\" was about in my message.\n\nIt is not too late to teach the 'edit hack' to do so. That would allow us\nto remove the \"$_->{DIRTY}\" bit my \"how about this\" patch adds, and I'll\nstop calling it the 'edit hack' and start calling it the 'edit feature'\nwhen that happens ;-).\n\nBut at least the \"how about this\" patch should restore the original\nbehaviour as long as the user does not use the 'edit hack' for now.\n"}]}