{"thread":{"id":"22754","subject":"[PATCH] add-interactive: fix bogus diff header line ordering","startedAt":"2010-02-22T10:32:56Z","lastAt":"2010-02-23T01:50:48Z","messageCount":5,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"135290","messageId":"20100222103256.GA10557@coredump.intra.peff.net","threadId":"22754","inReplyTo":null,"subject":"[PATCH] add-interactive: fix bogus diff header line ordering","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-02-22T10:32:56Z","receivedAt":"2010-02-22T10:32:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we look at a patch for adding hunks interactively, we\nfirst split it into a header and a list of hunks. Some of\nthe header lines, such as mode changes and deletion, however,\nbecome their own selectable hunks. Later when we reassemble\nthe patch, we simply concatenate the header and the selected\nhunks. This leads to patches like this:\n\n  diff --git a/file b/file\n  index d95f3ad..0000000\n  --- a/file\n  +++ /dev/null\n  deleted file mode 100644\n  @@ -1 +0,0 @@\n  -content\n\nNotice how the deletion comes _after_ the ---/+++ lines,\nwhen it should come before.\n\nIn many cases, we can get away with this as git-apply\naccepts the slightly bogus input. However, in the specific\ncase of a deletion line that is being applied via \"apply\n-R\", this malformed patch triggers an assert in git-apply.\nThis comes up when discarding a deletion via \"git checkout\n-p\".\n\nRather than try to make git-apply accept our odd input,\nlet's just reassemble the patch in the correct order.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nWe talked about this bogosity a few months ago:\n\nhttp://article.gmane.org/gmane.comp.version-control.git/134815\n\n(see my \"slightly different approach\" section). And your response was:\n\n  That might be something we may want to fix someday [...] but someday\n  is not today.\n\nBut since it is triggering a real problem, I think someday is today. :)\n\nThe reassembly feels a little hack-ish to me, but it should work fine in\nall cases I can imagine (and of course passes all tests).  The\nalternatives would be:\n\n  - making git-apply more liberal in accepting this bogus input. I don't\n    see much of a point, as the input is clearly malformed (we just\n    happened to accept it in some cases).\n\n  - refactoring the whole hunk selection loop to be more clueful about\n    magic like mode changes and deletion, and keep the patch in the\n    \"right\" order. I just don't think it's worth the effort.\n\nI've cc'd Thomas because the bug was found through his \"checkout -p\". It\nis not your bug at all, Thomas, but I thought you would be a good person\nto sanity check the fix.\n\n git-add--interactive.perl |   17 ++++++++++++++++-\n t/t2016-checkout-patch.sh |    8 ++++++++\n 2 files changed, 24 insertions(+), 1 deletions(-)\n\ndiff --git a/git-add--interactive.perl b/git-add--interactive.perl\nindex bfd1003..4173200 100755\n--- a/git-add--interactive.perl\n+++ b/git-add--interactive.perl\n@@ -979,6 +979,21 @@ sub coalesce_overlapping_hunks {\n \treturn @out;\n }\n \n+sub reassemble_patch {\n+\tmy $head = shift;\n+\treturn (\n+\t\t# Include everything in the header except the beginning of the\n+\t\t# diff.\n+\t\t(grep { !/^[-+]{3}/ } @$head),\n+\t\t# Then include any other non-diff header lines from the hunks.\n+\t\t(grep { !/^[@ +-]/ } @_),\n+\t\t# Then begin the diff.\n+\t\t(grep { /^[-+]{3}/ } @$head),\n+\t\t# And then the hunk diff lines.\n+\t\t(grep { /^[@ +-]/ } @_)\n+\t);\n+}\n+\n sub color_diff {\n \treturn map {\n \t\tcolored((/^@/  ? $fraginfo_color :\n@@ -1475,7 +1490,7 @@ sub patch_update_file {\n \n \tif (@result) {\n \t\tmy $fh;\n-\t\tmy @patch = (@{$head->{TEXT}}, @result);\n+\t\tmy @patch = reassemble_patch($head->{TEXT}, @result);\n \t\tmy $apply_routine = $patch_mode_flavour{APPLY};\n \t\t&$apply_routine(@patch);\n \t\trefresh();\ndiff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh\nindex 4d1c2e9..2144184 100755\n--- a/t/t2016-checkout-patch.sh\n+++ b/t/t2016-checkout-patch.sh\n@@ -66,6 +66,14 @@ test_expect_success 'git checkout -p HEAD^' '\n \tverify_state dir/foo parent parent\n '\n \n+test_expect_success 'git checkout -p handles deletion' '\n+\tset_state dir/foo work index &&\n+\trm dir/foo &&\n+\t(echo n; echo y) | git checkout -p &&\n+\tverify_saved_state bar &&\n+\tverify_state dir/foo index index\n+'\n+\n # The idea in the rest is that bar sorts first, so we always say 'y'\n # first and if the path limiter fails it'll apply to bar instead of\n # dir/foo.  There's always an extra 'n' to reject edits to dir/foo in\n-- \n1.7.0.206.g3f950.dirty\n"},{"id":"135349","messageId":"7vbpfg6h80.fsf@alter.siamese.dyndns.org","threadId":"22754","inReplyTo":"20100222103256.GA10557@coredump.intra.peff.net","subject":"Re: [PATCH] add-interactive: fix bogus diff header line ordering","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-22T22:25:19Z","receivedAt":"2010-02-22T22:25:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>  git-add--interactive.perl |   17 ++++++++++++++++-\n>  t/t2016-checkout-patch.sh |    8 ++++++++\n>  2 files changed, 24 insertions(+), 1 deletions(-)\n>\n> diff --git a/git-add--interactive.perl b/git-add--interactive.perl\n> index bfd1003..4173200 100755\n> --- a/git-add--interactive.perl\n> +++ b/git-add--interactive.perl\n> @@ -979,6 +979,21 @@ sub coalesce_overlapping_hunks {\n>  \treturn @out;\n>  }\n>  \n> +sub reassemble_patch {\n> +\tmy $head = shift;\n> +\treturn (\n> +\t\t# Include everything in the header except the beginning of the\n> +\t\t# diff.\n> +\t\t(grep { !/^[-+]{3}/ } @$head),\n> +\t\t# Then include any other non-diff header lines from the hunks.\n> +\t\t(grep { !/^[@ +-]/ } @_),\n> +\t\t# Then begin the diff.\n> +\t\t(grep { /^[-+]{3}/ } @$head),\n> +\t\t# And then the hunk diff lines.\n> +\t\t(grep { /^[@ +-]/ } @_)\n\nHmm.  Are you handling \"\\No newline at the end of the file\" correctly?\n"},{"id":"135387","messageId":"20100223005645.GB3254@coredump.intra.peff.net","threadId":"22754","inReplyTo":"7vbpfg6h80.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] add-interactive: fix bogus diff header line ordering","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-02-23T00:56:45Z","receivedAt":"2010-02-23T00:56:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 22, 2010 at 02:25:19PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> >  git-add--interactive.perl |   17 ++++++++++++++++-\n> >  t/t2016-checkout-patch.sh |    8 ++++++++\n> >  2 files changed, 24 insertions(+), 1 deletions(-)\n> >\n> > diff --git a/git-add--interactive.perl b/git-add--interactive.perl\n> > index bfd1003..4173200 100755\n> > --- a/git-add--interactive.perl\n> > +++ b/git-add--interactive.perl\n> > @@ -979,6 +979,21 @@ sub coalesce_overlapping_hunks {\n> >  \treturn @out;\n> >  }\n> >  \n> > +sub reassemble_patch {\n> > +\tmy $head = shift;\n> > +\treturn (\n> > +\t\t# Include everything in the header except the beginning of the\n> > +\t\t# diff.\n> > +\t\t(grep { !/^[-+]{3}/ } @$head),\n> > +\t\t# Then include any other non-diff header lines from the hunks.\n> > +\t\t(grep { !/^[@ +-]/ } @_),\n> > +\t\t# Then begin the diff.\n> > +\t\t(grep { /^[-+]{3}/ } @$head),\n> > +\t\t# And then the hunk diff lines.\n> > +\t\t(grep { /^[@ +-]/ } @_)\n> \n> Hmm.  Are you handling \"\\No newline at the end of the file\" correctly?\n\nNope, good catch. Maybe this should specifically be hoisting known git\nheader lines back into the header? Or perhaps a better logic would be to\ntreat each hunk individually, but just take all lines before the \"@@\"\nhunk header? Hmm. I am not sure we would even need to treat hunks\nindividually...the misplaced header lines should always be part of the\n_first_ hunk.\n\n-Peff\n"},{"id":"135388","messageId":"20100223010544.GC3254@coredump.intra.peff.net","threadId":"22754","inReplyTo":"20100223005645.GB3254@coredump.intra.peff.net","subject":"[PATCH v2] add-interactive: fix bogus diff header line ordering","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-02-23T01:05:44Z","receivedAt":"2010-02-23T01:05:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we look at a patch for adding hunks interactively, we\nfirst split it into a header and a list of hunks. Some of\nthe header lines, such as mode changes and deletion, however,\nbecome their own selectable hunks. Later when we reassemble\nthe patch, we simply concatenate the header and the selected\nhunks. This leads to patches like this:\n\n  diff --git a/file b/file\n  index d95f3ad..0000000\n  --- a/file\n  +++ /dev/null\n  deleted file mode 100644\n  @@ -1 +0,0 @@\n  -content\n\nNotice how the deletion comes _after_ the ---/+++ lines,\nwhen it should come before.\n\nIn many cases, we can get away with this as git-apply\naccepts the slightly bogus input. However, in the specific\ncase of a deletion line that is being applied via \"apply\n-R\", this malformed patch triggers an assert in git-apply.\nThis comes up when discarding a deletion via \"git checkout\n-p\".\n\nRather than try to make git-apply accept our odd input,\nlet's just reassemble the patch in the correct order.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nOn Mon, Feb 22, 2010 at 07:56:45PM -0500, Jeff King wrote:\n\n> Nope, good catch. Maybe this should specifically be hoisting known git\n> header lines back into the header? Or perhaps a better logic would be to\n> treat each hunk individually, but just take all lines before the \"@@\"\n> hunk header? Hmm. I am not sure we would even need to treat hunks\n> individually...the misplaced header lines should always be part of the\n> _first_ hunk.\n\nLike this.\n\n git-add--interactive.perl |   24 +++++++++++++++++++++++-\n t/t2016-checkout-patch.sh |    8 ++++++++\n 2 files changed, 31 insertions(+), 1 deletions(-)\n\ndiff --git a/git-add--interactive.perl b/git-add--interactive.perl\nindex cd43c34..21f1330 100755\n--- a/git-add--interactive.perl\n+++ b/git-add--interactive.perl\n@@ -957,6 +957,28 @@ sub coalesce_overlapping_hunks {\n \treturn @out;\n }\n \n+sub reassemble_patch {\n+\tmy $head = shift;\n+\tmy @patch;\n+\n+\t# Include everything in the header except the beginning of the diff.\n+\tpush @patch, (grep { !/^[-+]{3}/ } @$head);\n+\n+\t# Then include any headers from the hunk lines, which must\n+\t# come before any actual hunk.\n+\twhile (@_ && $_[0] !~ /^@/) {\n+\t\tpush @patch, shift;\n+\t}\n+\n+\t# Then begin the diff.\n+\tpush @patch, grep { /^[-+]{3}/ } @$head;\n+\n+\t# And then the actual hunks.\n+\tpush @patch, @_;\n+\n+\treturn @patch;\n+}\n+\n sub color_diff {\n \treturn map {\n \t\tcolored((/^@/  ? $fraginfo_color :\n@@ -1453,7 +1475,7 @@ sub patch_update_file {\n \n \tif (@result) {\n \t\tmy $fh;\n-\t\tmy @patch = (@{$head->{TEXT}}, @result);\n+\t\tmy @patch = reassemble_patch($head->{TEXT}, @result);\n \t\tmy $apply_routine = $patch_mode_flavour{APPLY};\n \t\t&$apply_routine(@patch);\n \t\trefresh();\ndiff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh\nindex 4d1c2e9..2144184 100755\n--- a/t/t2016-checkout-patch.sh\n+++ b/t/t2016-checkout-patch.sh\n@@ -66,6 +66,14 @@ test_expect_success 'git checkout -p HEAD^' '\n \tverify_state dir/foo parent parent\n '\n \n+test_expect_success 'git checkout -p handles deletion' '\n+\tset_state dir/foo work index &&\n+\trm dir/foo &&\n+\t(echo n; echo y) | git checkout -p &&\n+\tverify_saved_state bar &&\n+\tverify_state dir/foo index index\n+'\n+\n # The idea in the rest is that bar sorts first, so we always say 'y'\n # first and if the path limiter fails it'll apply to bar instead of\n # dir/foo.  There's always an extra 'n' to reject edits to dir/foo in\n-- \n1.7.0.207.g88f1\n"},{"id":"135390","messageId":"7v7hq4n2iv.fsf@alter.siamese.dyndns.org","threadId":"22754","inReplyTo":"20100223010544.GC3254@coredump.intra.peff.net","subject":"Re: [PATCH v2] add-interactive: fix bogus diff header line ordering","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-23T01:50:48Z","receivedAt":"2010-02-23T01:50:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> ...  Hmm. I am not sure we would even need to treat hunks\n>> individually...the misplaced header lines should always be part of the\n>> _first_ hunk.\n>\n> Like this.\n\nLooks much better ;-).  Thanks.\n\n>\n>  git-add--interactive.perl |   24 +++++++++++++++++++++++-\n>  t/t2016-checkout-patch.sh |    8 ++++++++\n>  2 files changed, 31 insertions(+), 1 deletions(-)\n>\n> diff --git a/git-add--interactive.perl b/git-add--interactive.perl\n> index cd43c34..21f1330 100755\n> --- a/git-add--interactive.perl\n> +++ b/git-add--interactive.perl\n> @@ -957,6 +957,28 @@ sub coalesce_overlapping_hunks {\n>  \treturn @out;\n>  }\n>  \n> +sub reassemble_patch {\n> +\tmy $head = shift;\n> +\tmy @patch;\n> +\n> +\t# Include everything in the header except the beginning of the diff.\n> +\tpush @patch, (grep { !/^[-+]{3}/ } @$head);\n> +\n> +\t# Then include any headers from the hunk lines, which must\n> +\t# come before any actual hunk.\n> +\twhile (@_ && $_[0] !~ /^@/) {\n> +\t\tpush @patch, shift;\n> +\t}\n> +\n> +\t# Then begin the diff.\n> +\tpush @patch, grep { /^[-+]{3}/ } @$head;\n> +\n> +\t# And then the actual hunks.\n> +\tpush @patch, @_;\n> +\n> +\treturn @patch;\n> +}\n> +\n>  sub color_diff {\n>  \treturn map {\n>  \t\tcolored((/^@/  ? $fraginfo_color :\n> @@ -1453,7 +1475,7 @@ sub patch_update_file {\n>  \n>  \tif (@result) {\n>  \t\tmy $fh;\n> -\t\tmy @patch = (@{$head->{TEXT}}, @result);\n> +\t\tmy @patch = reassemble_patch($head->{TEXT}, @result);\n>  \t\tmy $apply_routine = $patch_mode_flavour{APPLY};\n>  \t\t&$apply_routine(@patch);\n>  \t\trefresh();\n> diff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh\n> index 4d1c2e9..2144184 100755\n> --- a/t/t2016-checkout-patch.sh\n> +++ b/t/t2016-checkout-patch.sh\n> @@ -66,6 +66,14 @@ test_expect_success 'git checkout -p HEAD^' '\n>  \tverify_state dir/foo parent parent\n>  '\n>  \n> +test_expect_success 'git checkout -p handles deletion' '\n> +\tset_state dir/foo work index &&\n> +\trm dir/foo &&\n> +\t(echo n; echo y) | git checkout -p &&\n> +\tverify_saved_state bar &&\n> +\tverify_state dir/foo index index\n> +'\n> +\n>  # The idea in the rest is that bar sorts first, so we always say 'y'\n>  # first and if the path limiter fails it'll apply to bar instead of\n>  # dir/foo.  There's always an extra 'n' to reject edits to dir/foo in\n> -- \n> 1.7.0.207.g88f1\n"}]}