[RFH/RFC] typechange tests for git apply (currently failing)

[RFH/RFC] typechange tests for git apply (currently failing)

[Eric Wong]

I've found that git apply is incapable of handling patches
involving object type changes to the same path.

Of course git itself is perfectly capable of making commits that
generate these changes, as it only tracks trees states.  It's
just that the diffs between them are less useful if they can't
be applied.

Some of these are rare, but I've hit one of them (file becoming
a symlink) recently in real-world usage, and was inspired to
find more potential breakages :)

I'm not sure when I'll have time to fix these myself and I'm not
very familiar with the apply code.   So if someone could get
some or all of these cases working, they would be my hero :)

Some of these are what I would refer to as corner-cases from
hell.  Most (if not all) other systems fail some of these.  In
fact, they aren't even capable of representing most of these
changes in their histories; much less being able to handle
patches to that effect.

Signed-off-by: Eric Wong <normalperson@yhbt.net>
---
 t/t4114-apply-typechange.sh |  105 +++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 105 insertions(+), 0 deletions(-)

diff --git a/t/t4114-apply-typechange.sh b/t/t4114-apply-typechange.sh
new file mode 100755
index 0000000..ca81d72
--- /dev/null
+++ b/t/t4114-apply-typechange.sh
@@ -0,0 +1,105 @@
+#!/bin/sh
+#
+# Copyright (c) 2006 Eric Wong
+#
+
+test_description='git-apply should not get confused with type changes.
+
+'
+
+. ./test-lib.sh
+
+test_expect_success 'setup repository and commits' '
+	echo "hello world" > foo &&
+	echo "hi planet" > bar &&
+	git update-index --add foo bar &&
+	git commit -m initial &&
+	git branch initial &&
+	rm -f foo &&
+	ln -s bar foo &&
+	git update-index foo &&
+	git commit -m "foo symlinked to bar" &&
+	git branch foo-symlinked-to-bar &&
+	rm -f foo &&
+	echo "how far is the sun?" > foo &&
+	git update-index foo &&
+	git commit -m "foo back to file" &&
+	git branch foo-back-to-file &&
+	rm -f foo &&
+	git update-index --remove foo &&
+	mkdir foo &&
+	echo "if only I knew" > foo/baz &&
+	git update-index --add foo/baz &&
+	git commit -m "foo becomes a directory" &&
+	git branch "foo-becomes-a-directory" &&
+	echo "hello world" > foo/baz &&
+	git update-index foo/baz &&
+	git commit -m "foo/baz is the original foo" &&
+	git branch foo-baz-renamed-from-foo
+	'
+
+test_expect_success 'file renamed from foo to foo/baz' '
+	git checkout -f initial &&
+	git diff-tree -M -p HEAD foo-baz-renamed-from-foo > patch &&
+	git apply --index < patch
+	'
+test_debug 'cat patch'
+
+
+test_expect_success 'file renamed from foo/baz to foo' '
+	git checkout -f foo-baz-renamed-from-foo &&
+	git diff-tree -M -p HEAD initial > patch &&
+	git apply --index < patch
+	'
+test_debug 'cat patch'
+
+
+test_expect_success 'directory becomes file' '
+	git checkout -f foo-becomes-a-directory &&
+	git diff-tree -p HEAD initial > patch &&
+	git apply --index < patch
+	'
+test_debug 'cat patch'
+
+
+test_expect_success 'file becomes directory' '
+	git checkout -f initial &&
+	git diff-tree -p HEAD foo-becomes-a-directory > patch &&
+	git apply --index < patch
+	'
+test_debug 'cat patch'
+
+
+test_expect_success 'file becomes symlink' '
+	git checkout -f initial &&
+	git diff-tree -p HEAD foo-symlinked-to-bar > patch &&
+	git apply --index < patch
+	'
+test_debug 'cat patch'
+
+
+test_expect_success 'symlink becomes file' '
+	git checkout -f foo-symlinked-to-bar &&
+	git diff-tree -p HEAD foo-back-to-file > patch &&
+	git apply --index < patch
+	'
+test_debug 'cat patch'
+
+
+test_expect_success 'symlink becomes directory' '
+	git checkout -f foo-symlinked-to-bar &&
+	git diff-tree -p HEAD foo-becomes-a-directory > patch &&
+	git apply --index < patch
+	'
+test_debug 'cat patch'
+
+
+test_expect_success 'directory becomes symlink' '
+	git checkout -f foo-becomes-a-directory &&
+	git diff-tree -p HEAD foo-symlinked-to-bar > patch &&
+	git apply --index < patch
+	'
+test_debug 'cat patch'
+
+
+test_done
-- 
1.4.1.g9d8f

Re: [RFH/RFC] typechange tests for git apply (currently failing)

[Junio C Hamano]

Eric Wong <normalperson@yhbt.net> writes:

> I've found that git apply is incapable of handling patches
> involving object type changes to the same path.

It's more of directory vs file conflicts -- and we do not track
directories.  Some are pure bugs and relatively simple to fix
(and important), some others I am not sure if they are worth
dealing with.

> +test_expect_success 'file renamed from foo to foo/baz' '
> +	git checkout -f initial &&
> +	git diff-tree -M -p HEAD foo-baz-renamed-from-foo > patch &&
> +	git apply --index < patch
> +	'

If you look at where it fails closely you would notice this
first fails during git-checkout (without failing, I should
add).  Try adding "git diff" immediately after "git checkout".

A fix for read-tree is in this message to fix this, but this has
only been very lightly tested, so please check it thoroughly.

After that is cleared, this and the next one uncover a few bugs
in "git apply".

> +test_expect_success 'file renamed from foo/baz to foo' '
> +	git checkout -f foo-baz-renamed-from-foo &&
> +	git diff-tree -M -p HEAD initial > patch &&
> +	git apply --index < patch
> +	'
> +test_debug 'cat patch'

one-way merge used in "git checkout -f" does not remove existing
directory when checking out a file.  "git reset --hard" used to
be more careful but recent rewrite made them more or less
equivalent, and now has the same problem.  This patch to read-tree
should fix it.

diff --git a/builtin-read-tree.c b/builtin-read-tree.c
index 6df5d7c..122b6f1 100644
--- a/builtin-read-tree.c
+++ b/builtin-read-tree.c
@@ -507,7 +507,7 @@ static int merged_entry(struct cache_ent
 	}
 
 	merge->ce_flags &= ~htons(CE_STAGEMASK);
-	add_cache_entry(merge, ADD_CACHE_OK_TO_ADD);
+	add_cache_entry(merge, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE);
 	return 1;
 }
 
@@ -518,7 +518,7 @@ static int deleted_entry(struct cache_en
 	else
 		verify_absent(ce->name, "removed");
 	ce->ce_mode = 0;
-	add_cache_entry(ce, ADD_CACHE_OK_TO_ADD);
+	add_cache_entry(ce, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE);
 	invalidate_ce_path(ce);
 	return 1;
 }


Then apply, when applying to create a file where a directory
lies, or vice versa, was not careful and did not remove
conflicting one.  This patch makes the first few tests to work,
but it is not enough.

Currently, builtin-apply.c::write_out_one_result() says "remove
the old, write the new" for each "struct patch" (which
corresponds to "diff --git" line in your patch file).  I think
the loop write_out_results() should be modified to first remove
what we are going to remove in all patches, and then create what
we are going to create.

What causes the fourth test to fail is that you have foo/baz in
the working tree and the index, and the patch creates file foo
and removes file foo/baz.  The current loop to deal with one
patch at a time means we try to create file "foo" first, which
would not work without removing directory "foo" first.

diff --git a/builtin-apply.c b/builtin-apply.c
index c903146..9727442 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -1732,9 +1732,14 @@ static int check_patch(struct patch *pat
 		if (check_index && cache_name_pos(new_name, strlen(new_name)) >= 0)
 			return error("%s: already exists in index", new_name);
 		if (!cached) {
-			if (!lstat(new_name, &st))
-				return error("%s: already exists in working directory", new_name);
-			if (errno != ENOENT)
+			struct stat nst;
+			if (!lstat(new_name, &nst)) {
+				if (S_ISDIR(nst.st_mode))
+					; /* ok */
+				else
+					return error("%s: already exists in working directory", new_name);
+			}
+			else if ((errno != ENOENT) && (errno != ENOTDIR))
 				return error("%s: %s", new_name, strerror(errno));
 		}
 		if (!patch->new_mode) {
@@ -2011,6 +2016,16 @@ static void create_one_file(char *path, 
 	}
 
 	if (errno == EEXIST) {
+		/* We may be trying to create a file where a directory
+		 * used to be.
+		 */
+		struct stat st;
+		errno = 0;
+		if (!lstat(path, &st) && S_ISDIR(st.st_mode) && !rmdir(path))
+			errno = EEXIST;
+	}
+
+	if (errno == EEXIST) {
 		unsigned int nr = getpid();
 
 		for (;;) {