[PATCH 0/6] Reusing "checkout [-m]" logic for "reset --merge"

[PATCH 0/6] Reusing "checkout [-m]" logic for "reset --merge"

[Junio C Hamano]

"checkout" that switches the branches, and "reset" that updates the HEAD,
have some similarities.  Their major difference is that "checkout" affects
which branch HEAD symref points at, while "reset" affects what commit such
a branch pointed at by the HEAD symref points at.  Interestingly, when
your HEAD is detached, they operate on the same thing.

They both change the commit pointed at by your HEAD from one commit to
another.  And what "checkout" and "reset --merge" do to the index and the
work tree while doing so are exactly the same.  Keep your local changes,
while updating everything else from what is in the current commit to the
one we are switching to.

There is one difference, though.

"reset --merge" does not have an equivalent of "checkout -m" that falls
back to three-way merge to resolve the conflicts at the content level.
This series, which is still a WIP, is an attempt to do so.

There are few things to notice that this series doesn't do:

 - What "reset --hard" and "checkout -f HEAD" do to the index and work
   tree (at least in naive thinking) ought to be the same.  This series
   does not attempt to unify these.

 - "reset --merge" is left as posted by Linus, and it does only "git
   checkout" equivalent, without "-m" (yet).

The latter can be enabled by changing a single line (see comment in
reset_index_file() in [Patch 6/6]), but I haven't done so yet.

While investigating for the former possibility, I noticed one interesting
thing what "checkout -f" does *NOT* do.  Starting with an index with
conflicts, "git checkout -f HEAD" keeps higher stage entries.

For example, you can insert "exit" before the "D/F resolve" test in t1004,
run the test (this leaves a handful higher stages in the index), go to the
trash directory and say "git checkout -f HEAD".

It leaves a single stage #1 entry (subdir/file2).  It probably is a bug in
unpack-trees, but I didn't take a very deep look at it.

Junio C Hamano (6):
  builtin-checkout.c: check error return from read_cache()
  read-cache.c: typofix in comment
  Make reset_tree() in builtin-checkout.c a bit more library-ish
  builtin-checkout.c: refactor merge_working_tree()
  builtin-commit.c: further refactor branch switching
  builtin-reset.c: use reset_index_and_worktree()

 builtin-checkout.c |  193 +++++++++++++++++++++++++++++-----------------------
 builtin-reset.c    |   66 +++++++++++++++++-
 read-cache.c       |    2 +-
 reset.h            |   11 +++
 4 files changed, 183 insertions(+), 89 deletions(-)
 create mode 100644 reset.h

[PATCH 1/6] builtin-checkout.c: check error return from read_cache()

[Junio C Hamano]

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-checkout.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/builtin-checkout.c b/builtin-checkout.c
index 7f3bd7b..c2c0561 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -228,7 +228,8 @@ static int checkout_paths(struct tree *source_tree, const char **pathspec,
 	struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));
 
 	newfd = hold_locked_index(lock_file, 1);
-	read_cache();
+	if (read_cache() < 0)
+		return error("corrupt index file");
 
 	if (source_tree)
 		read_tree_some(source_tree, pathspec);
@@ -371,7 +372,9 @@ static int merge_working_tree(struct checkout_opts *opts,
 	int ret;
 	struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));
 	int newfd = hold_locked_index(lock_file, 1);
-	read_cache();
+
+	if (read_cache() < 0)
+		return error("corrupt index file");
 
 	if (opts->force) {
 		ret = reset_tree(new->commit->tree, opts, 1);
-- 
1.6.1.rc1.72.ga5ce6

[PATCH 2/6] read-cache.c: typofix in comment

[Junio C Hamano]

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 read-cache.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index 22a8143..58d3f88 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1486,7 +1486,7 @@ int write_index(const struct index_state *istate, int newfd)
 
 /*
  * Read the index file that is potentially unmerged into given
- * index_state, dropping any unmerged entries.  Returns true is
+ * index_state, dropping any unmerged entries.  Returns true if
  * the index is unmerged.  Callers who want to refuse to work
  * from an unmerged state can call this and check its return value,
  * instead of calling read_cache().
-- 
1.6.1.rc1.72.ga5ce6

[PATCH 3/6] Make reset_tree() in builtin-checkout.c a bit more library-ish

[Junio C Hamano]

The function demanded an internal structure "struct checkout_opts" as its
argument, but we'd want to split this (and one of its callers) out into a
separate library.  This makes what the function wants a bit more explicit
and much less dependent on the rest of builtin-checkout.c

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-checkout.c |   15 +++++++++------
 1 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/builtin-checkout.c b/builtin-checkout.c
index c2c0561..d88fce2 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -319,7 +319,7 @@ static void describe_detached_head(char *msg, struct commit *commit)
 	strbuf_release(&sb);
 }
 
-static int reset_tree(struct tree *tree, struct checkout_opts *o, int worktree)
+static int reset_tree(struct tree *tree, int quiet, int worktree, int *wt_error)
 {
 	struct unpack_trees_options opts;
 	struct tree_desc tree_desc;
@@ -331,14 +331,14 @@ static int reset_tree(struct tree *tree, struct checkout_opts *o, int worktree)
 	opts.reset = 1;
 	opts.merge = 1;
 	opts.fn = oneway_merge;
-	opts.verbose_update = !o->quiet;
+	opts.verbose_update = !quiet;
 	opts.src_index = &the_index;
 	opts.dst_index = &the_index;
 	parse_tree(tree);
 	init_tree_desc(&tree_desc, tree->buffer, tree->size);
 	switch (unpack_trees(1, &tree_desc, &opts)) {
 	case -2:
-		o->writeout_error = 1;
+		*wt_error = 1;
 		/*
 		 * We return 0 nevertheless, as the index is all right
 		 * and more importantly we have made best efforts to
@@ -377,7 +377,8 @@ static int merge_working_tree(struct checkout_opts *opts,
 		return error("corrupt index file");
 
 	if (opts->force) {
-		ret = reset_tree(new->commit->tree, opts, 1);
+		ret = reset_tree(new->commit->tree, opts->quiet, 1,
+				 &opts->writeout_error);
 		if (ret)
 			return ret;
 	} else {
@@ -446,14 +447,16 @@ static int merge_working_tree(struct checkout_opts *opts,
 			o.verbosity = 0;
 			work = write_tree_from_memory(&o);
 
-			ret = reset_tree(new->commit->tree, opts, 1);
+			ret = reset_tree(new->commit->tree, opts->quiet, 1,
+					 &opts->writeout_error);
 			if (ret)
 				return ret;
 			o.branch1 = new->name;
 			o.branch2 = "local";
 			merge_trees(&o, new->commit->tree, work,
 				old->commit->tree, &result);
-			ret = reset_tree(new->commit->tree, opts, 0);
+			ret = reset_tree(new->commit->tree, opts->quiet, 0,
+					 &opts->writeout_error);
 			if (ret)
 				return ret;
 		}
-- 
1.6.1.rc1.72.ga5ce6

[PATCH 4/6] builtin-checkout.c: refactor merge_working_tree()

[Junio C Hamano]

The logic to bring the index and the working tree from one commit to
another is a nontrivial amount of code in this function.  Separate it out
into its own function, so that other callers can call it.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-checkout.c |  172 +++++++++++++++++++++++++++-------------------------
 1 files changed, 89 insertions(+), 83 deletions(-)

diff --git a/builtin-checkout.c b/builtin-checkout.c
index d88fce2..9c45c49 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -352,6 +352,87 @@ static int reset_tree(struct tree *tree, int quiet, int worktree, int *wt_error)
 	}
 }
 
+static int switch_trees(int merge, int quiet, int *wt_error,
+			struct commit *old_commit, const char *old_label,
+			struct commit *new_commit, const char *new_label)
+{
+	int ret;
+	struct tree_desc trees[2];
+	struct tree *tree;
+	struct unpack_trees_options topts;
+
+	memset(&topts, 0, sizeof(topts));
+	topts.head_idx = -1;
+	topts.src_index = &the_index;
+	topts.dst_index = &the_index;
+
+	topts.msgs.not_uptodate_file = "You have local changes to '%s'; cannot switch branches.";
+
+	refresh_cache(REFRESH_QUIET);
+
+	if (unmerged_cache()) {
+		error("you need to resolve your current index first");
+		return 1;
+	}
+
+	/* 2-way merge to the new branch */
+	topts.initial_checkout = is_cache_unborn();
+	topts.update = 1;
+	topts.merge = 1;
+	topts.gently = merge;
+	topts.verbose_update = !quiet;
+	topts.fn = twoway_merge;
+	topts.dir = xcalloc(1, sizeof(*topts.dir));
+	topts.dir->show_ignored = 1;
+	topts.dir->exclude_per_dir = ".gitignore";
+	tree = parse_tree_indirect(old_commit->object.sha1);
+	init_tree_desc(&trees[0], tree->buffer, tree->size);
+	tree = parse_tree_indirect(new_commit->object.sha1);
+	init_tree_desc(&trees[1], tree->buffer, tree->size);
+
+	ret = unpack_trees(2, trees, &topts);
+	if (ret == -1) {
+		/*
+		 * Unpack couldn't do a trivial merge; either give up
+		 * or do a real merge, depending on whether the merge
+		 * flag was used.
+		 */
+		struct tree *result;
+		struct tree *work;
+		struct merge_options o;
+		if (!merge)
+			return 1;
+		parse_commit(old_commit);
+
+		/* Do more real merge */
+
+		/*
+		 * We update the index fully, then write the tree from
+		 * the index, then merge the new branch with the
+		 * current tree, with the old branch as the base. Then
+		 * we reset the index (but not the working tree) to
+		 * the new branch, leaving the working tree as the
+		 * merged version, but skipping unmerged entries in
+		 * the index.
+		 */
+
+		add_files_to_cache(NULL, NULL, 0);
+		init_merge_options(&o);
+		o.verbosity = 0;
+		work = write_tree_from_memory(&o);
+
+		ret = reset_tree(new_commit->tree, quiet, 1, wt_error);
+		if (ret)
+			return ret;
+		o.branch1 = new_label;
+		o.branch2 = old_label;
+		merge_trees(&o, new_commit->tree, work,
+			    old_commit->tree, &result);
+		ret = reset_tree(new_commit->tree, quiet, 0, wt_error);
+	}
+	return ret;
+}
+
 struct branch_info {
 	const char *name; /* The short name used */
 	const char *path; /* The full name of a real branch */
@@ -376,91 +457,16 @@ static int merge_working_tree(struct checkout_opts *opts,
 	if (read_cache() < 0)
 		return error("corrupt index file");
 
-	if (opts->force) {
+	if (opts->force)
 		ret = reset_tree(new->commit->tree, opts->quiet, 1,
 				 &opts->writeout_error);
-		if (ret)
-			return ret;
-	} else {
-		struct tree_desc trees[2];
-		struct tree *tree;
-		struct unpack_trees_options topts;
-
-		memset(&topts, 0, sizeof(topts));
-		topts.head_idx = -1;
-		topts.src_index = &the_index;
-		topts.dst_index = &the_index;
-
-		topts.msgs.not_uptodate_file = "You have local changes to '%s'; cannot switch branches.";
-
-		refresh_cache(REFRESH_QUIET);
-
-		if (unmerged_cache()) {
-			error("you need to resolve your current index first");
-			return 1;
-		}
-
-		/* 2-way merge to the new branch */
-		topts.initial_checkout = is_cache_unborn();
-		topts.update = 1;
-		topts.merge = 1;
-		topts.gently = opts->merge;
-		topts.verbose_update = !opts->quiet;
-		topts.fn = twoway_merge;
-		topts.dir = xcalloc(1, sizeof(*topts.dir));
-		topts.dir->show_ignored = 1;
-		topts.dir->exclude_per_dir = ".gitignore";
-		tree = parse_tree_indirect(old->commit->object.sha1);
-		init_tree_desc(&trees[0], tree->buffer, tree->size);
-		tree = parse_tree_indirect(new->commit->object.sha1);
-		init_tree_desc(&trees[1], tree->buffer, tree->size);
-
-		ret = unpack_trees(2, trees, &topts);
-		if (ret == -1) {
-			/*
-			 * Unpack couldn't do a trivial merge; either
-			 * give up or do a real merge, depending on
-			 * whether the merge flag was used.
-			 */
-			struct tree *result;
-			struct tree *work;
-			struct merge_options o;
-			if (!opts->merge)
-				return 1;
-			parse_commit(old->commit);
-
-			/* Do more real merge */
-
-			/*
-			 * We update the index fully, then write the
-			 * tree from the index, then merge the new
-			 * branch with the current tree, with the old
-			 * branch as the base. Then we reset the index
-			 * (but not the working tree) to the new
-			 * branch, leaving the working tree as the
-			 * merged version, but skipping unmerged
-			 * entries in the index.
-			 */
-
-			add_files_to_cache(NULL, NULL, 0);
-			init_merge_options(&o);
-			o.verbosity = 0;
-			work = write_tree_from_memory(&o);
-
-			ret = reset_tree(new->commit->tree, opts->quiet, 1,
-					 &opts->writeout_error);
-			if (ret)
-				return ret;
-			o.branch1 = new->name;
-			o.branch2 = "local";
-			merge_trees(&o, new->commit->tree, work,
-				old->commit->tree, &result);
-			ret = reset_tree(new->commit->tree, opts->quiet, 0,
-					 &opts->writeout_error);
-			if (ret)
-				return ret;
-		}
-	}
+	else
+		ret = switch_trees(opts->merge, opts->quiet,
+				   &opts->writeout_error,
+				   old->commit, "local",
+				   new->commit, new->name);
+	if (ret)
+		return ret;
 
 	if (write_cache(newfd, active_cache, active_nr) ||
 	    commit_locked_index(lock_file))
-- 
1.6.1.rc1.72.ga5ce6

[PATCH 5/6] builtin-commit.c: further refactor branch switching

[Junio C Hamano]

This adds reset_index_and_worktree() to further factor out the logic for
switching the index and the work tree state from one commit to another to
shrink merge_working_tree() function.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-checkout.c |   24 ++++++++++++++++--------
 1 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/builtin-checkout.c b/builtin-checkout.c
index 9c45c49..a08941a 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -433,6 +433,18 @@ static int switch_trees(int merge, int quiet, int *wt_error,
 	return ret;
 }
 
+int reset_index_and_worktree(int force, int merge, int quiet, int *wt_error,
+			     struct commit *old_commit, const char *old_label,
+			     struct commit *new_commit, const char *new_label)
+{
+	if (force)
+		return reset_tree(new_commit->tree, quiet, 1, wt_error);
+	else
+		return switch_trees(merge, quiet, wt_error,
+				    old_commit, old_label,
+				    new_commit, new_label);
+}
+
 struct branch_info {
 	const char *name; /* The short name used */
 	const char *path; /* The full name of a real branch */
@@ -457,14 +469,10 @@ static int merge_working_tree(struct checkout_opts *opts,
 	if (read_cache() < 0)
 		return error("corrupt index file");
 
-	if (opts->force)
-		ret = reset_tree(new->commit->tree, opts->quiet, 1,
-				 &opts->writeout_error);
-	else
-		ret = switch_trees(opts->merge, opts->quiet,
-				   &opts->writeout_error,
-				   old->commit, "local",
-				   new->commit, new->name);
+	ret = reset_index_and_worktree(opts->force, opts->merge, opts->quiet,
+				       &opts->writeout_error,
+				       old->commit, "local",
+				       new->commit, new->name);
 	if (ret)
 		return ret;
 
-- 
1.6.1.rc1.72.ga5ce6

[PATCH 6/6] builtin-reset.c: use reset_index_and_worktree()

[Junio C Hamano]

This makes "git reset --merge" to use the same underlying mechanism "git
checkout" uses to update the index and the work tree.

It is possible to make it use the 3-way merge fallback "git checkout -m"
allows, but this commit does not go there yet.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-checkout.c |    1 +
 builtin-reset.c    |   66 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 reset.h            |   11 ++++++++
 3 files changed, 76 insertions(+), 2 deletions(-)
 create mode 100644 reset.h

diff --git a/builtin-checkout.c b/builtin-checkout.c
index a08941a..d196521 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -16,6 +16,7 @@
 #include "blob.h"
 #include "xdiff-interface.h"
 #include "ll-merge.h"
+#include "reset.h"
 
 static const char * const checkout_usage[] = {
 	"git checkout [options] <branch>",
diff --git a/builtin-reset.c b/builtin-reset.c
index c0cb915..481a1cc 100644
--- a/builtin-reset.c
+++ b/builtin-reset.c
@@ -18,6 +18,7 @@
 #include "tree.h"
 #include "branch.h"
 #include "parse-options.h"
+#include "reset.h"
 
 static const char * const git_reset_usage[] = {
 	"git reset [--mixed | --soft | --hard | --merge] [-q] [<commit>]",
@@ -52,7 +53,7 @@ static inline int is_merge(void)
 	return !access(git_path("MERGE_HEAD"), F_OK);
 }
 
-static int reset_index_file(const unsigned char *sha1, int reset_type, int quiet)
+static int reset_index_file_via_read_tree(const unsigned char *sha1, int reset_type, int quiet)
 {
 	int i = 0;
 	const char *args[6];
@@ -77,6 +78,67 @@ static int reset_index_file(const unsigned char *sha1, int reset_type, int quiet
 	return run_command_v_opt(args, RUN_GIT_CMD);
 }
 
+static int reset_index_file(struct commit *new, int reset_type, int quiet)
+{
+	/*
+	 * SOFT reset won't even touch index nor work tree so
+	 * this function is not called.
+	 * MIXED updates the index only (should have been called
+	 * --cached), and we let "git read-tree" to do so.
+	 * HARD and MERGE corresponds to "checkout -f" and "checkout [-m]"
+	 */
+	int merge, wt_error, ret;
+	struct commit *old;
+	unsigned char head_sha1[20];
+	unsigned char *new_sha1 = new->object.sha1;
+	struct lock_file *lock_file;
+	int newfd;
+
+	/*
+	 * We play lazy and let read-tree complain if HEAD is not
+	 * readable.  Also on hard reset, HEAD does not have to be
+	 * readable.
+	 */
+	if (reset_type == MIXED ||
+	    reset_type == HARD ||
+	    get_sha1("HEAD", head_sha1) ||
+	    !(old = lookup_commit_reference_gently(head_sha1, 1)))
+		return reset_index_file_via_read_tree(new_sha1, reset_type,
+						      quiet);
+
+	lock_file = xcalloc(1, sizeof(struct lock_file));
+	newfd = hold_locked_index(lock_file, 1);
+	if (read_cache() < 0) {
+		rollback_lock_file(lock_file);
+		return reset_index_file_via_read_tree(new_sha1, reset_type,
+						      quiet);
+	}
+
+	/*
+	 * If we want "checkout -m" behaviour of falling back to
+	 * the 3-way content level merges, we could use
+	 *
+	 *	 merge = (reset_type == MERGE);
+	 *
+	 * here.
+	 */
+	merge = 0;
+
+	wt_error = 0;
+	ret = reset_index_and_worktree(0, merge, quiet, &wt_error,
+				       old, "local",
+				       new, "reset");
+	if (ret || wt_error) {
+		rollback_lock_file(lock_file);
+		return -1;
+	}
+
+	if (write_cache(newfd, active_cache, active_nr) ||
+	    commit_locked_index(lock_file))
+		return error("unable to write new index file");
+	return 0;
+}
+
 static void print_new_head_line(struct commit *commit)
 {
 	const char *hex, *body;
@@ -276,7 +338,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 		if (is_merge() || read_cache() < 0 || unmerged_cache())
 			die("Cannot do a soft reset in the middle of a merge.");
 	}
-	else if (reset_index_file(sha1, reset_type, quiet))
+	else if (reset_index_file(commit, reset_type, quiet))
 		die("Could not reset index file to revision '%s'.", rev);
 
 	/* Any resets update HEAD to the head being switched to,
diff --git a/reset.h b/reset.h
new file mode 100644
index 0000000..9c42838
--- /dev/null
+++ b/reset.h
@@ -0,0 +1,11 @@
+#ifndef RESET_H
+#define RESET_H
+
+extern int reset_index_and_worktree(int force, int merge, int quiet,
+				    int *wt_error,
+				    struct commit *old_commit,
+				    const char *old_label,
+				    struct commit *new_commit,
+				    const char *new_label);
+
+#endif
-- 
1.6.1.rc1.72.ga5ce6

Re: [PATCH 5/6] builtin-commit.c: further refactor branch switching

[Linus Torvalds]

The whole series looks good, I think.

But one thing I reacted to is exemplified best by this one:

On Fri, 5 Dec 2008, Junio C Hamano wrote:
>
> +int reset_index_and_worktree(int force, int merge, int quiet, int *wt_error,
> +			     struct commit *old_commit, const char *old_label,
> +			     struct commit *new_commit, const char *new_label)

This takes three flags (force, merge, quiet) as three parameters.

And in the series, 3/6 and 4/6 had other cases where the same flags (just 
not _all_ of them) were passed around to other functions. In 4/6, you had 
"switch_trees()" taking "int merge, int quiet", and in 3/6 you had 
"reset_tree()" taking "int quiet" and also have a "int worktree" flag.

I think it would be really nice to have a unified namespace for these 
flags. There are lots of commonalities between "checkout" and "reset" (and 
certainly differences too), wouldn't it be nice to have something like

	#define CO_FORCE 0x001
	#define CO_MERGE 0x002
	#define CO_QUIET 0x004
	#define CO_WORKTREE 0x008

and just pass in a single "unsigned int checkout_flags" as an argument?

Having eight parameters is obscene.

		Linus