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

Linus Torvalds [off-list ref] writes:

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.

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

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?

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 [off-list ref]
    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 [off-list ref]

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

diff --git a/builtin-apply.c b/builtin-apply.c
index 1103625..395f16b 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c

@@ -418,7 +418,7 @@ static int guess_p_value(const char *nameline)
 }
 
 /*
- * Get the name etc info from the --/+++ lines of a traditional patch header
+ * Get the name etc info from the ---/+++ lines of a traditional patch header
  *
  * FIXME! The end-of-filename heuristics are kind of screwy. For existing
  * files, we can happily check the index for a match, but for creating a

@@ -1143,21 +1143,6 @@ static int parse_single_patch(char *line, unsigned long size, struct patch *patc
 	if (patch->is_delete < 0 &&
 	    (newlines || (patch->fragments && patch->fragments->next)))
 		patch->is_delete = 0;
-	if (!unidiff_zero || context) {
-		/* If the user says the patch is not generated with
-		 * --unified=0, or if we have seen context lines,
-		 * then not having oldlines means the patch is creation,
-		 * and not having newlines means the patch is deletion.
-		 */
-		if (patch->is_new < 0 && !oldlines) {
-			patch->is_new = 1;
-			patch->old_name = NULL;
-		}
-		if (patch->is_delete < 0 && !newlines) {
-			patch->is_delete = 1;
-			patch->new_name = NULL;
-		}
-	}
 
 	if (0 < patch->is_new && oldlines)
 		die("new file %s depends on old contents", patch->new_name);