Change in "git checkout" behaviour between 1.6.0.2 and 1.6.0.3

The following works fine with 1.6.0.2 and before, but not 1.6.0.3 or later:

	git clone -n git git-test
        cd git-test
        git checkout -b work v1.6.0.2

When it breaks, the error is:

error: Entry '.gitignore' would be overwritten by merge. Cannot merge.

I'm guessing it's a bug rather than a deliberate change?

Bruce Stephens venit, vidit, dixit 12.11.2008 15:36:

Bisecting gives:

5521883490e85f4d973141972cf16f89a79f1979 is first bad commit commit 5521883490e85f4d973141972cf16f89a79f1979

Author: Junio C Hamano <[email protected]>
Date:   Sun Sep 7 19:49:25 2008 -0700

checkout: do not lose staged removal

CCing Junio...

Michael

Michael J Gruber <[email protected]> writes:

[...]

I got the same, which is reassuring.

Looks like a deliberate change with (what seems to me to be) an unfortunate interaction with "git clone -n"

Yeah, it was meant to allow:

git clone -n $there $here
cd $here
git checkout

and was not taking care of the case to switch branches when the initial checkout is made.

Perhaps this would help.

 builtin-checkout.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

index 05eee4e..d2265df 100644
@@ -269,8 +269,7 @@ static int merge_working_tree(struct checkout_opts *opts,
 		}
 
 		/* 2-way merge to the new branch */
-		topts.initial_checkout = (!active_nr &&
-					  (old->commit == new->commit));
+		topts.initial_checkout = !active_nr;
 		topts.update = 1;
 		topts.merge = 1;
 		topts.gently = opts->merge;

Here is a more involved but hopefully more maintainable fix.

-- >8 --
Subject: checkout: Fix "initial checkout" detection

Earlier commit 5521883 (checkout: do not lose staged removal, 2008-09-07) tightened the rule to prevent switching branches from losing local changes, so that staged removal of paths can be protected, while attempting to keep a loophole to still allow a special case of switching out of an un-checked-out state.

However, the loophole was made a bit too tight, and did not allow switching from one branch (in an un-checked-out state) to check out another branch.

The change to builtin-checkout.c in this commit loosens it to allow this, by not insisting the original commit and the new commit to be the same.

It also introduces a new function, is_index_unborn (and an associated macro, is_cache_unborn), to check if the repository is truly in an un-checked-out state more reliably, by making sure that $GIT_INDEX_FILE did not exist when populating the in-core index structure. A few places the earlier commit 5521883 added the check for the initial checkout condition are updated to use this function.

---
 builtin-checkout.c  |    3 +--
 builtin-read-tree.c |    2 +-
 cache.h             |    2 ++
 read-cache.c        |    5 +++++
 4 files changed, 9 insertions(+), 3 deletions(-)

index 05eee4e..25845cd 100644
@@ -269,8 +269,7 @@ static int merge_working_tree(struct checkout_opts *opts,
 		}
 
 		/* 2-way merge to the new branch */
-		topts.initial_checkout = (!active_nr &&
-					  (old->commit == new->commit));
+		topts.initial_checkout = is_cache_unborn();
 		topts.update = 1;
 		topts.merge = 1;
 		topts.gently = opts->merge;

index 0706c95..38fef34 100644
@@ -206,7 +206,7 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
 			break;
 		case 2:
 			opts.fn = twoway_merge;
-			opts.initial_checkout = !active_nr;
+			opts.initial_checkout = is_cache_unborn();
 			break;
 		case 3:
 		default:

index a1e4982..3960931 100644
@@ -255,6 +255,7 @@ static inline void remove_name_hash(struct cache_entry *ce)
 
 #define read_cache() read_index(&the_index)
 #define read_cache_from(path) read_index_from(&the_index, (path))
+#define is_cache_unborn() is_index_unborn(&the_index)
 #define read_cache_unmerged() read_index_unmerged(&the_index)
 #define write_cache(newfd, cache, entries) write_index(&the_index, (newfd))
 #define discard_cache() discard_index(&the_index)

@@ -360,6 +361,7 @@ extern int init_db(const char *template_dir, unsigned int flags);
 /* Initialize and use the cache information */
 extern int read_index(struct index_state *);
 extern int read_index_from(struct index_state *, const char *path);
+extern int is_index_unborn(struct index_state *);
 extern int read_index_unmerged(struct index_state *);
 extern int write_index(const struct index_state *, int newfd);
 extern int discard_index(struct index_state *);

index 967f483..525d138 100644
@@ -1239,6 +1239,11 @@ unmap:
 	die("index file corrupt");
 }
 
+int is_index_unborn(struct index_state *istate)
+{
+	return (!istate->cache_nr && !istate->alloc && !istate->timestamp);
+}
+
 int discard_index(struct index_state *istate)
 {
 	istate->cache_nr = 0;

Junio C Hamano <[email protected]> writes:

[...]

That specific sequence does work. I guess that's why I hadn't noticed the issue for so long (I guess git's test suite has some tests using "clone -n", and perhaps they're of that form).

Works for me.

[...]

According to "git bisect", the commit that caused this is the following one. Perhaps "git clone -n" doesn't start out with the index empty in the relevant sense?

commit 5521883490e85f4d973141972cf16f89a79f1979

Author: Junio C Hamano <[email protected]>
Date:   Sun Sep 7 19:49:25 2008 -0700

checkout: do not lose staged removal

The logic to checkout a different commit implements the safety to never lose user's local changes. For example, switching from a commit to another commit, when you have changed a path that is different between them, need to merge your changes to the version from the switched-to commit, which you may not necessarily be able to resolve easily. By default, "git checkout" refused to switch branches, to give you a chance to stash your local changes (or use "-m" to merge, accepting the risks of getting conflicts).

This safety, however, had one deliberate hole since early June 2005. When your local change was to remove a path (and optionally to stage that removal), the command checked out the path from the switched-to commit nevertheless.

This was to allow an initial checkout to happen smoothly (e.g. an initial checkout is done by starting with an empty index and switching from the commit at the HEAD to the same commit). We can tighten the rule slightly to allow this special case to pass, without losing sight of removal explicitly done by the user, by noticing if the index is truly empty when the operation begins.

For historical background, see:

http://thread.gmane.org/gmane.comp.version-control.git/4641/focus=4646

This case is marked as *0* in the message, which both Linus and I said "it feels somewhat wrong but otherwise we cannot start from an empty index".