Re: [PATCH] builtin-apply: check for empty files when detecting creation patch

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(-)