{"thread":{"id":"21729","subject":"git-apply fails on creating a new file, with both -p and --directory specified","startedAt":"2009-11-23T19:45:24Z","lastAt":"2009-12-08T07:53:19Z","messageCount":14,"participants":["Steven J. Murdoch","Junio C Hamano","James Vega","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"128199","messageId":"20091123194523.GZ15966@cl.cam.ac.uk","threadId":"21729","inReplyTo":null,"subject":"git-apply fails on creating a new file, with both -p and --directory specified","fromName":"Steven J. Murdoch","fromEmail":"git+steven.murdoch@cl.cam.ac.uk","sentAt":"2009-11-23T19:45:24Z","receivedAt":"2009-11-23T19:45:24Z","isPatch":false,"sender":{"key":"git+steven.murdoch@cl.cam.ac.uk","avatar":null},"body":"While trying to apply a patch from one repository (created by\ngit-format-patch), to another (using git-am), git fails with:\n\n\"fatal: git apply: bad git-diff - inconsistent new filename on line X\"\n\nThis appears to be because I was both using -p to strip some path\ncomponents, and --directory to add different ones in. Only creating\nnew files was affected.\n\nThis was the case in git 1.6.5.2, and also the development version\n1.6.6.rc0.15.g4fa80. I have tested this on MacOS X Snow Leopard.\n\nThis appears related to the bug discussed in:\n http://marc.info/?l=git&m=122237537312597&w=2\nin which the following fix was posted:\n http://git.kernel.org/?p=git/git.git;a=commitdiff;h=969c877506cf8cc760c7b251fef6c5b6850bfc19\n\nI have included a patch below to the test cases, which currently fails\nbut, if I understand correctly, should succeed.\n\nSteven Murdoch.\n\n-- >8 --\nTest git-apply creating a new file, combining --directory and -p flags\n\nSigned-off-by: Steven Murdoch \n---\n t/t4128-apply-root.sh | 17 +++++++++++++++++\n 1 files changed, 17 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t4128-apply-root.sh b/t/t4128-apply-root.sh\nindex 8f6aea4..6cc741a 100755\n--- a/t/t4128-apply-root.sh\n+++ b/t/t4128-apply-root.sh\n@@ -58,6 +58,23 @@ test_expect_success 'apply --directory (new file)' '\n '\n \n cat > patch << EOF\n+diff --git a/c/newfile2 b/c/newfile2\n+new file mode 100644\n+index 0000000..d95f3ad\n+--- /dev/null\n++++ b/c/newfile2\n+@@ -0,0 +1 @@\n++content\n+EOF\n+\n+test_expect_success 'apply --directory -p (new file)' '\n+\tgit reset --hard initial &&\n+\tgit apply -p2 --directory=some/sub/dir/ --index patch &&\n+\ttest content = $(git show :some/sub/dir/newfile2) &&\n+\ttest content = $(cat some/sub/dir/newfile2)\n+'\n+\n+cat > patch << EOF\n diff --git a/delfile b/delfile\n deleted file mode 100644\n index d95f3ad..0000000\n-- \n1.6.5.2\n\n-- \nhttp://www.cl.cam.ac.uk/users/sjm217/\n"},{"id":"128300","messageId":"7vws1e3ma1.fsf@alter.siamese.dyndns.org","threadId":"21729","inReplyTo":"20091123194523.GZ15966@cl.cam.ac.uk","subject":"Re: git-apply fails on creating a new file, with both -p and --directory specified","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-11-25T10:56:54Z","receivedAt":"2009-11-25T10:56:54Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Steven J. Murdoch\" writes:\n\n> This appears to be because I was both using -p to strip some path\n> components, and --directory to add different ones in. Only creating\n> new files was affected.\n\nA very nicely done report.\n\nIn addition to your test case, I suspect that a patch that only changes\nmode would have acted funny with -p option.\n\n-- >8 --\n[PATCH] builtin-apply.c: pay attention to -p when determining the name\n\nThe patch structure has def_name component that is used to validate the\nsanity of a \"diff --git\" patch by checking pathnames that appear on the\npatch header lines for consistency. The git_header_name() function is\nused to compute this out of \"diff --git a/... b/...\" line, but the code\nalways stripped one level of prefix (i.e. \"a/\" and \"b/\"), without paying\nattention to -p option. Code in find_name() function that parses other\nlines in the patch header (e.g. \"--- a/...\" and \"+++ b/...\" lines) however\ndid strip the correct number of leading paths prefixes, and the sanity\ncheck between these computed values failed.\n\nTeach git_header_name() to honor -p option like find_name() function\ndoes.\n\nSigned-off-by: Junio C Hamano \n---\n builtin-apply.c | 5 +++--\n 1 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex f667368..36e2f9d 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -823,12 +823,13 @@ static int gitdiff_unrecognized(const char *line, struct patch *patch)\n \n static const char *stop_at_slash(const char *line, int llen)\n {\n+\tint nslash = p_value;\n \tint i;\n \n \tfor (i = 0; i < llen; i++) {\n \t\tint ch = line[i];\n-\t\tif (ch == '/')\n-\t\t\treturn line + i;\n+\t\tif (ch == '/' && --nslash <= 0)\n+\t\t\treturn &line[i];\n \t}\n \treturn NULL;\n }\n"},{"id":"129454","messageId":"loom.20091207T222449-752@post.gmane.org","threadId":"21729","inReplyTo":"7vws1e3ma1.fsf@alter.siamese.dyndns.org","subject":"Re: git-apply fails on creating a new file, with both -p and --directory specified","fromName":"James Vega","fromEmail":"vega.james@gmail.com","sentAt":"2009-12-07T21:35:36Z","receivedAt":"2009-12-07T21:35:36Z","isPatch":false,"sender":{"key":"vega.james@gmail.com","avatar":"https://avatars.githubusercontent.com/u/112971?v=4"},"body":"Junio C Hamano pobox.com> writes:\n\n> \n> \"Steven J. Murdoch\" cl.cam.ac.uk> writes:\n> \n> > This appears to be because I was both using -p to strip some path\n> > components, and --directory to add different ones in. Only creating\n> > new files was affected.\n> \n> A very nicely done report.\n> \n> In addition to your test case, I suspect that a patch that only changes\n> mode would have acted funny with -p option.\n> \n\nIt looks like this may have introduced a bug when staging a file\nremoval. Here's an example git session showing the issue:\n\n$ git init test\nInitialized empty Git repository in /local_disk/tmp/test/.git/\n$ cd test\n$ echo \"foo\" > foo\n$ git add foo\n$ git commit -m 'Add foo'\n[master (root-commit) 3643b5d] Add foo\n 1 files changed, 1 insertions(+), 0 deletions(-)\n create mode 100644 foo\n$ mv foo bar\n$ git add -p\ndiff --git a/foo b/foo\nindex 257cc56..0000000\n--- a/foo\n+++ /dev/null\n@@ -1 +0,0 @@\n-foo\nStage this hunk [y,n,q,a,d,/,e,?]? y\n\n$ git status\n# On branch master\n# Changes to be committed:\n# (use \"git reset HEAD ...\" to unstage)\n#\n# new file: dev/null\n# deleted: foo\n#\n# Changed but not updated:\n# (use \"git add/rm ...\" to update what will be committed)\n# (use \"git checkout -- ...\" to discard changes in working directory)\n#\n# deleted: dev/null\n#\n# Untracked files:\n# (use \"git add ...\" to include in what will be committed)\n#\n# bar\n\nReplacing the 'git add -p' with\n\n git diff | sed '/^deleted file/d' | git apply --cached\n\nalso exhibits the problem.\n"},{"id":"129472","messageId":"7vk4wyqigf.fsf@alter.siamese.dyndns.org","threadId":"21729","inReplyTo":"loom.20091207T222449-752@post.gmane.org","subject":"Re: git-apply fails on creating a new file, with both -p and --directory specified","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-08T02:59:28Z","receivedAt":"2009-12-08T02:59:28Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"James Vega writes:\n\n> It looks like this may have introduced a bug when staging a file\n> removal. Here's an example git session showing the issue:\n> <>\n\nThanks for a report, but I cannot get the evidence that the said patch has\nanything to do with the issue you illustrated.\n\n$ cat >patch0 <<\\EOF\ndiff --git a/foo b/foo\ndeleted file mode 100644\nindex 257cc56..0000000\n--- a/foo\n+++ /dev/null\n@@ -1 +0,0 @@\n-foo\nEOF\n$ git apply --numstat patch0\n0\t1\tfoo\n$ sed -e '/deleted file/d' patch0 | git apply --numstat\n0\t1\tdev/null\n\nThe last one is showing the symptom in your message. Git versions 1.4.0\nand newer yield the same result, but 1.3.0 gives a funny message:\n\n ** warning: file dev/null becomes empty but is not deleted\n 0 1 foo\n\nSo it appears that the bug is somewhere else not in that patch.\n"},{"id":"129475","messageId":"7v3a3mqhhd.fsf@alter.siamese.dyndns.org","threadId":"21729","inReplyTo":"7vk4wyqigf.fsf@alter.siamese.dyndns.org","subject":"Re: git-apply fails on creating a new file, with both -p and --directory specified","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-08T03:20:30Z","receivedAt":"2009-12-08T03:20:30Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> James Vega writes:\n>\n>> It looks like this may have introduced a bug when staging a file\n>> removal. Here's an example git session showing the issue:\n\nAn update. I tried your reproduction recipe with 1.6.5.2 and it doesn't\nreproduce, but with 1.6.5.3 it does.\n\n$ git init test\nInitialized empty Git repository in /local_disk/tmp/test/.git/\n$ cd test\n$ echo \"foo\" > foo\n$ git add foo\n$ git commit -m 'Add foo'\n[master (root-commit) 3643b5d] Add foo\n 1 files changed, 1 insertions(+), 0 deletions(-)\n create mode 100644 foo\n$ mv foo bar\n$ git add -p\ndiff --git a/foo b/foo\nindex 257cc56..0000000\n--- a/foo\n+++ /dev/null\n@@ -1 +0,0 @@\n-foo\nStage this hunk [y,n,q,a,d,/,e,?]? y\n\n$ git status\n# On branch master\n# Changes to be committed:\n# (use \"git reset HEAD ...\" to unstage)\n#\n# new file: dev/null\n# deleted: foo\n#\n\nA quick bisection of the original issue points at\n\n24ab81a (add-interactive: handle deletion of empty files, 2009-10-27)\n"},{"id":"129479","messageId":"20091208033931.GK14401@jamessan.com","threadId":"21729","inReplyTo":"7vk4wyqigf.fsf@alter.siamese.dyndns.org","subject":"Re: git-apply fails on creating a new file, with both -p and --directory specified","fromName":"James Vega","fromEmail":"vega.james@gmail.com","sentAt":"2009-12-08T03:39:31Z","receivedAt":"2009-12-08T03:39:31Z","isPatch":false,"sender":{"key":"vega.james@gmail.com","avatar":"https://avatars.githubusercontent.com/u/112971?v=4"},"body":"On Mon, Dec 07, 2009 at 06:59:28PM -0800, Junio C Hamano wrote:\n> James Vega writes:\n> \n> > It looks like this may have introduced a bug when staging a file\n> > removal. Here's an example git session showing the issue:\n> > <>\n> \n> Thanks for a report, but I cannot get the evidence that the said patch has\n> anything to do with the issue you illustrated.\n\nRight, I incorrectly assumed the problem was with git-apply when I saw\nSteve's patch since the symptoms seemed similar.\n\nI just finished a bisect, though, and the problem is in removing a\nnon-empty file with \"git add -p\". This wasn't caught by existing tests\nbecause they only try to remove an empty file.\n\nThis was introduced in\n\n8f0bef6 (git-apply--interactive: Refactor patch mode code, 2009-08-13)\n\n-- \nJames\nGPG Key: 1024D/61326D40 2003-09-02 James Vega \n"},{"id":"129481","messageId":"20091208054724.GA21347@coredump.intra.peff.net","threadId":"21729","inReplyTo":"7v3a3mqhhd.fsf@alter.siamese.dyndns.org","subject":"Re: git-apply fails on creating a new file, with both -p and --directory specified","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-12-08T05:47:24Z","receivedAt":"2009-12-08T05:47:24Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 07, 2009 at 07:20:30PM -0800, Junio C Hamano wrote:\n\n> An update. I tried your reproduction recipe with 1.6.5.2 and it doesn't\n> reproduce, but with 1.6.5.3 it does.\n\nThanks, both, for a very helpful bug report. 24ab81a was totally bogus,\nbut we lacked a test for deleting a non-empty file. That test and a fix\nfor the problem are in the patch below.\n\nI am still slightly concerned that James's\n\n git diff | sed '/^deleted file/d' | git apply --cached\n\nbehaves as it does. What should git-apply do with a patch like:\n\n diff --git a/foo b/foo\n index 257cc56..0000000\n --- a/foo\n +++ /dev/null\n @@ -1 +0,0 @@\n -foo\n\n? I can see either turning it into a deletion patch (because /dev/null\nis special) or barfing (because /dev/null as a special case should have\nappeared in the \"diff\" line). But creating a dev/null file seems very\nwrong.\n\nBut maybe it is not worth worrying about too much. That patch format is\nnot generated intentionally by any known software.\n\nHere is the fix directly on top of 24ab81a.\n\n-- >8 --\nSubject: [PATCH] add-interactive: fix deletion of non-empty files\n\nCommit 24ab81a fixed the deletion of empty files, but broke\ndeletion of non-empty files. The approach it took was to\nfactor out the \"deleted\" line from the patch header into its\nown hunk, the same way we do for mode changes. However,\nunlike mode changes, we only showed the special \"delete this\nfile\" hunk if there were no other hunks. Otherwise, the user\nwould annoyingly be presented with _two_ hunks: one for\ndeleting the file and one for deleting the content.\n\nInstead, this patch takes a separate approach. We leave the\ndeletion line in the header, so it will be used as usual by\nnon-empty files if their deletion hunk is staged. For empty\nfiles, we create a deletion hunk with no content; it doesn't\nadd anything to the patch, but by staging it we trigger the\napplication of the header, which does contain the deletion.\n\nSigned-off-by: Jeff King \n---\nThere is a slightly different approach we could take, too: keep the\n\"deletion\" hunk as a first-class hunk, and just meld the content hunk's\noutput into it. Then both cases would get the \"Stage deletion\" question\ninstead of the \"Stage this hunk\" you get now for non-empty files (which\njust happens to trigger a deletion due to the headers).\n\nThat would take some refactoring, though, as pulling the deletion hunk\nout means we are re-ordering the headers. So right now if you did that\nyour ($head, @hunk) output would be something like:\n\n diff --git a/foo b/foo\n index 257cc56..0000000\n --- a/foo\n +++ /dev/null\n deleted file mode 100644\n @@ -1 +0,0 @@\n -foo\n\nwhich is pretty weird. On the other hand, we already do that funny\nordering for mode hunks, and git-apply is just fine with it. A mode hunk\nwith content change looks like this:\n\n diff --git a/foo b/foo\n index 257cc56..19c6cc1\n --- a/foo\n +++ b/foo\n old mode 100644\n new mode 100755\n\nAnd it also opens the door to editing the hunk to stop the deletion, but\nstill tweak the content change. Right now if you edit a deletion patch,\nyou can't remove the 'deleted' bit, and if your edit result keeps any\ncontent in the file, apply will complain. I'm not sure that particular\nfeature would be useful though (I have certainly never wanted it).\n\n git-add--interactive.perl | 18 +++++++++++-------\n t/t3701-add-interactive.sh | 20 ++++++++++++++++++++\n 2 files changed, 31 insertions(+), 7 deletions(-)\n\ndiff --git a/git-add--interactive.perl b/git-add--interactive.perl\nindex 35f4ef1..f4b95b1 100755\n--- a/git-add--interactive.perl\n+++ b/git-add--interactive.perl\n@@ -731,17 +731,19 @@ sub parse_diff_header {\n \n \tmy $head = { TEXT => [], DISPLAY => [], TYPE => 'header' };\n \tmy $mode = { TEXT => [], DISPLAY => [], TYPE => 'mode' };\n-\tmy $deletion = { TEXT => [], DISPLAY => [], TYPE => 'deletion' };\n+\tmy $is_deletion;\n \n \tfor (my $i = 0; $i < @{$src->{TEXT}}; $i++) {\n \t\tmy $dest =\n \t\t $src->{TEXT}->[$i] =~ /^(old|new) mode (\\d+)$/ ? $mode :\n-\t\t $src->{TEXT}->[$i] =~ /^deleted file/ ? $deletion :\n \t\t $head;\n \t\tpush @{$dest->{TEXT}}, $src->{TEXT}->[$i];\n \t\tpush @{$dest->{DISPLAY}}, $src->{DISPLAY}->[$i];\n+\t\tif ($src->{TEXT}->[$i] =~ /^deleted file/) {\n+\t\t\t$is_deletion = 1;\n+\t\t}\n \t}\n-\treturn ($head, $mode, $deletion);\n+\treturn ($head, $mode, $is_deletion);\n }\n \n sub hunk_splittable {\n@@ -1209,7 +1211,7 @@ sub patch_update_file {\n \tmy ($ix, $num);\n \tmy $path = shift;\n \tmy ($head, @hunk) = parse_diff($path);\n-\t($head, my $mode, my $deletion) = parse_diff_header($head);\n+\t($head, my $mode, my $is_deletion) = parse_diff_header($head);\n \tfor (@{$head->{DISPLAY}}) {\n \t\tprint;\n \t}\n@@ -1217,8 +1219,8 @@ sub patch_update_file {\n \tif (@{$mode->{TEXT}}) {\n \t\tunshift @hunk, $mode;\n \t}\n-\tif (@{$deletion->{TEXT}} && !@hunk) {\n-\t\t@hunk = ($deletion);\n+\tif ($is_deletion && !@hunk) {\n+\t\t@hunk = ({TEXT => [], DISPLAY => [], TYPE => 'deletion'});\n \t}\n \n \t$num = scalar @hunk;\n@@ -1441,14 +1443,16 @@ sub patch_update_file {\n \t@hunk = coalesce_overlapping_hunks(@hunk);\n \n \tmy $n_lofs = 0;\n+\tmy $hunks_used = 0;\n \tmy @result = ();\n \tfor (@hunk) {\n \t\tif ($_->{USE}) {\n \t\t\tpush @result, @{$_->{TEXT}};\n+\t\t\t$hunks_used++;\n \t\t}\n \t}\n \n-\tif (@result) {\n+\tif ($hunks_used) {\n \t\tmy $fh;\n \t\tmy @patch = (@{$head->{TEXT}}, @result);\n \t\tmy $apply_routine = $patch_mode_flavour{APPLY};\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex aa5909b..0926b91 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -215,6 +215,26 @@ test_expect_success 'add first line works' '\n '\n \n cat >expected <non-empty &&\n+\tgit add non-empty &&\n+\tgit commit -m non-empty &&\n+\trm non-empty &&\n+\techo y | git add -p non-empty &&\n+\tgit diff --cached >diff &&\n+\ttest_cmp expected diff\n+'\n+\n+cat >expected < There is a slightly different approach we could take, too: keep the\n> \"deletion\" hunk as a first-class hunk, and just meld the content hunk's\n> output into it. Then both cases would get the \"Stage deletion\" question\n> instead of the \"Stage this hunk\" you get now for non-empty files (which\n> just happens to trigger a deletion due to the headers).\n\nBTW, the code for this is the much smaller change below. If you prefer\nthat, I can squash in the test and write up an appropriate commit\nmessage.\n\ndiff --git a/git-add--interactive.perl b/git-add--interactive.perl\nindex 35f4ef1..02e97b9 100755\n--- a/git-add--interactive.perl\n+++ b/git-add--interactive.perl\n@@ -1217,7 +1217,11 @@ sub patch_update_file {\n \tif (@{$mode->{TEXT}}) {\n \t\tunshift @hunk, $mode;\n \t}\n-\tif (@{$deletion->{TEXT}} && !@hunk) {\n+\tif (@{$deletion->{TEXT}}) {\n+\t\tforeach my $hunk (@hunk) {\n+\t\t\tpush @{$deletion->{TEXT}}, @{$hunk->{TEXT}};\n+\t\t\tpush @{$deletion->{DISPLAY}}, @{$hunk->{DISPLAY}};\n+\t\t}\n \t\t@hunk = ($deletion);\n \t}\n \n"},{"id":"129490","messageId":"20091208064944.GM14401@jamessan.com","threadId":"21729","inReplyTo":"20091208060109.GB9951@coredump.intra.peff.net","subject":"Re: git-apply fails on creating a new file, with both -p and --directory specified","fromName":"James Vega","fromEmail":"vega.james@gmail.com","sentAt":"2009-12-08T06:49:44Z","receivedAt":"2009-12-08T06:49:44Z","isPatch":false,"sender":{"key":"vega.james@gmail.com","avatar":"https://avatars.githubusercontent.com/u/112971?v=4"},"body":"On Tue, Dec 08, 2009 at 01:01:09AM -0500, Jeff King wrote:\n> On Tue, Dec 08, 2009 at 12:47:24AM -0500, Jeff King wrote:\n> \n> > There is a slightly different approach we could take, too: keep the\n> > \"deletion\" hunk as a first-class hunk, and just meld the content hunk's\n> > output into it. Then both cases would get the \"Stage deletion\" question\n> > instead of the \"Stage this hunk\" you get now for non-empty files (which\n> > just happens to trigger a deletion due to the headers).\n> \n> BTW, the code for this is the much smaller change below. If you prefer\n> that, I can squash in the test and write up an appropriate commit\n> message.\n> \n> diff --git a/git-add--interactive.perl b/git-add--interactive.perl\n> index 35f4ef1..02e97b9 100755\n> --- a/git-add--interactive.perl\n> +++ b/git-add--interactive.perl\n> @@ -1217,7 +1217,11 @@ sub patch_update_file {\n> \tif (@{$mode->{TEXT}}) {\n> \t\tunshift @hunk, $mode;\n> \t}\n> -\tif (@{$deletion->{TEXT}} && !@hunk) {\n> +\tif (@{$deletion->{TEXT}}) {\n> +\t\tforeach my $hunk (@hunk) {\n> +\t\t\tpush @{$deletion->{TEXT}}, @{$hunk->{TEXT}};\n> +\t\t\tpush @{$deletion->{DISPLAY}}, @{$hunk->{DISPLAY}};\n> +\t\t}\n> \t\t@hunk = ($deletion);\n> \t}\n> \n\nThanks for the quick patches. This was similar to what I was working on, but\ncleaner than what I had. Works well for me.\n\n-- \nJames\nGPG Key: 1024D/61326D40 2003-09-02 James Vega \n"},{"id":"129491","messageId":"7vvdgindo3.fsf@alter.siamese.dyndns.org","threadId":"21729","inReplyTo":"20091208054724.GA21347@coredump.intra.peff.net","subject":"Re: git-apply fails on creating a new file, with both -p and --directory specified","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-08T07:11:08Z","receivedAt":"2009-12-08T07:11:08Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Dec 07, 2009 at 07:20:30PM -0800, Junio C Hamano wrote:\n>\n>> An update. I tried your reproduction recipe with 1.6.5.2 and it doesn't\n>> reproduce, but with 1.6.5.3 it does.\n>\n> Thanks, both, for a very helpful bug report. 24ab81a was totally bogus,\n> but we lacked a test for deleting a non-empty file. That test and a fix\n> for the problem are in the patch below.\n\nThanks.\n\n> I am still slightly concerned that James's\n>\n> git diff | sed '/^deleted file/d' | git apply --cached\n>\n> behaves as it does. What should git-apply do with a patch like:\n>\n> diff --git a/foo b/foo\n> index 257cc56..0000000\n> --- a/foo\n> +++ /dev/null\n> @@ -1 +0,0 @@\n> -foo\n>\n> ? I can see either turning it into a deletion patch (because /dev/null\n> is special) or barfing (because /dev/null as a special case should have\n> appeared in the \"diff\" line). But creating a dev/null file seems very\n> wrong.\n\nI was wondering about the same thing while bisecting. By the current\ndefinition of \"diff --git\", removing the \"deleted file\" or \"new file\" line\nmakes the patch an invalid \"git format diff\". See the beginning of\nparse_git_header() where we say \"we don't guess\" and initialize both\nis_new and is_delete to false (and we flip them upon seeing \"deleted file\"\nand \"new file\", but never with \"/dev/null\").\n\n> But maybe it is not worth worrying about too much. That patch format is\n> not generated intentionally by any known software.\n\nI think some recent other SCMs produce what they claim to be \"diff --git\",\nbut I don't know if they implement the format correctly enough. I am not\nworried about their implemention of binary patches (if they do not\nimplement it correctly they will most likely get garbage), but do they get\nthe abbreviated hash on the \"index\" line correctly? You can put garbage\non the line and most of the time it would work but it will break \"am -3\"\nby breaking \"apply --build-fake-ancestor\".\n\nI just checked \"hg diff --git\"; at least it shows \"deleted file\".\n\n> That would take some refactoring, though, as pulling the deletion hunk\n> out means we are re-ordering the headers. So right now if you did that\n> your ($head, @hunk) output would be something like:\n>\n> diff --git a/foo b/foo\n> index 257cc56..0000000\n> --- a/foo\n> +++ /dev/null\n> deleted file mode 100644\n> @@ -1 +0,0 @@\n> -foo\n>\n> which is pretty weird.\n\nI agree it is weird.\n\n> And it also opens the door to editing the hunk to stop the deletion, but\n> still tweak the content change. Right now if you edit a deletion patch,\n> you can't remove the 'deleted' bit, and if your edit result keeps any\n> content in the file, apply will complain. I'm not sure that particular\n> feature would be useful though (I have certainly never wanted it).\n\nInteresting. Does \"add -p\" (especially its [e]dit codepath) know enough\nabout what it is doing? If so, it should be able to add \"deleted file\" on\nits own (and remove it when the result of editing and picking hunks makes\nthe patch a non-deletion). For example, if you have a two-liner in the\nindex and have deleted one line in the work tree, and run \"add -p\":\n\n diff --git a/foo b/foo\n index 3bd1f0e..257cc56 100644\n --- a/foo\n +++ b/foo\n @@ -1,2 +1 @@\n foo\n -bar\n\nyou *should* be able to edit it into a patch that removes all lines.\n\nPerhaps the \"add -i\" at the end should offer, after noticing that the\nchosen and edited hunks will make the postimage an empty file, a chance\nfor the user to say \"I not only want to remove the contents from the path,\nbut want to remove the path itself\" in such a case?\n\nI dunno.\n"},{"id":"129495","messageId":"7v3a3lorge.fsf@alter.siamese.dyndns.org","threadId":"21729","inReplyTo":"20091208060109.GB9951@coredump.intra.peff.net","subject":"Re: git-apply fails on creating a new file, with both -p and --directory specified","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-08T07:28:01Z","receivedAt":"2009-12-08T07:28:01Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Dec 08, 2009 at 12:47:24AM -0500, Jeff King wrote:\n>\n>> There is a slightly different approach we could take, too: keep the\n>> \"deletion\" hunk as a first-class hunk, and just meld the content hunk's\n>> output into it. Then both cases would get the \"Stage deletion\" question\n>> instead of the \"Stage this hunk\" you get now for non-empty files (which\n>> just happens to trigger a deletion due to the headers).\n>\n> BTW, the code for this is the much smaller change below. If you prefer\n> that, I can squash in the test and write up an appropriate commit\n> message.\n\nDoubly interesting, as I recall reading \"That would take some refactoring,\nthough, as pulling the deletion hunk\"\n\n ... goes and looks ...\n\nAh, Ok, the \"refactoring\" refers to the \"header reordering weirdness\".\n\nThat might be something we may want to fix someday, when we find ourselves\nneeding to add a feature to turn deletion into non-deletion or vice versa\nduring \"add -p\" [e]dit, as I suspect that the \"hunk editing\" codepath does\nnot keep track of what the user's patch is doing, to the point that it\ndoes not even know how many lines there are supposed to be in the\nresulting hunk that it asks \"git apply\" to recount. There is no way to\nadd/delete \"deleted file\" line if the logic does not know what the patch\nis doing.\n\nBut someday is not today. I think this six-liner is preferable.\n\n> diff --git a/git-add--interactive.perl b/git-add--interactive.perl\n> index 35f4ef1..02e97b9 100755\n> --- a/git-add--interactive.perl\n> +++ b/git-add--interactive.perl\n> @@ -1217,7 +1217,11 @@ sub patch_update_file {\n> \tif (@{$mode->{TEXT}}) {\n> \t\tunshift @hunk, $mode;\n> \t}\n> -\tif (@{$deletion->{TEXT}} && !@hunk) {\n> +\tif (@{$deletion->{TEXT}}) {\n> +\t\tforeach my $hunk (@hunk) {\n> +\t\t\tpush @{$deletion->{TEXT}}, @{$hunk->{TEXT}};\n> +\t\t\tpush @{$deletion->{DISPLAY}}, @{$hunk->{DISPLAY}};\n> +\t\t}\n> \t\t@hunk = ($deletion);\n> \t}\n> \n"},{"id":"129496","messageId":"20091208073859.GA12049@coredump.intra.peff.net","threadId":"21729","inReplyTo":"7vvdgindo3.fsf@alter.siamese.dyndns.org","subject":"Re: git-apply fails on creating a new file, with both -p and --directory specified","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-12-08T07:38:59Z","receivedAt":"2009-12-08T07:38:59Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 07, 2009 at 11:11:08PM -0800, Junio C Hamano wrote:\n\n> I was wondering about the same thing while bisecting. By the current\n> definition of \"diff --git\", removing the \"deleted file\" or \"new file\" line\n> makes the patch an invalid \"git format diff\". See the beginning of\n> parse_git_header() where we say \"we don't guess\" and initialize both\n> is_new and is_delete to false (and we flip them upon seeing \"deleted file\"\n> and \"new file\", but never with \"/dev/null\").\n\nHmm. In that case, I think converting it to deletion is definitely\nwrong; \"diff --git\" is about not guessing. So the only question is\nwhether it should be flagged as an error. I was somewhat worried that\nyou could produce a patch which would make apply complain by doing \"git\ndiff /dev/null /your/file\". But actually that already produces a \"new\nfile\" header (and the opposite produces a \"deleted file\" header).\n\nSo I think the patch below would notice both the new and deleted cases,\nand is probably a good thing.\n\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex c8372a0..43a1535 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -673,8 +673,12 @@ static int gitdiff_hdrend(const char *line, struct patch *patch)\n */\n static char *gitdiff_verify_name(const char *line, int isnull, char *orig_name, const char *oldnew)\n {\n-\tif (!orig_name && !isnull)\n+\tif (!orig_name && !isnull) {\n+\t\tif (!memcmp(line, \"/dev/null\\n\", 10))\n+\t\t\tdie(\"git apply: bad git-diff - expected filename, got /dev/null on line %d\", linenr);\n \t\treturn find_name(line, NULL, p_value, TERM_TAB);\n+\t}\n+\n \n \tif (orig_name) {\n \t\tint len;\n\n> I think some recent other SCMs produce what they claim to be \"diff --git\",\n> but I don't know if they implement the format correctly enough. I am not\n> worried about their implemention of binary patches (if they do not\n> implement it correctly they will most likely get garbage), but do they get\n> the abbreviated hash on the \"index\" line correctly? You can put garbage\n> on the line and most of the time it would work but it will break \"am -3\"\n> by breaking \"apply --build-fake-ancestor\".\n> \n> I just checked \"hg diff --git\"; at least it shows \"deleted file\".\n\nUgh. A whole new source of problems. :) I am not too interested in\nseeking out and evaluating other SCM's implementations; I think we\nshould wait for people who actually use those systems to find\ninteroperability bugs, determine whether they are or are not simply bugs\nin the other people's implementations, and then report the bug to us.\n\n> > That would take some refactoring, though, as pulling the deletion hunk\n> > out means we are re-ordering the headers. So right now if you did that\n> > your ($head, @hunk) output would be something like:\n> >\n> > diff --git a/foo b/foo\n> > index 257cc56..0000000\n> > --- a/foo\n> > +++ /dev/null\n> > deleted file mode 100644\n> > @@ -1 +0,0 @@\n> > -foo\n> >\n> > which is pretty weird.\n> \n> I agree it is weird.\n\nNote that we already do this for mode changes which also have a content\nchange. They look like:\n\n diff --git a/foo b/foo\n index 257cc56..19c6cc1\n --- a/foo\n +++ b/foo\n old mode 100644\n new mode 100755\n Stage mode change [y,n,q,a,d,/,j,J,g,?]?\n @@ -1 +1,2 @@\n foo\n +content\n Stage this hunk [y,n,q,a,d,/,K,g,e,?]?\n\n> Interesting. Does \"add -p\" (especially its [e]dit codepath) know enough\n> about what it is doing? If so, it should be able to add \"deleted file\" on\n> its own (and remove it when the result of editing and picking hunks makes\n> the patch a non-deletion). For example, if you have a two-liner in the\n> index and have deleted one line in the work tree, and run \"add -p\":\n\nNo, it doesn't know enough now. That would be part of the refactoring I\nmentioned. I'm not sure how useful it is to support this. I can see\ngoing from \"I had hunk A, but I really wanted to tweak it to hunk B\". I\ncan't think of a single time I've wanted \"I deleted the entire file, but\nI really wanted to keep 2 lines\". And if I did, I would probably just:\n\n git checkout file\n $EDITOR file\n git add -p file\n\n> Perhaps the \"add -i\" at the end should offer, after noticing that the\n> chosen and edited hunks will make the postimage an empty file, a chance\n> for the user to say \"I not only want to remove the contents from the path,\n> but want to remove the path itself\" in such a case?\n> \n> I dunno.\n\nI would not say no to such a patch, but I really have no interest in\nwriting it myself.\n\n-Peff\n"},{"id":"129498","messageId":"20091208074935.GB12049@coredump.intra.peff.net","threadId":"21729","inReplyTo":"7v3a3lorge.fsf@alter.siamese.dyndns.org","subject":"Re: git-apply fails on creating a new file, with both -p and --directory specified","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-12-08T07:49:35Z","receivedAt":"2009-12-08T07:49:35Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 07, 2009 at 11:28:01PM -0800, Junio C Hamano wrote:\n\n> That might be something we may want to fix someday, when we find ourselves\n> needing to add a feature to turn deletion into non-deletion or vice versa\n> during \"add -p\" [e]dit, as I suspect that the \"hunk editing\" codepath does\n> not keep track of what the user's patch is doing, to the point that it\n> does not even know how many lines there are supposed to be in the\n> resulting hunk that it asks \"git apply\" to recount. There is no way to\n> add/delete \"deleted file\" line if the logic does not know what the patch\n> is doing.\n> \n> But someday is not today. I think this six-liner is preferable.\n\nOK, here it is with the test and an amended commit message. You could\nalmost do an [e]dit on this and delete the \"deleted\" line, but you have\nno way of fixing up the \"+++ /dev/null\" line. For now, we have\ndisabled [e]dit entirely for non-content hunks, so at least you cannot\nget yourself into trouble creating a broken patch. :)\n\n-- >8 --\nSubject: [PATCH] add-interactive: fix deletion of non-empty files\n\nCommit 24ab81a fixed the deletion of empty files, but broke\ndeletion of non-empty files. The approach it took was to\nfactor out the \"deleted\" line from the patch header into its\nown hunk, the same way we do for mode changes. However,\nunlike mode changes, we only showed the special \"delete this\nfile\" hunk if there were no other hunks. Otherwise, the user\nwould annoyingly be presented with _two_ hunks: one for\ndeleting the file and one for deleting the content.\n\nThis meant that in the non-empty case, we forgot about the\ndeleted line entirely, and we submitted a bogus patch to\ngit-apply (with \"/dev/null\" as the destination file, but not\nmarked as a deletion).\n\nInstead, this patch combines the file deletion hunk and the\ncontent deletion hunk (if there is one) into a single\ndeletion hunk which is either staged or not.\n\nSigned-off-by: Jeff King \n---\n git-add--interactive.perl | 6 +++++-\n t/t3701-add-interactive.sh | 20 ++++++++++++++++++++\n 2 files changed, 25 insertions(+), 1 deletions(-)\n\ndiff --git a/git-add--interactive.perl b/git-add--interactive.perl\nindex 35f4ef1..02e97b9 100755\n--- a/git-add--interactive.perl\n+++ b/git-add--interactive.perl\n@@ -1217,7 +1217,11 @@ sub patch_update_file {\n \tif (@{$mode->{TEXT}}) {\n \t\tunshift @hunk, $mode;\n \t}\n-\tif (@{$deletion->{TEXT}} && !@hunk) {\n+\tif (@{$deletion->{TEXT}}) {\n+\t\tforeach my $hunk (@hunk) {\n+\t\t\tpush @{$deletion->{TEXT}}, @{$hunk->{TEXT}};\n+\t\t\tpush @{$deletion->{DISPLAY}}, @{$hunk->{DISPLAY}};\n+\t\t}\n \t\t@hunk = ($deletion);\n \t}\n \ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex aa5909b..0926b91 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -215,6 +215,26 @@ test_expect_success 'add first line works' '\n '\n \n cat >expected <non-empty &&\n+\tgit add non-empty &&\n+\tgit commit -m non-empty &&\n+\trm non-empty &&\n+\techo y | git add -p non-empty &&\n+\tgit diff --cached >diff &&\n+\ttest_cmp expected diff\n+'\n+\n+cat >expected < writes:\n\n> OK, here it is with the test and an amended commit message. You could\n> almost do an [e]dit on this and delete the \"deleted\" line, but you have\n> no way of fixing up the \"+++ /dev/null\" line. For now, we have\n> disabled [e]dit entirely for non-content hunks, so at least you cannot\n> get yourself into trouble creating a broken patch. :)\n\n;-)\n\nThanks.\n"}]}