patchbuiltin-apply: check for empty files when detecting creation patch

When we can only guess if we have a creation patch, we do this by treating the patch as such if there weren't any old lines. Zero length files can be patched without old lines though, so do an extra check for file size.

Signed-off-by: Imre Deak <imre.deak@gmail.com>
---
 builtin-apply.c |   34 +++++++++++++++++++++++++++++++++-
 1 files changed, 33 insertions(+), 1 deletions(-)

Imre Deak <imre.deak@gmail.com> writes:

You described what your patch does, but you did not explain why it is a good addition. One way to do so is to illustrate in what occasion what the existing code does is insufficient.

ntohl()?  I thought ce->ce_* are host-native byte order these days...

Doesn't this break the use case where "git-apply --stat" is used as an improved diffstat outside a git repository?

The user did not say the patch was produced without context, or we do have context. The latter cannot be a creation patch so the new logic is not appropriate. But let's forget that problem for now and look at the case where the patch did _not_ have any context, i.e. only added and deleted lines.

If the patch did not have context, and the user did not ask for -u0 patch when it was produced, it could be a creation patch, but if there are deleted lines it cannot be. That is the original logic.

After your patch, the original logic is allowed to decide that the patch is a creation _only if_ you happen to already have a file that is _to be created_ in the work tree with some existing contents, or the file does not exist. I do not see a sane logic behind that. If you were making sure that the work tree does _not_ have the file, then I would understand, even though I think it is wrong for "apply --stat" case. If you see a file in the work tree, and if you assume the patch would apply to the work tree, then the patch cannot be creation!

In general, it is not right to look at the work tree to decide how to interpret what the patch means to begin with, but maybe you are trying to use work tree status as a hint to disambiguate a corner case that the information in a patch we are reading is insufficient, in which case it might be Ok. But I cannot tell what that corner case is.

I am lost. Please explain what you are trying to fix first before explaining how you attempted to fix it.

Hi,

thanks for your answer.

On Sun, May 11, 2008 at 5:36 AM, Junio C Hamano <gitster@pobox.com> wrote:

The patch makes it possible to apply foreign patches (not created with git diff) to zero length files already existing in the index. The problem:

$ git init
Initialized empty Git repository in .git/
$ rm -rf a
$ touch a
$ git add a
$ git commit -madd
Created initial commit 818f2b7: add
 0 files changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 a
$ echo 123 > a.new
$ diff -u a a.new > patch
$ git apply patch
error: a: already exists in working directory

The error happens because git guesses that `patch` is a creation patch and since `a` already exists in the index it will bail out. With the modification foreign patches won't be guessed to be a creation patch if the file to be patch already exists in the index or the working directory.

Sorry, I actually created the patch against an older version. It should be without ntohl().

In that case we'll return error (file doesn't exist) which will leave the possibility open to guess the patch to be a creation patch. With the change when doing --stat the patch might or might not be changed to a creation patch based on the current working directory content. Though I think this doesn't result in any difference in the --stat output one could argue that --stat's operation shouldn't depend in any way on the working directory content except when --check is specified in addition. So probably we should special case --stat here...

So as far as I understand creation or deletion patches cannot have any context lines. If that's correct shouldn't the if clause

if (!unidiff_zero || context)

read instead

if (!unidiff_zero)

?

You are right, that condition is wrong. The right one then should be:

existing_file_size(patch->old_name) < 0

Hm, I agree with that if it's only a --stat, but if it's --check or --apply I think there is no other way to guess if we really have a creation patch or only an add-to-a-zero-length-file patch.

--Imre

"Imre Deak" <imre.deak@gmail.com> writes:

Ok, that is much clearer. How about this two-liner instead, then (the first hunk is just an unreleated typofix)?

Originally when Linus wrote "git-apply" he was trying to be very cautious and used the /dev/null cue only in a positive way (i.e. if the patch is from /dev/null it is a creation). But the preimage being something other than /dev/null was not used as a statement that the patch is not a creation.

Non SCM patches of any nontrivial size would be created by comparing two trees with "diff -ru" (with some more combination of options). We would reliably use /dev/null cue in this case --- if the patch is from /dev/null it is creation but more importantly if it is not from /dev/null it is not creation but modification.

Patches from foreign SCMs also follow the /dev/null convention (e.g. SVN and CVS --- I did not check Hg but I would be surprised if it didn't follow suit), and we can reliably use the lack of /dev/null as a cue that it is not a creation patch.

A single-file non SCM patches are done by comparing $a.orig and $a, $a~ and $a, etc., i.e. a pair of files the original and the modified with some file suffixes. What would people do to represent a creation in such a case? --- You are right. By doing "diff -u /dev/null $a" (it is more cumbersome to do "touch $a.empty; diff -u $a.empty $a").

So I think it is reasonable to use non-/dev/null-ness of first as a cue that it is not a creation patch.

Linus, what do you think? FYR, the original patch that led to this conversation was:

    http://thread.gmane.org/gmane.comp.version-control.git/81531

---

 builtin-apply.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

On Tue, 13 May 2008, Junio C Hamano wrote:

I disagree.

The fact is, /dev/null means that for patches generated by GNU diff, but a lot of other systems you'll find that it means no such thing.

Look at CVS-generated patches, or SVN for that matter. The diffs look like this:

	Index: file
	===================================================================
	--- file (revision 0)
	+++ file (working copy)
	@@ -0,0 +1 @@
	+test

and there is no /dev/null there.

The thing is, git-apply is careful, and it's very much careful with respect to *knowing* that there are lots of different versions of "diff" floating around, and lots of different SCM systems that generate odd diff headers. We should absolutely NOT start expecting that diffs are only generated with GNU diff.

So non-/dev/null'ness means absolutely nothing. It means "don't know", and we should leave is_new and is_delete as -1.

		Linus

Linus Torvalds <torvalds@linux-foundation.org> writes:

Ok, then what's the judgement for the original issue? Is it a user error to have a tracked absolutely empty file in the index?

On Tue, 13 May 2008, Junio C Hamano wrote:

I think this is the fundamental problem:

	..
	if (patch->is_new < 0 && !oldlines) {
		patch->is_new = 1;
	..

because that logic simply isn't right. (is_new < 0 && !oldlines) does *not* mean that it must be new.

We can say it the other way around, of course:

	if (patch->is_new < 0 && oldlines)
		patch->is_new = 0;

and that's a valid rule, but I think we already would never set "is_new" to -1 if we had old lines, so that would probably be a pointless thing to do.

So: remove the check for (is_new < 0 && !oldlines) because it doesn't 
actually add any information, and leave "is_new" as unknown until later 
when we actually *see* that file or not. Hmm?

		Linus

Linus Torvalds <torvalds@linux-foundation.org> writes:

Yeah, in fact we have this:

	if (patch->is_new < 0 &&
	    (oldlines || (patch->fragments && patch->fragments->next)))
		patch->is_new = 0;

a several lines above the piece you quoted in parse_single_patch().

That leads to the similar logic to remove "If we do not know if it is delete or not, not adding any lines (or any replacement lines) means delete" logic as well, which in turn means the whole if (!unidiff_zero...) block would go.

Which actually is a good thing, I think. As Imre mentioned, I do not think

	if (!unidiff_zero || context)

makes sense.  I cannot guess what the intention of that was.

    commit 4be609625e48e908f2b76d35bfeb61a8ba3a83a0
    Author: Junio C Hamano <junkio@cox.net>
    Date:   Sun Sep 17 01:04:24 2006 -0700

        apply --unidiff-zero: loosen sanity checks for --unidiff=0 patches

        In "git-apply", we have a few sanity checks and heuristics that
        expects that the patch fed to us is a unified diff with at least
        one line of context.

         ...description of good tests)...

        These sanity checks are good safety measures, but breaks down
        when people feed a diff generated with --unified=0.  This was
        recently noticed first by Matthew Wilcox and Gerrit Pape.

        This adds a new flag, --unified-zero, to allow bypassing these
        checks.  If you are in control of the patch generation process,
        you should not use --unified=0 patch and fix it up with this
        flag; rather you should try work with a patch with context.  But
        if all you have to work with is a patch without context, this
        flag may come handy as the last resort.

        Signed-off-by: Junio C Hamano <junkio@cox.net>

So unless unidiff-zero is given, we would want extra checks inside that block. Also if we have context in the patch anywhere, that means the patch cannot be coming from "diff -u0", so we go into that block.

Even though other checks that are guarded with "if (!unidiff_zero)" elsewhere in the code do make sense, however, this block may not do anything useful, as you pointed out.

With the change to remove the whole block, all tests still passes, and a limited test with this:

        --- empty	2008-05-13 16:56:57.000000000 -0700
        +++ empty.1	2008-05-13 16:57:07.000000000 -0700
        @@ -0,0 +1 @@
        +foo

to update an originally empty file "empty" also seems to work.

However, with this change, it no longer allows you to accept such a patch and treat it as a creation of "empty". Instead we barf with "error: empty: No such file or directory", if you do not have an empty "empty" file in the work tree when you run "git apply" on the above patch.

When "diff" was run with flags that could produce context (that is what we get from !unidiff_zero), if we do not see any lines that begin with '-', the only case that it is not a creation patch is if you compared the postimage with a preimage of size 0. Because we do not have usable /dev/null cue, we cannot tell that case from a creation patch.

I am having a strong suspicion that doing this change is robbing Peter to pay Imre. We simply cannot have it both ways, methinks.

---

 builtin-apply.c |   17 +----------------
 1 files changed, 1 insertions(+), 16 deletions(-)

On Tue, 13 May 2008, Junio C Hamano wrote:

Ok, that's a bug. It should *not* require that existing empty file, since "is_new" is -1. That's what -1 means: we don't know if it is new or not.

So I think your patch is correct, but we need to fix the thing that barfs to not barf if we don't know the status of "is_new"

		Linus

Linus Torvalds <torvalds@linux-foundation.org> writes:

Sorry for taking some time to follow this through (I've been busy with day job). Two patches follow this message to address this issue.

A patch from a foreign SCM (or plain "diff" output) often have both preimage and postimage filename on ---/+++ lines even for a patch that creates a new file. However, when there is a filename for preimage, we used to insist the file to exist (either in the work tree and/or in the index). When we cannot be sure by parsing the patch that it is not a creation patch, we shouldn't complain when if there is no such a file. This commit fixes the logic.

Refactor the code that validates the preimage file into a separate function while we are at it, as it is getting rather big.

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

When we see no context nor deleted line in the patch, we used to declare that the patch creates a new file. But some people create an empty file and then apply a patch to it. Similarly, a patch that delete everything is not a deletion patch either.

This commit corrects these two issues. Together with the previous commit, it allows a diff between an empty file and a line-ful file to be treated as both creation patch and "add stuff to an existing empty file", depending on the context. A new test t4126 demonstrates the fix.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-apply.c        |   15 ------------
 t/t4126-apply-empty.sh |   60 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+), 15 deletions(-)
 create mode 100755 t/t4126-apply-empty.sh