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

From: Junio C Hamano <junkio@cox.net>
Date: Jul 17, 2006, 06:21 UTC
Message-ID: <7vwtaciwek.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <1153046320538-git-send-email-normalperson@yhbt.net>

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.

Show 5 quoted lines

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

Show 6 quoted lines

> +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 (;;) {