[PATCH] reset --hard/read-tree --reset -u: remove unmerged new paths

When aborting a failed merge that has brought in a new path using "git
reset --hard" or "git read-tree --reset -u", we used to first forget about
the new path (via read_cache_unmerged) and then matched the working tree
to what is recorded in the index, thus ending up leaving the new path in
the work tree.

Signed-off-by: Junio C Hamano <redacted>
---
 Junio C Hamano [off-list ref] writes:

 > Linus Torvalds [off-list ref] writes:
 >
 >> On Wed, 15 Oct 2008, Linus Torvalds wrote:
 >>> 
 >> It's quite possible that we should remove unmerged entries. Except that's 
 >> not how our internal 'read_cache_unmerged()' function works. It really 
 >> just ignores them, and throws them on the floor. We _could_ try to just 
 >> turn them into a (since) stage-0 entry.
 >>
 >> Junio?
 >
 > I am not sure what should happen when we can't drop the unmerged entry
 > down to stage-0 due to D/F conflicts, though.  IIRC, read-tree proper
 > would not touch the work tree in such a case, but merge-recursive creates
 > our and their versions with funny suffixes, which will not be known to the
 > index and will be left in the working tree.

 I am still unsure what we should do when we hit D/F conflicts; this one
 simply replaces but it may be safer to drop ADD_CACHE_OK_TO_REPLACE from
 the options to trigger an error in such a case.  I dunno.

 read-cache.c               |   32 +++++++++++++++++++-------------
 t/t1005-read-tree-reset.sh |   30 ++++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+), 13 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index c229fd4..efbab6a 100644
--- a/read-cache.c
+++ b/read-cache.c

@@ -1489,25 +1489,31 @@ int write_index(const struct index_state *istate, int newfd)
 int read_index_unmerged(struct index_state *istate)
 {
 	int i;
-	struct cache_entry **dst;
-	struct cache_entry *last = NULL;
+	int unmerged = 0;
 
 	read_index(istate);
-	dst = istate->cache;
 	for (i = 0; i < istate->cache_nr; i++) {
 		struct cache_entry *ce = istate->cache[i];
-		if (ce_stage(ce)) {
-			remove_name_hash(ce);
-			if (last && !strcmp(ce->name, last->name))
-				continue;
-			cache_tree_invalidate_path(istate->cache_tree, ce->name);
-			last = ce;
+		struct cache_entry *new_ce;
+		int size, len, option;
+
+		if (!ce_stage(ce))
 			continue;
-		}
-		*dst++ = ce;
+		unmerged = 1;
+		len = strlen(ce->name);
+		size = cache_entry_size(len);
+		new_ce = xcalloc(1, size);
+		hashcpy(new_ce->sha1, ce->sha1);
+		memcpy(new_ce->name, ce->name, len);
+		new_ce->ce_flags = create_ce_flags(len, 0);
+		new_ce->ce_mode = ce->ce_mode;
+		option = ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE;
+		if (add_index_entry(istate, new_ce, option))
+			return error("%s: cannot drop to stage #0",
+				     ce->name);
+		i = index_name_pos(istate, new_ce->name, len);
 	}
-	istate->cache_nr = dst - istate->cache;
-	return !!last;
+	return unmerged;
 }
 
 struct update_callback_data

diff --git a/t/t1005-read-tree-reset.sh b/t/t1005-read-tree-reset.sh
index b0d31f5..0cd519c 100755
--- a/t/t1005-read-tree-reset.sh
+++ b/t/t1005-read-tree-reset.sh

@@ -27,4 +27,34 @@ test_expect_success 'reset should work' '
   test_cmp expect actual
 '
 
+test_expect_success 'reset should remove remnants from a failed merge' '
+  git read-tree --reset -u HEAD &&
+  git ls-files -s >expect &&
+  sha1=$(git rev-parse :new) &&
+  (
+	echo "100644 $sha1 1	old"
+	echo "100644 $sha1 3	old"
+  ) | git update-index --index-info &&
+  >old &&
+  git ls-files -s &&
+  git read-tree --reset -u HEAD &&
+  git ls-files -s >actual &&
+  ! test -f old
+'
+
+test_expect_success 'Porcelain reset should remove remnants too' '
+  git read-tree --reset -u HEAD &&
+  git ls-files -s >expect &&
+  sha1=$(git rev-parse :new) &&
+  (
+	echo "100644 $sha1 1	old"
+	echo "100644 $sha1 3	old"
+  ) | git update-index --index-info &&
+  >old &&
+  git ls-files -s &&
+  git reset --hard &&
+  git ls-files -s >actual &&
+  ! test -f old
+'
+
 test_done

-- 
1.6.0.2.717.gc6f0a

On Wed, 15 Oct 2008, Junio C Hamano wrote:

When aborting a failed merge that has brought in a new path using "git
reset --hard" or "git read-tree --reset -u", we used to first forget about
the new path (via read_cache_unmerged) and then matched the working tree
to what is recorded in the index, thus ending up leaving the new path in
the work tree.

Looks good to me. And from my tests, I think "git checkout -f" didn't have 
this problem at all, because it ends up using not got read-tree, but doing 
its own "reset_tree()" that uses unpack_trees().

I do wonder if "git reset" should perhaps be written in those terms, 
instead of just being a wrapper around git read-tree. But the patch looks 
fine.

		Linus

* Junio C Hamano [off-list ref] wrote:

When aborting a failed merge that has brought in a new path using "git 
reset --hard" or "git read-tree --reset -u", we used to first forget 
about the new path (via read_cache_unmerged) and then matched the 
working tree to what is recorded in the index, thus ending up leaving 
the new path in the work tree.

i've met this problem in various variants in the past few months, and i 
always assumed that it's "as designed" - as Git's policy is to never 
lose information unless forced to do so. (which i find very nice in 
general, and which saved modification from getting lost a couple of 
times in the past)

the situations where i end up with a messed up working tree [using 
git-c427559 right now]:

 - doing a conflicted Octopus merge will leave the tree in some weird 
   half-merged state, with lots of untracked working tree files that not 
   even a hard reset will recover from. The routine thing i do to clean 
   up is:

      git reset --hard HEAD
      git checkout HEAD .
      git ls-files --others | xargs rm              # DANGEROUS

   doing git checkout -f alone is not enough, as there might be various 
   dangling files left around.

 - git auto-gc thinking that it needs to do another pass in the middle 
   of a random git operation, but i dont have 10 minutes to wait so i 
   decide to Ctrl-C it.

 - doing the wrong "git checkout" and then Ctlr-C-ing it can leave the
   working tree in limbo as well, needing fixups. If i'm stuck between
   two branches that rename/remove files it might need the full fixup
   sequence above.

 - if a testbox has a corrupted system clock, its git repo and the 
   kernel build can get confused. This is to be expected i think - but
   the full sequence above will recover the corrupted tree. Not much Git
   can do about this i guess.

Does your fix mean that all i have to do in the future is a hard reset 
back to HEAD, and that dangling files are not supposed to stay around?

	Ingo