Jeff King <peff@xxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html