"git checkout" branch switching safety broken in 'next'

"git checkout" branch switching safety broken in 'next'

[Junio C Hamano]

Linus, please be careful when switching branches if you are using your
unpack_trees() patch (in 'next') for production.  There is a breakage that
makes switching branches with "git checkout" lose your work-in-progress.

Traditionally, when you have a change in the work tree, and switch to
another branch that has different contents in the modified path, we
errored out by saying:

    error: Entry 'foo' not uptodate. Cannot merge.

The updated unpack_trees() notices the condition, issues this message
correctly and tries to propagate the error up, but loses the error at a
few places in the call chain.  The end result is that the caller does not
notice the two-way merge failure, and after that all h*ll breaks loose.

I think this should fix it.

-- >8 --
traverse_trees_recursive(): propagate merge errors up

There were few places where merge errors detected deeper in the call chain
were ignored and not propagated up the callchain to the caller.

Most notably, this caused "git checkout" to switch branches to internally
notice that a path modified in a work tree are different between the HEAD
version and the commit being switched to, but ignore that and switch
branches nevertheless, resulting in an incorrect two-way merge and loss of
the change in the work tree.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 unpack-trees.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index da68557..5a0f038 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -144,8 +144,7 @@ int traverse_trees_recursive(int n, unsigned long dirmask, unsigned long df_conf
 			sha1 = names[i].sha1;
 		fill_tree_descriptor(t+i, sha1);
 	}
-	traverse_trees(n, t, &newinfo);
-	return 0;
+	return traverse_trees(n, t, &newinfo);
 }
 
 /*
@@ -306,7 +305,9 @@ static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, str
 			if (src[0])
 				conflicts |= 1;
 		}
-		traverse_trees_recursive(n, dirmask, conflicts, names, info);
+		if (traverse_trees_recursive(n, dirmask, conflicts,
+					     names, info) < 0)
+			return -1;
 		return mask;
 	}
 

Re: "git checkout" branch switching safety broken in 'next'

[Jakub Narebski]

Junio C Hamano wrote:

> Traditionally, when you have a change in the work tree, and switch to
> another branch that has different contents in the modified path, we
> errored out by saying:
> 
>     error: Entry 'foo' not uptodate. Cannot merge.

By the way, could this error message be made less cryptic? It is there
since the very beginning, and was not changed during making error
messages more user friendly...

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: "git checkout" branch switching safety broken in 'next'

[Jay Soffian]

2008/3/10 Jakub Narebski <jnareb@gmail.com>:
> Junio C Hamano wrote:
> >     error: Entry 'foo' not uptodate. Cannot merge.
>
>  By the way, could this error message be made less cryptic? It is there
>  since the very beginning, and was not changed during making error
>  messages more user friendly...

+1. Most concise is probably:

 * "error: 'foo' has uncommitted changes."

More helpful messages that I can think of are probably too verbose. e.g.:

 * "error: 'foo' differs in target branch but has uncommitted changes."

 * "error: 'foo' has uncommitted changes. Commit, use -f to lose
    changes, or -m to merge changes in target branch."

j.

setting git-gui locale

[Maxim Fridental]

Hi!

Could someone point me please to a doc describing how to override language setting for git-gui tool (precompiled for Windows, downloaded from http://code.google.com/p/msysgit/downloads/list)? It is choosing German language for its menu items on any system I install it; and I personally find the German translations unusable.

Regards
Maxim Fridental

Re: setting git-gui locale

[Johannes Schindelin]

Hi,

On Tue, 11 Mar 2008, Maxim Fridental wrote:

> Could someone point me please to a doc describing how to override 
> language setting for git-gui tool (precompiled for Windows, downloaded 
> from http://code.google.com/p/msysgit/downloads/list)?

Try setting LANG=C or LANG=en before calling git-gui.

> It is choosing German language for its menu items on any system I 
> install it; and I personally find the German translations unusable.

Patches welcome,
Dscho

Re: "git checkout" branch switching safety broken in 'next'

[Linus Torvalds]

On Mon, 10 Mar 2008, Junio C Hamano wrote:
>
> Linus, please be careful when switching branches if you are using your
> unpack_trees() patch (in 'next') for production.  There is a breakage that
> makes switching branches with "git checkout" lose your work-in-progress.

Ouch. I've been using those patches "in production" since posting them, 
but since I don't generally use branches on the kernel (it happens, but 
it's rare), I wouldn't have noticed.

And in git, it probably wouldn't have triggered for me, since it looks 
like it would only trigger for changes in subdirectories, not in the top 
level (which is probably also why it passed all the tests!).

My bad. Your patch looks obviously correct.

			Linus

"git diff-index" broken in 'next'

[Junio C Hamano]

When you are in the middle of conflicted merge, it is useful to view "git
diff" that compares the stage #2 and your work tree (which has conflict
markers and both sides of the changes).  This still works.

However, with the unpack_trees() update, another form of useful sanity
check seems to be totally broken:

      $ git diff HEAD -- $conflicted_file

This used to show the diff between the HEAD and the work tree, but not
anymore.  It shows a diff as if the whole file is gone, and that is
because diff-lib.c::oneway_diff() gets idx==NULL from the caller for
unmerged entries. It never reaches show_modified() codepath.

I think it is somewhere around ll. 280 in unpack_callback() that causes
this, but I don't have time to dig this further for now.

Re: "git diff-index" broken in 'next'

[Linus Torvalds]

On Mon, 10 Mar 2008, Junio C Hamano wrote:
> 
> I think it is somewhere around ll. 280 in unpack_callback() that causes
> this, but I don't have time to dig this further for now.

Yes indeed. I was confused. I even knew I was confused about the whole 
difference between "skip_unmerged" and not skipping unmerged entries in 
the tree unpacking, but nothing seemed to care, so I was lazy.

Now that I remember what the heck that oneway_diff thing wanted, it all 
makes sense to me again.

This should fix it.

That oneway_diff() thing is magic, and used to have a "count_skip()" 
function to skip over unmerged entries. In my confusion, I had instead 
skipped them in the generic unpack-trees.c code, and thus removed the 
skipping from oneway_diff().

That wasn't correct - we should pass the unmerged entries down and let the callback handle it.

[ Side note: nobody else than the diff code should ever have them in the 
  source index anyway - that would be a bug, since we cannot merge with a 
  source index that has unmerged entries ]

Sorry about that. Holler if you notice anythign else.

		Linus

---
 diff-lib.c     |   18 ++++++++++++++++++
 unpack-trees.c |    2 --
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/diff-lib.c b/diff-lib.c
index 9520773..52dbac3 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -641,6 +641,21 @@ static void do_oneway_diff(struct unpack_trees_options *o,
 	show_modified(revs, tree, idx, 1, cached, match_missing);
 }
 
+static inline void skip_same_name(struct cache_entry *ce, struct unpack_trees_options *o)
+{
+	int len = ce_namelen(ce);
+	const struct index_state *index = o->src_index;
+
+	while (o->pos < index->cache_nr) {
+		struct cache_entry *next = index->cache[o->pos];
+		if (len != ce_namelen(next))
+			break;
+		if (memcmp(ce->name, next->name, len))
+			break;
+		o->pos++;
+	}
+}
+
 /*
  * The unpack_trees() interface is designed for merging, so
  * the different source entries are designed primarily for
@@ -662,6 +677,9 @@ static int oneway_diff(struct cache_entry **src, struct unpack_trees_options *o)
 	struct cache_entry *tree = src[1];
 	struct rev_info *revs = o->unpack_data;
 
+	if (idx && ce_stage(idx))
+		skip_same_name(idx, o);
+
 	/*
 	 * Unpack-trees generates a DF/conflict entry if
 	 * there was a directory in the index and a tree
diff --git a/unpack-trees.c b/unpack-trees.c
index da68557..7a30361 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -116,7 +116,6 @@ static int unpack_index_entry(struct cache_entry *ce, struct unpack_trees_option
 			add_entry(o, ce, 0, 0);
 			return 0;
 		}
-		return 0;
 	}
 	return call_unpack_fn(src, o);
 }
@@ -287,7 +286,6 @@ static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, str
 						add_entry(o, ce, 0, 0);
 						return mask;
 					}
-					continue;
 				}
 				src[0] = ce;
 			}