bug in read-tree -m on A -> A/A

bug in read-tree -m on A -> A/A

[Shawn O. Pearce]

dancor on #git just noticed a bug in read-tree -m that prevents
him from switching branches when the type of a path changes
between a directory and a file.  The following test appears to
trigger the same failure, but I likely won't have time to fix
it myself tonight.  So if someone else gets a chance...

diff --git a/t/t9999-typechange.sh b/t/t9999-typechange.sh
new file mode 100755
index 0000000..dc92094
--- /dev/null
+++ b/t/t9999-typechange.sh
@@ -0,0 +1,25 @@
+#!/bin/sh
+
+test_description='switch branches over path typechange'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+	: >A &&
+	git-add A &&
+	tree1=$(git-write-tree) &&
+	comm1=$(echo A | git-commit-tree $tree1) &&
+	git-update-ref refs/heads/comm1 $comm1 &&
+	rm .git/index &&
+	rm -f A &&
+	mkdir A &&
+	echo : >A/A &&
+	git-add A/A &&
+	tree2=$(git-write-tree) &&
+	comm2=$(echo A/A | git-commit-tree $tree2) &&
+	git-update-ref HEAD $comm2
+'
+
+test_expect_success checkout 'git-checkout $comm1'
+
+test_done

-- 
Shawn.

Re: bug in read-tree -m on A -> A/A

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> ....  The following test appears to
> trigger the same failure,...

You have file "A" on one branch, and file "A/A" on another
branch.  You are on the latter branch and switching to the
former one.

The following patch illustrates where you need to implement an
alternate, loosened check, but should not be applied to your
tree as-is.  If you have local modification to path "A/A", this
will lose it.

-- >8 --
diff --git a/unpack-trees.c b/unpack-trees.c
index 2e2232c..345b2ee 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -479,9 +479,27 @@ static void verify_absent(const char *path, const char *action,
 
 	if (o->index_only || o->reset || !o->update)
 		return;
-	if (!lstat(path, &st) && !(o->dir && excluded(o->dir, path)))
+
+	if (!lstat(path, &st)) {
+		if (o->dir && excluded(o->dir, path))
+			/*
+			 * path is explicitly excluded, so it is Ok to
+			 * overwrite it.
+			 */
+			return;
+		if (S_ISDIR(st.st_mode))
+			/*
+			 * We are checking out path "foo" and
+			 * found "foo/." in the working tree.
+			 * This is tricky -- if we have modified
+			 * files that are in "foo/" we would lose
+			 * it if we just uncoditinally return here.
+			 */
+			return;
+
 		die("Untracked working tree file '%s' "
 		    "would be %s by merge.", path, action);
+	}
 }
 
 static int merged_entry(struct cache_entry *merge, struct cache_entry *old,


-- 8< --

To solve this sanely I think you would need to merge bottom-up,
which is quite a large change to read-tree.  Currently the code
asks "is it Ok to extract A from the new tree?" and if we say
yes here it would remove all existing cache entries "A/*" and
replaces it with "A".  After that happens, you would not even
have a chance to see if A/A has local modifications, or if you
have a local file that is not even known to git, say A/C, that
will also be lost by this tree switching.

Re: bug in read-tree -m on A -> A/A

[Junio C Hamano]

Junio C Hamano <junkio@cox.net> writes:

> "Shawn O. Pearce" <spearce@spearce.org> writes:
>
>> ....  The following test appears to
>> trigger the same failure,...
>
> You have file "A" on one branch, and file "A/A" on another
> branch.  You are on the latter branch and switching to the
> former one.
>
> The following patch illustrates where you need to implement an
> alternate, loosened check, but should not be applied to your
> tree as-is.  If you have local modification to path "A/A", this
> will lose it.

This does not do the bottom-up merge, but tries to catch the
lossy case within the limit of the current framework.

Only lightly tested, and I won't be applying it as-is yet as I
am not thinking very clearly tonight (no, I am not drunk, just
under the weather a bit).

Testing and improvements are very much appreciated.

---
 unpack-trees.c |   75 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 74 insertions(+), 1 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index 2e2232c..2288762 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -468,6 +468,60 @@ static void invalidate_ce_path(struct cache_entry *ce)
 		cache_tree_invalidate_path(active_cache_tree, ce->name);
 }
 
+static void verify_clean_subdirectory(const char *path, const char *action,
+				      struct unpack_trees_options *o)
+{
+	/*
+	 * we are about to extract "path"; we would not want to lose
+	 * anything in the existing directory there.
+	 */
+	int namelen;
+	int pos, i;
+	struct dir_struct d;
+	char *pathbuf;
+
+	/*
+	 * First let's make sure we do not have a local modification
+	 * in that directory.
+	 */
+	namelen = strlen(path);
+	pos = cache_name_pos(path, namelen);
+	if (0 <= pos)
+		return; /* we have it as nondirectory */
+	pos = -pos - 1;
+	for (i = pos; i < active_nr; i++) {
+		struct cache_entry *ce = active_cache[i];
+		int len = ce_namelen(ce);
+		if (len < namelen ||
+		    strncmp(path, ce->name, namelen) ||
+		    ce->name[namelen] != '/')
+			break;
+		/*
+		 * ce->name is an entry in the subdirectory.
+		 */
+		verify_uptodate(ce, o);
+	}
+
+	/*
+	 * Then we need to make sure that we do not lose a locally
+	 * present file that is not ignored.
+	 */
+	if (!o->dir)
+		return;
+
+	pathbuf = xmalloc(namelen + 2);
+	memcpy(pathbuf, path, namelen);
+	strcpy(pathbuf+namelen, "/");
+
+	memset(&d, 0, sizeof(d));
+	d.exclude_per_dir = o->dir->exclude_per_dir;
+	i = read_directory(&d, path, pathbuf, namelen+1);
+	if (i)
+		die("Updating '%s' would lose untracked files in it",
+		    path);
+	free(pathbuf);
+}
+
 /*
  * We do not want to remove or overwrite a working tree file that
  * is not tracked, unless it is ignored.
@@ -479,9 +533,28 @@ static void verify_absent(const char *path, const char *action,
 
 	if (o->index_only || o->reset || !o->update)
 		return;
-	if (!lstat(path, &st) && !(o->dir && excluded(o->dir, path)))
+
+	if (!lstat(path, &st)) {
+		if (o->dir && excluded(o->dir, path))
+			/*
+			 * path is explicitly excluded, so it is Ok to
+			 * overwrite it.
+			 */
+			return;
+		if (S_ISDIR(st.st_mode))
+			/*
+			 * We are checking out path "foo" and
+			 * found "foo/." in the working tree.
+			 * This is tricky -- if we have modified
+			 * files that are in "foo/" we would lose
+			 * it.
+			 */
+			verify_clean_subdirectory(path, action, o);
+			return;
+
 		die("Untracked working tree file '%s' "
 		    "would be %s by merge.", path, action);
+	}
 }
 
 static int merged_entry(struct cache_entry *merge, struct cache_entry *old,