git-apply fails on creating a new file, with both -p and --directory specified

git-apply fails on creating a new file, with both -p and --directory specified

[Steven J. Murdoch]

While trying to apply a patch from one repository (created by
git-format-patch), to another (using git-am), git fails with:

"fatal: git apply: bad git-diff - inconsistent new filename on line X"

This appears to be because I was both using -p to strip some path
components, and --directory to add different ones in. Only creating
new files was affected.

This was the case in git 1.6.5.2, and also the development version
1.6.6.rc0.15.g4fa80. I have tested this on MacOS X Snow Leopard.

This appears related to the bug discussed in:
  http://marc.info/?l=git&m=122237537312597&w=2
in which the following fix was posted:
  http://git.kernel.org/?p=git/git.git;a=commitdiff;h=969c877506cf8cc760c7b251fef6c5b6850bfc19

I have included a patch below to the test cases, which currently fails
but, if I understand correctly, should succeed.

Steven Murdoch.

-- >8 --
Test git-apply creating a new file, combining --directory and -p flags

Signed-off-by: Steven Murdoch <Steven.Murdoch@cl.cam.ac.uk>
---
 t/t4128-apply-root.sh |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/t/t4128-apply-root.sh b/t/t4128-apply-root.sh
index 8f6aea4..6cc741a 100755
--- a/t/t4128-apply-root.sh
+++ b/t/t4128-apply-root.sh
@@ -58,6 +58,23 @@ test_expect_success 'apply --directory (new file)' '
 '
 
 cat > patch << EOF
+diff --git a/c/newfile2 b/c/newfile2
+new file mode 100644
+index 0000000..d95f3ad
+--- /dev/null
++++ b/c/newfile2
+@@ -0,0 +1 @@
++content
+EOF
+
+test_expect_success 'apply --directory -p (new file)' '
+	git reset --hard initial &&
+	git apply -p2 --directory=some/sub/dir/ --index patch &&
+	test content = $(git show :some/sub/dir/newfile2) &&
+	test content = $(cat some/sub/dir/newfile2)
+'
+
+cat > patch << EOF
 diff --git a/delfile b/delfile
 deleted file mode 100644
 index d95f3ad..0000000
-- 
1.6.5.2

-- 
http://www.cl.cam.ac.uk/users/sjm217/

Re: git-apply fails on creating a new file, with both -p and --directory specified

[Junio C Hamano]

"Steven J. Murdoch" <git+Steven.Murdoch@cl.cam.ac.uk> writes:

> This appears to be because I was both using -p to strip some path
> components, and --directory to add different ones in. Only creating
> new files was affected.

A very nicely done report.

In addition to your test case, I suspect that a patch that only changes
mode would have acted funny with -p<n> option.

-- >8 --
[PATCH] builtin-apply.c: pay attention to -p<n> when determining the name

The patch structure has def_name component that is used to validate the
sanity of a "diff --git" patch by checking pathnames that appear on the
patch header lines for consistency.  The git_header_name() function is
used to compute this out of "diff --git a/... b/..." line, but the code
always stripped one level of prefix (i.e. "a/" and "b/"), without paying
attention to -p<n> option.  Code in find_name() function that parses other
lines in the patch header (e.g. "--- a/..." and "+++ b/..." lines) however
did strip the correct number of leading paths prefixes, and the sanity
check between these computed values failed.

Teach git_header_name() to honor -p<n> option like find_name() function
does.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-apply.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/builtin-apply.c b/builtin-apply.c
index f667368..36e2f9d 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -823,12 +823,13 @@ static int gitdiff_unrecognized(const char *line, struct patch *patch)
 
 static const char *stop_at_slash(const char *line, int llen)
 {
+	int nslash = p_value;
 	int i;
 
 	for (i = 0; i < llen; i++) {
 		int ch = line[i];
-		if (ch == '/')
-			return line + i;
+		if (ch == '/' && --nslash <= 0)
+			return &line[i];
 	}
 	return NULL;
 }

Re: git-apply fails on creating a new file, with both -p and --directory specified

[James Vega]

Junio C Hamano <gitster <at> pobox.com> writes:

> 
> "Steven J. Murdoch" <git+Steven.Murdoch <at> cl.cam.ac.uk> writes:
> 
> > This appears to be because I was both using -p to strip some path
> > components, and --directory to add different ones in. Only creating
> > new files was affected.
> 
> A very nicely done report.
> 
> In addition to your test case, I suspect that a patch that only changes
> mode would have acted funny with -p<n> option.
> 

It looks like this may have introduced a bug when staging a file
removal.  Here's an example git session showing the issue:

$ git init test
Initialized empty Git repository in /local_disk/tmp/test/.git/
$ cd test
$ echo "foo" > foo
$ git add foo
$ git commit -m 'Add foo'
[master (root-commit) 3643b5d] Add foo
 1 files changed, 1 insertions(+), 0 deletions(-)
 create mode 100644 foo
$ mv foo bar
$ git add -p
diff --git a/foo b/foo
index 257cc56..0000000
--- a/foo
+++ /dev/null
@@ -1 +0,0 @@
-foo
Stage this hunk [y,n,q,a,d,/,e,?]? y

$ git status
# On branch master
# Changes to be committed:
#   (use "git reset HEAD ..." to unstage)
#
#       new file:   dev/null
#       deleted:    foo
#
# Changed but not updated:
#   (use "git add/rm ..." to update what will be committed)
#   (use "git checkout -- ..." to discard changes in working directory)
#
#       deleted:    dev/null
#
# Untracked files:
#   (use "git add ..." to include in what will be committed)
#
#       bar

Replacing the 'git add -p' with

  git diff | sed '/^deleted file/d' | git apply --cached

also exhibits the problem.

Re: git-apply fails on creating a new file, with both -p and --directory specified

[Junio C Hamano]

James Vega <vega.james@gmail.com> writes:

> It looks like this may have introduced a bug when staging a file
> removal.  Here's an example git session showing the issue:
> <<snipped>>

Thanks for a report, but I cannot get the evidence that the said patch has
anything to do with the issue you illustrated.

$ cat >patch0 <<\EOF
diff --git a/foo b/foo
deleted file mode 100644
index 257cc56..0000000
--- a/foo
+++ /dev/null
@@ -1 +0,0 @@
-foo
EOF
$ git apply --numstat patch0
0	1	foo
$ sed -e '/deleted file/d' patch0 | git apply --numstat
0	1	dev/null

The last one is showing the symptom in your message.  Git versions 1.4.0
and newer yield the same result, but 1.3.0 gives a funny message:

        ** warning: file dev/null becomes empty but is not deleted
        0       1       foo

So it appears that the bug is somewhere else not in that patch.

Re: git-apply fails on creating a new file, with both -p and --directory specified

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> James Vega <vega.james@gmail.com> writes:
>
>> It looks like this may have introduced a bug when staging a file
>> removal.  Here's an example git session showing the issue:

An update.  I tried your reproduction recipe with 1.6.5.2 and it doesn't
reproduce, but with 1.6.5.3 it does.

$ git init test
Initialized empty Git repository in /local_disk/tmp/test/.git/
$ cd test
$ echo "foo" > foo
$ git add foo
$ git commit -m 'Add foo'
[master (root-commit) 3643b5d] Add foo
 1 files changed, 1 insertions(+), 0 deletions(-)
 create mode 100644 foo
$ mv foo bar
$ git add -p
diff --git a/foo b/foo
index 257cc56..0000000
--- a/foo
+++ /dev/null
@@ -1 +0,0 @@
-foo
Stage this hunk [y,n,q,a,d,/,e,?]? y

$ git status
# On branch master
# Changes to be committed:
#   (use "git reset HEAD ..." to unstage)
#
#       new file:   dev/null
#       deleted:    foo
#

A quick bisection of the original issue points at

24ab81a (add-interactive: handle deletion of empty files, 2009-10-27)

Re: git-apply fails on creating a new file, with both -p and --directory specified

[James Vega]

On Mon, Dec 07, 2009 at 06:59:28PM -0800, Junio C Hamano wrote:
> James Vega <vega.james@gmail.com> writes:
> 
> > It looks like this may have introduced a bug when staging a file
> > removal.  Here's an example git session showing the issue:
> > <<snipped>>
> 
> Thanks for a report, but I cannot get the evidence that the said patch has
> anything to do with the issue you illustrated.

Right, I incorrectly assumed the problem was with git-apply when I saw
Steve's patch since the symptoms seemed similar.

I just finished a bisect, though, and the problem is in removing a
non-empty file with "git add -p".  This wasn't caught by existing tests
because they only try to remove an empty file.

This was introduced in

8f0bef6 (git-apply--interactive: Refactor patch mode code, 2009-08-13)

-- 
James
GPG Key: 1024D/61326D40 2003-09-02 James Vega <vega.james@gmail.com>

Re: git-apply fails on creating a new file, with both -p and --directory specified

[Jeff King]

On Mon, Dec 07, 2009 at 07:20:30PM -0800, Junio C Hamano wrote:

> An update.  I tried your reproduction recipe with 1.6.5.2 and it doesn't
> reproduce, but with 1.6.5.3 it does.

Thanks, both, for a very helpful bug report.  24ab81a was totally bogus,
but we lacked a test for deleting a non-empty file. That test and a fix
for the problem are in the patch below.

I am still slightly concerned that James's

  git diff | sed '/^deleted file/d' | git apply --cached

behaves as it does. What should git-apply do with a patch like:

  diff --git a/foo b/foo
  index 257cc56..0000000
  --- a/foo
  +++ /dev/null
  @@ -1 +0,0 @@
  -foo

? I can see either turning it into a deletion patch (because /dev/null
is special) or barfing (because /dev/null as a special case should have
appeared in the "diff" line). But creating a dev/null file seems very
wrong.

But maybe it is not worth worrying about too much. That patch format is
not generated intentionally by any known software.

Here is the fix directly on top of 24ab81a.

-- >8 --
Subject: [PATCH] add-interactive: fix deletion of non-empty files

Commit 24ab81a fixed the deletion of empty files, but broke
deletion of non-empty files. The approach it took was to
factor out the "deleted" line from the patch header into its
own hunk, the same way we do for mode changes. However,
unlike mode changes, we only showed the special "delete this
file" hunk if there were no other hunks. Otherwise, the user
would annoyingly be presented with _two_ hunks: one for
deleting the file and one for deleting the content.

Instead, this patch takes a separate approach. We leave the
deletion line in the header, so it will be used as usual by
non-empty files if their deletion hunk is staged. For empty
files, we create a deletion hunk with no content; it doesn't
add anything to the patch, but by staging it we trigger the
application of the header, which does contain the deletion.

Signed-off-by: Jeff King <peff@peff.net>
---
There is a slightly different approach we could take, too: keep the
"deletion" hunk as a first-class hunk, and just meld the content hunk's
output into it. Then both cases would get the "Stage deletion" question
instead of the "Stage this hunk" you get now for non-empty files (which
just happens to trigger a deletion due to the headers).

That would take some refactoring, though, as pulling the deletion hunk
out means we are re-ordering the headers. So right now if you did that
your ($head, @hunk) output would be something like:

       diff --git a/foo b/foo
       index 257cc56..0000000
       --- a/foo
       +++ /dev/null
       deleted file mode 100644
       @@ -1 +0,0 @@
       -foo

which is pretty weird. On the other hand, we already do that funny
ordering for mode hunks, and git-apply is just fine with it. A mode hunk
with content change looks like this:

       diff --git a/foo b/foo
       index 257cc56..19c6cc1
       --- a/foo
       +++ b/foo
       old mode 100644
       new mode 100755

And it also opens the door to editing the hunk to stop the deletion, but
still tweak the content change. Right now if you edit a deletion patch,
you can't remove the 'deleted' bit, and if your edit result keeps any
content in the file, apply will complain. I'm not sure that particular
feature would be useful though (I have certainly never wanted it).

 git-add--interactive.perl  |   18 +++++++++++-------
 t/t3701-add-interactive.sh |   20 ++++++++++++++++++++
 2 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 35f4ef1..f4b95b1 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -731,17 +731,19 @@ sub parse_diff_header {
 
 	my $head = { TEXT => [], DISPLAY => [], TYPE => 'header' };
 	my $mode = { TEXT => [], DISPLAY => [], TYPE => 'mode' };
-	my $deletion = { TEXT => [], DISPLAY => [], TYPE => 'deletion' };
+	my $is_deletion;
 
 	for (my $i = 0; $i < @{$src->{TEXT}}; $i++) {
 		my $dest =
 		   $src->{TEXT}->[$i] =~ /^(old|new) mode (\d+)$/ ? $mode :
-		   $src->{TEXT}->[$i] =~ /^deleted file/ ? $deletion :
 		   $head;
 		push @{$dest->{TEXT}}, $src->{TEXT}->[$i];
 		push @{$dest->{DISPLAY}}, $src->{DISPLAY}->[$i];
+		if ($src->{TEXT}->[$i] =~ /^deleted file/) {
+			$is_deletion = 1;
+		}
 	}
-	return ($head, $mode, $deletion);
+	return ($head, $mode, $is_deletion);
 }
 
 sub hunk_splittable {
@@ -1209,7 +1211,7 @@ sub patch_update_file {
 	my ($ix, $num);
 	my $path = shift;
 	my ($head, @hunk) = parse_diff($path);
-	($head, my $mode, my $deletion) = parse_diff_header($head);
+	($head, my $mode, my $is_deletion) = parse_diff_header($head);
 	for (@{$head->{DISPLAY}}) {
 		print;
 	}
@@ -1217,8 +1219,8 @@ sub patch_update_file {
 	if (@{$mode->{TEXT}}) {
 		unshift @hunk, $mode;
 	}
-	if (@{$deletion->{TEXT}} && !@hunk) {
-		@hunk = ($deletion);
+	if ($is_deletion && !@hunk) {
+		@hunk = ({TEXT => [], DISPLAY => [], TYPE => 'deletion'});
 	}
 
 	$num = scalar @hunk;
@@ -1441,14 +1443,16 @@ sub patch_update_file {
 	@hunk = coalesce_overlapping_hunks(@hunk);
 
 	my $n_lofs = 0;
+	my $hunks_used = 0;
 	my @result = ();
 	for (@hunk) {
 		if ($_->{USE}) {
 			push @result, @{$_->{TEXT}};
+			$hunks_used++;
 		}
 	}
 
-	if (@result) {
+	if ($hunks_used) {
 		my $fh;
 		my @patch = (@{$head->{TEXT}}, @result);
 		my $apply_routine = $patch_mode_flavour{APPLY};
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index aa5909b..0926b91 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -215,6 +215,26 @@ test_expect_success 'add first line works' '
 '
 
 cat >expected <<EOF
+diff --git a/non-empty b/non-empty
+deleted file mode 100644
+index d95f3ad..0000000
+--- a/non-empty
++++ /dev/null
+@@ -1 +0,0 @@
+-content
+EOF
+test_expect_success 'deleting a non-empty file' '
+	git reset --hard &&
+	echo content >non-empty &&
+	git add non-empty &&
+	git commit -m non-empty &&
+	rm non-empty &&
+	echo y | git add -p non-empty &&
+	git diff --cached >diff &&
+	test_cmp expected diff
+'
+
+cat >expected <<EOF
 diff --git a/empty b/empty
 deleted file mode 100644
 index e69de29..0000000
-- 
1.6.5.1.g24ab.dirty

Re: git-apply fails on creating a new file, with both -p and --directory specified

[Jeff King]

On Tue, Dec 08, 2009 at 12:47:24AM -0500, Jeff King wrote:

> There is a slightly different approach we could take, too: keep the
> "deletion" hunk as a first-class hunk, and just meld the content hunk's
> output into it. Then both cases would get the "Stage deletion" question
> instead of the "Stage this hunk" you get now for non-empty files (which
> just happens to trigger a deletion due to the headers).

BTW, the code for this is the much smaller change below. If you prefer
that, I can squash in the test and write up an appropriate commit
message.

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 35f4ef1..02e97b9 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1217,7 +1217,11 @@ sub patch_update_file {
 	if (@{$mode->{TEXT}}) {
 		unshift @hunk, $mode;
 	}
-	if (@{$deletion->{TEXT}} && !@hunk) {
+	if (@{$deletion->{TEXT}}) {
+		foreach my $hunk (@hunk) {
+			push @{$deletion->{TEXT}}, @{$hunk->{TEXT}};
+			push @{$deletion->{DISPLAY}}, @{$hunk->{DISPLAY}};
+		}
 		@hunk = ($deletion);
 	}
 

Re: git-apply fails on creating a new file, with both -p and --directory specified

[James Vega]

On Tue, Dec 08, 2009 at 01:01:09AM -0500, Jeff King wrote:
> On Tue, Dec 08, 2009 at 12:47:24AM -0500, Jeff King wrote:
> 
> > There is a slightly different approach we could take, too: keep the
> > "deletion" hunk as a first-class hunk, and just meld the content hunk's
> > output into it. Then both cases would get the "Stage deletion" question
> > instead of the "Stage this hunk" you get now for non-empty files (which
> > just happens to trigger a deletion due to the headers).
> 
> BTW, the code for this is the much smaller change below. If you prefer
> that, I can squash in the test and write up an appropriate commit
> message.
> 
> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index 35f4ef1..02e97b9 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -1217,7 +1217,11 @@ sub patch_update_file {
>  	if (@{$mode->{TEXT}}) {
>  		unshift @hunk, $mode;
>  	}
> -	if (@{$deletion->{TEXT}} && !@hunk) {
> +	if (@{$deletion->{TEXT}}) {
> +		foreach my $hunk (@hunk) {
> +			push @{$deletion->{TEXT}}, @{$hunk->{TEXT}};
> +			push @{$deletion->{DISPLAY}}, @{$hunk->{DISPLAY}};
> +		}
>  		@hunk = ($deletion);
>  	}
>  

Thanks for the quick patches.  This was similar to what I was working on, but
cleaner than what I had.  Works well for me.

-- 
James
GPG Key: 1024D/61326D40 2003-09-02 James Vega <vega.james@gmail.com>

Re: git-apply fails on creating a new file, with both -p and --directory specified

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Dec 07, 2009 at 07:20:30PM -0800, Junio C Hamano wrote:
>
>> An update.  I tried your reproduction recipe with 1.6.5.2 and it doesn't
>> reproduce, but with 1.6.5.3 it does.
>
> Thanks, both, for a very helpful bug report.  24ab81a was totally bogus,
> but we lacked a test for deleting a non-empty file. That test and a fix
> for the problem are in the patch below.

Thanks.

> I am still slightly concerned that James's
>
>   git diff | sed '/^deleted file/d' | git apply --cached
>
> behaves as it does. What should git-apply do with a patch like:
>
>   diff --git a/foo b/foo
>   index 257cc56..0000000
>   --- a/foo
>   +++ /dev/null
>   @@ -1 +0,0 @@
>   -foo
>
> ? I can see either turning it into a deletion patch (because /dev/null
> is special) or barfing (because /dev/null as a special case should have
> appeared in the "diff" line). But creating a dev/null file seems very
> wrong.

I was wondering about the same thing while bisecting.  By the current
definition of "diff --git", removing the "deleted file" or "new file" line
makes the patch an invalid "git format diff".  See the beginning of
parse_git_header() where we say "we don't guess" and initialize both
is_new and is_delete to false (and we flip them upon seeing "deleted file"
and "new file", but never with "/dev/null").

> But maybe it is not worth worrying about too much. That patch format is
> not generated intentionally by any known software.

I think some recent other SCMs produce what they claim to be "diff --git",
but I don't know if they implement the format correctly enough.  I am not
worried about their implemention of binary patches (if they do not
implement it correctly they will most likely get garbage), but do they get
the abbreviated hash on the "index" line correctly?  You can put garbage
on the line and most of the time it would work but it will break "am -3"
by breaking "apply --build-fake-ancestor".

I just checked "hg diff --git"; at least it shows "deleted file".

> That would take some refactoring, though, as pulling the deletion hunk
> out means we are re-ordering the headers. So right now if you did that
> your ($head, @hunk) output would be something like:
>
>        diff --git a/foo b/foo
>        index 257cc56..0000000
>        --- a/foo
>        +++ /dev/null
>        deleted file mode 100644
>        @@ -1 +0,0 @@
>        -foo
>
> which is pretty weird.

I agree it is weird.

> And it also opens the door to editing the hunk to stop the deletion, but
> still tweak the content change. Right now if you edit a deletion patch,
> you can't remove the 'deleted' bit, and if your edit result keeps any
> content in the file, apply will complain. I'm not sure that particular
> feature would be useful though (I have certainly never wanted it).

Interesting.  Does "add -p" (especially its [e]dit codepath) know enough
about what it is doing?  If so, it should be able to add "deleted file" on
its own (and remove it when the result of editing and picking hunks makes
the patch a non-deletion).  For example, if you have a two-liner in the
index and have deleted one line in the work tree, and run "add -p":

        diff --git a/foo b/foo
        index 3bd1f0e..257cc56 100644
        --- a/foo
        +++ b/foo
        @@ -1,2 +1 @@
         foo
        -bar

you *should* be able to edit it into a patch that removes all lines.

Perhaps the "add -i" at the end should offer, after noticing that the
chosen and edited hunks will make the postimage an empty file, a chance
for the user to say "I not only want to remove the contents from the path,
but want to remove the path itself" in such a case?

I dunno.

Re: git-apply fails on creating a new file, with both -p and --directory specified

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Dec 08, 2009 at 12:47:24AM -0500, Jeff King wrote:
>
>> There is a slightly different approach we could take, too: keep the
>> "deletion" hunk as a first-class hunk, and just meld the content hunk's
>> output into it. Then both cases would get the "Stage deletion" question
>> instead of the "Stage this hunk" you get now for non-empty files (which
>> just happens to trigger a deletion due to the headers).
>
> BTW, the code for this is the much smaller change below. If you prefer
> that, I can squash in the test and write up an appropriate commit
> message.

Doubly interesting, as I recall reading "That would take some refactoring,
though, as pulling the deletion hunk"

    ... goes and looks ...

Ah, Ok, the "refactoring" refers to the "header reordering weirdness".

That might be something we may want to fix someday, when we find ourselves
needing to add a feature to turn deletion into non-deletion or vice versa
during "add -p" [e]dit, as I suspect that the "hunk editing" codepath does
not keep track of what the user's patch is doing, to the point that it
does not even know how many lines there are supposed to be in the
resulting hunk that it asks "git apply" to recount.  There is no way to
add/delete "deleted file" line if the logic does not know what the patch
is doing.

But someday is not today.  I think this six-liner is preferable.

> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index 35f4ef1..02e97b9 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -1217,7 +1217,11 @@ sub patch_update_file {
>  	if (@{$mode->{TEXT}}) {
>  		unshift @hunk, $mode;
>  	}
> -	if (@{$deletion->{TEXT}} && !@hunk) {
> +	if (@{$deletion->{TEXT}}) {
> +		foreach my $hunk (@hunk) {
> +			push @{$deletion->{TEXT}}, @{$hunk->{TEXT}};
> +			push @{$deletion->{DISPLAY}}, @{$hunk->{DISPLAY}};
> +		}
>  		@hunk = ($deletion);
>  	}
>  

Re: git-apply fails on creating a new file, with both -p and --directory specified

[Jeff King]

On Mon, Dec 07, 2009 at 11:11:08PM -0800, Junio C Hamano wrote:

> I was wondering about the same thing while bisecting.  By the current
> definition of "diff --git", removing the "deleted file" or "new file" line
> makes the patch an invalid "git format diff".  See the beginning of
> parse_git_header() where we say "we don't guess" and initialize both
> is_new and is_delete to false (and we flip them upon seeing "deleted file"
> and "new file", but never with "/dev/null").

Hmm. In that case, I think converting it to deletion is definitely
wrong; "diff --git" is about not guessing. So the only question is
whether it should be flagged as an error. I was somewhat worried that
you could produce a patch which would make apply complain by doing "git
diff /dev/null /your/file". But actually that already produces a "new
file" header (and the opposite produces a "deleted file" header).

So I think the patch below would notice both the new and deleted cases,
and is probably a good thing.

diff --git a/builtin-apply.c b/builtin-apply.c
index c8372a0..43a1535 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -673,8 +673,12 @@ static int gitdiff_hdrend(const char *line, struct patch *patch)
  */
 static char *gitdiff_verify_name(const char *line, int isnull, char *orig_name, const char *oldnew)
 {
-	if (!orig_name && !isnull)
+	if (!orig_name && !isnull) {
+		if (!memcmp(line, "/dev/null\n", 10))
+			die("git apply: bad git-diff - expected filename, got /dev/null on line %d", linenr);
 		return find_name(line, NULL, p_value, TERM_TAB);
+	}
+
 
 	if (orig_name) {
 		int len;

> I think some recent other SCMs produce what they claim to be "diff --git",
> but I don't know if they implement the format correctly enough.  I am not
> worried about their implemention of binary patches (if they do not
> implement it correctly they will most likely get garbage), but do they get
> the abbreviated hash on the "index" line correctly?  You can put garbage
> on the line and most of the time it would work but it will break "am -3"
> by breaking "apply --build-fake-ancestor".
> 
> I just checked "hg diff --git"; at least it shows "deleted file".

Ugh. A whole new source of problems. :) I am not too interested in
seeking out and evaluating other SCM's implementations; I think we
should wait for people who actually use those systems to find
interoperability bugs, determine whether they are or are not simply bugs
in the other people's implementations, and then report the bug to us.

> > That would take some refactoring, though, as pulling the deletion hunk
> > out means we are re-ordering the headers. So right now if you did that
> > your ($head, @hunk) output would be something like:
> >
> >        diff --git a/foo b/foo
> >        index 257cc56..0000000
> >        --- a/foo
> >        +++ /dev/null
> >        deleted file mode 100644
> >        @@ -1 +0,0 @@
> >        -foo
> >
> > which is pretty weird.
> 
> I agree it is weird.

Note that we already do this for mode changes which also have a content
change. They look like:

  diff --git a/foo b/foo
  index 257cc56..19c6cc1
  --- a/foo
  +++ b/foo
  old mode 100644
  new mode 100755
  Stage mode change [y,n,q,a,d,/,j,J,g,?]?
  @@ -1 +1,2 @@
  foo
  +content
  Stage this hunk [y,n,q,a,d,/,K,g,e,?]?

> Interesting.  Does "add -p" (especially its [e]dit codepath) know enough
> about what it is doing?  If so, it should be able to add "deleted file" on
> its own (and remove it when the result of editing and picking hunks makes
> the patch a non-deletion).  For example, if you have a two-liner in the
> index and have deleted one line in the work tree, and run "add -p":

No, it doesn't know enough now. That would be part of the refactoring I
mentioned. I'm not sure how useful it is to support this. I can see
going from "I had hunk A, but I really wanted to tweak it to hunk B". I
can't think of a single time I've wanted "I deleted the entire file, but
I really wanted to keep 2 lines". And if I did, I would probably just:

  git checkout file
  $EDITOR file
  git add -p file

> Perhaps the "add -i" at the end should offer, after noticing that the
> chosen and edited hunks will make the postimage an empty file, a chance
> for the user to say "I not only want to remove the contents from the path,
> but want to remove the path itself" in such a case?
> 
> I dunno.

I would not say no to such a patch, but I really have no interest in
writing it myself.

-Peff

Re: git-apply fails on creating a new file, with both -p and --directory specified

[Jeff King]

On Mon, Dec 07, 2009 at 11:28:01PM -0800, Junio C Hamano wrote:

> That might be something we may want to fix someday, when we find ourselves
> needing to add a feature to turn deletion into non-deletion or vice versa
> during "add -p" [e]dit, as I suspect that the "hunk editing" codepath does
> not keep track of what the user's patch is doing, to the point that it
> does not even know how many lines there are supposed to be in the
> resulting hunk that it asks "git apply" to recount.  There is no way to
> add/delete "deleted file" line if the logic does not know what the patch
> is doing.
> 
> But someday is not today.  I think this six-liner is preferable.

OK, here it is with the test and an amended commit message. You could
almost do an [e]dit on this and delete the "deleted" line, but you have
no way of fixing up the "+++ /dev/null" line. For now, we have
disabled [e]dit entirely for non-content hunks, so at least you cannot
get yourself into trouble creating a broken patch. :)

-- >8 --
Subject: [PATCH] add-interactive: fix deletion of non-empty files

Commit 24ab81a fixed the deletion of empty files, but broke
deletion of non-empty files. The approach it took was to
factor out the "deleted" line from the patch header into its
own hunk, the same way we do for mode changes. However,
unlike mode changes, we only showed the special "delete this
file" hunk if there were no other hunks. Otherwise, the user
would annoyingly be presented with _two_ hunks: one for
deleting the file and one for deleting the content.

This meant that in the non-empty case, we forgot about the
deleted line entirely, and we submitted a bogus patch to
git-apply (with "/dev/null" as the destination file, but not
marked as a deletion).

Instead, this patch combines the file deletion hunk and the
content deletion hunk (if there is one) into a single
deletion hunk which is either staged or not.

Signed-off-by: Jeff King <peff@peff.net>
---
 git-add--interactive.perl  |    6 +++++-
 t/t3701-add-interactive.sh |   20 ++++++++++++++++++++
 2 files changed, 25 insertions(+), 1 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 35f4ef1..02e97b9 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1217,7 +1217,11 @@ sub patch_update_file {
 	if (@{$mode->{TEXT}}) {
 		unshift @hunk, $mode;
 	}
-	if (@{$deletion->{TEXT}} && !@hunk) {
+	if (@{$deletion->{TEXT}}) {
+		foreach my $hunk (@hunk) {
+			push @{$deletion->{TEXT}}, @{$hunk->{TEXT}};
+			push @{$deletion->{DISPLAY}}, @{$hunk->{DISPLAY}};
+		}
 		@hunk = ($deletion);
 	}
 
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index aa5909b..0926b91 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -215,6 +215,26 @@ test_expect_success 'add first line works' '
 '
 
 cat >expected <<EOF
+diff --git a/non-empty b/non-empty
+deleted file mode 100644
+index d95f3ad..0000000
+--- a/non-empty
++++ /dev/null
+@@ -1 +0,0 @@
+-content
+EOF
+test_expect_success 'deleting a non-empty file' '
+	git reset --hard &&
+	echo content >non-empty &&
+	git add non-empty &&
+	git commit -m non-empty &&
+	rm non-empty &&
+	echo y | git add -p non-empty &&
+	git diff --cached >diff &&
+	test_cmp expected diff
+'
+
+cat >expected <<EOF
 diff --git a/empty b/empty
 deleted file mode 100644
 index e69de29..0000000
-- 
1.6.5.1.g24ab.dirty

Re: git-apply fails on creating a new file, with both -p and --directory specified

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> OK, here it is with the test and an amended commit message. You could
> almost do an [e]dit on this and delete the "deleted" line, but you have
> no way of fixing up the "+++ /dev/null" line. For now, we have
> disabled [e]dit entirely for non-content hunks, so at least you cannot
> get yourself into trouble creating a broken patch. :)

;-)

Thanks.

git-apply fails on creating a new file, with both -p and --directory specified

[Matthias Lehmann]

Hi all,

the subject of this mail may read familiar to some - there was a discussion in 
November last year (see 
http://kerneltrap.org/mailarchive/git/2009/11/23/16899/) concerning this 
problem.

Today I had this same issue with git 1.7.0.4. Reading the above mentioned 
discussion and seaching the net did not help me in finding a solution to the 
problem. 

I have to apply patches from one repository to another repository, which have 
a different layout (I am working on splitting one big repository into several 
smaller ones, while development still continues on the big repository).

So I did

big-repo$ git format-patche -o /tmp/foo 
small-repo$ git apply -p2 --directory=some/path --check /tmp/foo/*

and get

fatal: git diff header lacks filename information when removing 2 leading 
pathname components (line 37)

the patch looks like this:

35| diff --git a/xyz/bar.jpg b/xyz/bar.jpg
36| new file mode 100644
37| index 
0000000000000000000000000000000000000000..3dcce2e1f68586ed2089d86b1bf4e7e41c818d97
38| GIT binary patch
39| literal 8791



Since this problem was discussed before - is there a solution?
This is something like a showstopper for me right now, so I am very thankful 
for any hints.

Best regards,

Mat

Re: git-apply fails on creating a new file, with both -p and --directory specified

[Matthias Lehmann]

Matthias Lehmann wrote:

> Hi all,
> 
> the subject of this mail may read familiar to some - there was a
> discussion in November last year (see
> http://kerneltrap.org/mailarchive/git/2009/11/23/16899/) concerning this
> problem.
> 
> Today I had this same issue with git 1.7.0.4. Reading the above mentioned
> discussion and seaching the net did not help me in finding a solution to
> the problem.
> 
> I have to apply patches from one repository to another repository, which
> have a different layout (I am working on splitting one big repository into
> several smaller ones, while development still continues on the big
> repository).
> 
> So I did
> 
> big-repo$ git format-patche -o /tmp/foo
> small-repo$ git apply -p2 --directory=some/path --check /tmp/foo/*
> 
> and get
> 
> fatal: git diff header lacks filename information when removing 2 leading
> pathname components (line 37)
> 
> the patch looks like this:
> 
> 35| diff --git a/xyz/bar.jpg b/xyz/bar.jpg
> 36| new file mode 100644
> 37| index
> 
0000000000000000000000000000000000000000..3dcce2e1f68586ed2089d86b1bf4e7e41c818d97
> 38| GIT binary patch
> 39| literal 8791
> 
> 
> 
> Since this problem was discussed before - is there a solution?
> This is something like a showstopper for me right now, so I am very
> thankful for any hints.
> 
> Best regards,
> 
> Mat

Is there anything I can do to get some feedback to this issue? I don't want 
to be impatient, it's just quite important to me. 

Since I have not much experience in reporting issues to a mailing list, it 
might well be, that I did something wrong - so please correct me and direct 
me to how I can be of better help.

Best regards,

Mat