Re: git-apply fails on creating a new file, with both -p and --directory specified
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Dec 8, 2009, 07:11 UTC
- Message-ID
- <7vvdgindo3.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <20091208054724.GA21347@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 8 quoted lines
> 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.
Show 17 quoted lines
> 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".
Show 13 quoted lines
> 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.
Show 5 quoted lines
> 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
-baryou *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.