[PATCH 1/2] checkout: do not check out unmerged higher stages randomly

[PATCH 1/2] checkout: do not check out unmerged higher stages randomly

[Junio C Hamano]

During a conflicted merge when you have unmerged stages for a path F in
the index, if you asked:

    $ git checkout F

we rewrote F as many times as we have stages for it, and the last one
(typically "theirs") was left in the work tree, without resolving the
conflict.

This patch fixes it by noticing that a specified pathspec pattern matches
an unmerged path, and by erroring out.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-checkout.c |   27 +++++++++++++++++++++++++++
 t/t7201-co.sh      |   23 +++++++++++++++++++++++
 2 files changed, 50 insertions(+), 0 deletions(-)

diff --git a/builtin-checkout.c b/builtin-checkout.c
index b380ad6..9b33f3a 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -76,6 +76,15 @@ static int read_tree_some(struct tree *tree, const char **pathspec)
 	return 0;
 }
 
+static int skip_same_name(struct cache_entry *ce, int pos)
+{
+	while (++pos < active_nr &&
+	       !strcmp(active_cache[pos]->name, ce->name))
+		; /* skip */
+	return pos;
+}
+
+
 static int checkout_paths(struct tree *source_tree, const char **pathspec)
 {
 	int pos;
@@ -107,6 +116,20 @@ static int checkout_paths(struct tree *source_tree, const char **pathspec)
 	if (report_path_error(ps_matched, pathspec, 0))
 		return 1;
 
+	/* Any unmerged paths? */
+	for (pos = 0; pos < active_nr; pos++) {
+		struct cache_entry *ce = active_cache[pos];
+		if (pathspec_match(pathspec, NULL, ce->name, 0) &&
+		    ce_stage(ce)) {
+			errs = 1;
+			error("path '%s' is unmerged", ce->name);
+			pos = skip_same_name(ce, pos) - 1;
+			continue;
+		}
+	}
+	if (errs)
+		return 1;
+
 	/* Now we are committed to check them out */
 	memset(&state, 0, sizeof(state));
 	state.force = 1;
@@ -114,6 +137,10 @@ static int checkout_paths(struct tree *source_tree, const char **pathspec)
 	for (pos = 0; pos < active_nr; pos++) {
 		struct cache_entry *ce = active_cache[pos];
 		if (pathspec_match(pathspec, NULL, ce->name, 0)) {
+			if (ce_stage(ce)) {
+				pos = skip_same_name(ce, pos) - 1;
+				continue;
+			}
 			errs |= checkout_entry(ce, &state, NULL);
 		}
 	}
diff --git a/t/t7201-co.sh b/t/t7201-co.sh
index 1dff84d..303cf62 100755
--- a/t/t7201-co.sh
+++ b/t/t7201-co.sh
@@ -369,4 +369,27 @@ test_expect_success \
     'checkout with --track, but without -b, fails with too short tracked name' '
     test_must_fail git checkout --track renamer'
 
+test_expect_success 'checkout an unmerged path should fail' '
+	rm -f .git/index &&
+	O=$(echo original | git hash-object -w --stdin) &&
+	A=$(echo ourside | git hash-object -w --stdin) &&
+	B=$(echo theirside | git hash-object -w --stdin) &&
+	(
+		echo "100644 $A 0	fild" &&
+		echo "100644 $O 1	file" &&
+		echo "100644 $A 2	file" &&
+		echo "100644 $B 3	file" &&
+		echo "100644 $A 0	filf"
+	) | git update-index --index-info &&
+	echo "none of the above" >sample &&
+	cat sample >fild &&
+	cat sample >file &&
+	cat sample >filf &&
+	test_must_fail git checkout fild file filf &&
+	test_cmp sample fild &&
+	test_cmp sample filf &&
+	test_cmp sample file
+'
+
 test_done
+
-- 
1.6.0.1.90.g27a6e

[PATCH 2/2] checkout: allow ignoring unmerged paths when checking out of the index

[Junio C Hamano]

Earlier we made "git checkout $pathspec" to atomically refuse the
operation, if $pathspec matched any path with unmerged stages.

This patch allows:

    $ git checkout -f a b c

to ignore, instead of erroring out on, such unmerged paths.  The fix to
prevent checkout of an unmerged path from random stages is still there.

Signed-off-by: Junio C Hamano <junio@twinsun.com>
---
 builtin-checkout.c |   41 +++++++++++++++++++++++------------------
 t/t7201-co.sh      |   23 +++++++++++++++++++++++
 2 files changed, 46 insertions(+), 18 deletions(-)

diff --git a/builtin-checkout.c b/builtin-checkout.c
index 9b33f3a..49c43d9 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -20,6 +20,17 @@ static const char * const checkout_usage[] = {
 	NULL,
 };
 
+struct checkout_opts {
+	int quiet;
+	int merge;
+	int force;
+	int writeout_error;
+
+	const char *new_branch;
+	int new_branch_log;
+	enum branch_track track;
+};
+
 static int post_checkout_hook(struct commit *old, struct commit *new,
 			      int changed)
 {
@@ -85,7 +96,8 @@ static int skip_same_name(struct cache_entry *ce, int pos)
 }
 
 
-static int checkout_paths(struct tree *source_tree, const char **pathspec)
+static int checkout_paths(struct tree *source_tree, const char **pathspec,
+			  struct checkout_opts *opts)
 {
 	int pos;
 	struct checkout state;
@@ -121,8 +133,12 @@ static int checkout_paths(struct tree *source_tree, const char **pathspec)
 		struct cache_entry *ce = active_cache[pos];
 		if (pathspec_match(pathspec, NULL, ce->name, 0) &&
 		    ce_stage(ce)) {
-			errs = 1;
-			error("path '%s' is unmerged", ce->name);
+			if (!opts->force) {
+				errs = 1;
+				error("path '%s' is unmerged", ce->name);
+			} else {
+				warning("path '%s' is unmerged", ce->name);
+			}
 			pos = skip_same_name(ce, pos) - 1;
 			continue;
 		}
@@ -178,17 +194,6 @@ static void describe_detached_head(char *msg, struct commit *commit)
 	strbuf_release(&sb);
 }
 
-struct checkout_opts {
-	int quiet;
-	int merge;
-	int force;
-	int writeout_error;
-
-	const char *new_branch;
-	int new_branch_log;
-	enum branch_track track;
-};
-
 static int reset_tree(struct tree *tree, struct checkout_opts *o, int worktree)
 {
 	struct unpack_trees_options opts;
@@ -569,15 +574,15 @@ no_reference:
 			die("invalid path specification");
 
 		/* Checkout paths */
-		if (opts.new_branch || opts.force || opts.merge) {
+		if (opts.new_branch || opts.merge) {
 			if (argc == 1) {
-				die("git checkout: updating paths is incompatible with switching branches/forcing\nDid you intend to checkout '%s' which can not be resolved as commit?", argv[0]);
+				die("git checkout: updating paths is incompatible with switching branches.\nDid you intend to checkout '%s' which can not be resolved as commit?", argv[0]);
 			} else {
-				die("git checkout: updating paths is incompatible with switching branches/forcing");
+				die("git checkout: updating paths is incompatible with switching branches.");
 			}
 		}
 
-		return checkout_paths(source_tree, pathspec);
+		return checkout_paths(source_tree, pathspec, &opts);
 	}
 
 	if (new.name && !new.commit) {
diff --git a/t/t7201-co.sh b/t/t7201-co.sh
index 303cf62..bbf86cb 100755
--- a/t/t7201-co.sh
+++ b/t/t7201-co.sh
@@ -391,5 +391,28 @@ test_expect_success 'checkout an unmerged path should fail' '
 	test_cmp sample file
 '
 
+test_expect_success 'checkout with an unmerged path can ignore' '
+	rm -f .git/index &&
+	O=$(echo original | git hash-object -w --stdin) &&
+	A=$(echo ourside | git hash-object -w --stdin) &&
+	B=$(echo theirside | git hash-object -w --stdin) &&
+	(
+		echo "100644 $A 0	fild" &&
+		echo "100644 $O 1	file" &&
+		echo "100644 $A 2	file" &&
+		echo "100644 $B 3	file" &&
+		echo "100644 $A 0	filf"
+	) | git update-index --index-info &&
+	echo "none of the above" >sample &&
+	echo ourside >expect &&
+	cat sample >fild &&
+	cat sample >file &&
+	cat sample >filf &&
+	git checkout -f fild file filf &&
+	test_cmp expect fild &&
+	test_cmp expect filf &&
+	test_cmp sample file
+'
+
 test_done
 
-- 
1.6.0.1.90.g27a6e

Re: [PATCH 2/2] checkout: allow ignoring unmerged paths when checking out of the index

[Junio C Hamano]

There is an obvious sequel to this story.  We can enhance the behaviour of
checkout out of the index during a conflicted merge by doing the
following, which is the reason why I stumbled upon this bug that was fixed
by these two patches:

 * Teach "git checkout --ours", "git checkout ---theirs" so that stages 2
   and stages 3 for unmerged paths are checked out (and possibly resolve
   the conflict).

 * Teach "git checkout --merge" to redo the file level merge between
   stages 1/2/3, and update the work tree with the result.

Re: [PATCH 1/2] checkout: do not check out unmerged higher stages randomly

[Junio C Hamano]

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

> During a conflicted merge when you have unmerged stages for a path F in
> the index, if you asked:
>
>     $ git checkout F
>
> we rewrote F as many times as we have stages for it, and the last one
> (typically "theirs") was left in the work tree, without resolving the
> conflict.
>
> This patch fixes it by noticing that a specified pathspec pattern matches
> an unmerged path, and by erroring out.

The patch claims that it is a bugfix, but I have a slight worry that
somebody might be depending on this broken behaviour in their workflow.

    $ git pull
    Conflicts in foo.c (modify/modify)
    .. Eeek, conflicts.  My changes are always crap, and I trust my
    .. upstream more than I trust myself, so I'll take advantage of
    .. the bug to check out "their" version.
    $ git checkout foo.c

In modify/modify case, because we will have stage #3 (state after their
change), that will be the version you will get as the final result of this
broken behaviour of "checkout".

This patch alone will make the above to check out "theirs" an error.  In a
later one in the series, you can still say:

    $ git checkout --theirs foo.c ;# or "git checkout -3 foo.c"

to get the feature back, at the same time you will now be able to say

    $ git checkout --ours foo.c ;# or "git checkout -2 foo.c"

for symmetry (admittably, it is the same thing as "git checkout HEAD
foo.c").

So we _could_ choose to stay backward compatible by leaving this "bug"
unattended without loss of ability to checkout either theirs or ours, even
though without fixing this, you cannot sanely add "git checkout -m foo.c"
feature that comes later in the updated series, which allows you to go
back to the original conflicted state after you (or rerere) messed up the
working tree state.

Thoughts?