Change in "git checkout" behaviour between 1.6.0.2 and 1.6.0.3

Change in "git checkout" behaviour between 1.6.0.2 and 1.6.0.3

[Bruce Stephens]

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?

Re: Change in "git checkout" behaviour between 1.6.0.2 and 1.6.0.3

[Michael J Gruber]

Bruce Stephens venit, vidit, dixit 12.11.2008 15:36:
> 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?

Bisecting gives:


5521883490e85f4d973141972cf16f89a79f1979 is first bad commit
commit 5521883490e85f4d973141972cf16f89a79f1979
Author: Junio C Hamano <gitster@pobox.com>
Date:   Sun Sep 7 19:49:25 2008 -0700

    checkout: do not lose staged removal

CCing Junio...

Michael

Re: Change in "git checkout" behaviour between 1.6.0.2 and 1.6.0.3

[Bruce Stephens]

Bruce Stephens <bruce.stephens@isode.com> writes:

> 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?

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 <gitster@pobox.com>
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".
    
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

Re: Change in "git checkout" behaviour between 1.6.0.2 and 1.6.0.3

[Bruce Stephens]

Michael J Gruber <git@drmicha.warpmail.net> writes:

[...]

> Bisecting gives:
>
>
> 5521883490e85f4d973141972cf16f89a79f1979 is first bad commit
> commit 5521883490e85f4d973141972cf16f89a79f1979
> Author: Junio C Hamano <gitster@pobox.com>
> Date:   Sun Sep 7 19:49:25 2008 -0700
>
>     checkout: do not lose staged removal

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"

Re: Change in "git checkout" behaviour between 1.6.0.2 and 1.6.0.3

[Junio C Hamano]

Bruce Stephens <bruce.stephens@isode.com> writes:

> Michael J Gruber <git@drmicha.warpmail.net> writes:
>
> [...]
>
>> Bisecting gives:
>>
>>
>> 5521883490e85f4d973141972cf16f89a79f1979 is first bad commit
>> commit 5521883490e85f4d973141972cf16f89a79f1979
>> Author: Junio C Hamano <gitster@pobox.com>
>> Date:   Sun Sep 7 19:49:25 2008 -0700
>>
>>     checkout: do not lose staged removal
>
> 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(-)

diff --git c/builtin-checkout.c w/builtin-checkout.c
index 05eee4e..d2265df 100644
--- c/builtin-checkout.c
+++ w/builtin-checkout.c
@@ -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;

        

Re: Change in "git checkout" behaviour between 1.6.0.2 and 1.6.0.3

[Bruce Stephens]

Junio C Hamano <gitster@pobox.com> writes:

[...]

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

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).

> Perhaps this would help.

Works for me.

[...]

Re* Change in "git checkout" behaviour between 1.6.0.2 and 1.6.0.3

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

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

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.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-checkout.c  |    3 +--
 builtin-read-tree.c |    2 +-
 cache.h             |    2 ++
 read-cache.c        |    5 +++++
 4 files changed, 9 insertions(+), 3 deletions(-)

diff --git c/builtin-checkout.c w/builtin-checkout.c
index 05eee4e..25845cd 100644
--- c/builtin-checkout.c
+++ w/builtin-checkout.c
@@ -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;
diff --git c/builtin-read-tree.c w/builtin-read-tree.c
index 0706c95..38fef34 100644
--- c/builtin-read-tree.c
+++ w/builtin-read-tree.c
@@ -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:
diff --git c/cache.h w/cache.h
index a1e4982..3960931 100644
--- c/cache.h
+++ w/cache.h
@@ -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 *);
diff --git c/read-cache.c w/read-cache.c
index 967f483..525d138 100644
--- c/read-cache.c
+++ w/read-cache.c
@@ -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;