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

From: Junio C Hamano <gitster@pobox.com>
Date: May 13, 2008, 21:48 UTC
Message-ID: <7vprrpswof.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <500f3d130805131316m59898392l46e0dbf7cb352981@mail.gmail.com>

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

Show 31 quoted lines

> On Sun, May 11, 2008 at 5:36 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Imre Deak <imre.deak@gmail.com> 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;