threads / discuss / 12614

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

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

## tl;dr

8 messages between Mar 10, 2008 and Mar 11, 2008.

replies: 7people: 6as markdown or json

Junio C Hamano· Mar 10, 2008, 08:12 UTC · lore

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;
 	}
Jakub Narebski· Mar 10, 2008, 09:29 UTC · re: Junio C Hamano · lore

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

Junio C Hamano wrote:
Show 5 quoted lines
> 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
Jay Soffian· Mar 10, 2008, 13:56 UTC · re: Jakub Narebski · lore

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

2008/3/10 Jakub Narebski <jnareb@gmail.com>:
Show 6 quoted lines
> 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.
Maxim Fridental· Mar 11, 2008, 08:57 UTC · re: Jay Soffian · lore

setting git-gui locale

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

Johannes Schindelin· Mar 11, 2008, 10:01 UTC · re: Maxim Fridental · lore

Re: setting git-gui locale

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

Linus Torvalds· Mar 10, 2008, 14:38 UTC · re: Junio C Hamano · lore

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

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
Junio C Hamano· Mar 11, 2008, 02:41 UTC · re: Linus Torvalds · lore

"git diff-index" broken in 'next'

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.

Linus Torvalds· Mar 11, 2008, 03:20 UTC · re: Junio C Hamano · lore

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

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;
 			}

← back to recent threads