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

"Imre Deak" [off-list ref] writes:

On Sun, May 11, 2008 at 5:36 AM, Junio C Hamano [off-list ref] wrote:

quoted

Imre Deak [off-list ref] writes:

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

 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.

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.

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

diff --git a/builtin-apply.c b/builtin-apply.c
index 1103625..9d7cb05 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

@@ -451,6 +451,8 @@ static void parse_traditional_patch(const char *first, const char *second, struc
 		name = find_name(first, NULL, p_value, TERM_SPACE | TERM_TAB);
 		patch->old_name = name;
 	} else {
+		patch->is_new = 0;
+		patch->is_delete = 0;
 		name = find_name(first, NULL, p_value, TERM_SPACE | TERM_TAB);
 		name = find_name(second, name, p_value, TERM_SPACE | TERM_TAB);
 		patch->old_name = patch->new_name = name;

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

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

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