Merge branch 'jc/unresolve-removal'

"checkout --merge -- path" and "update-index --unresolve path" did
not resurrect conflicted state that was resolved to remove path,
but now they do.

* jc/unresolve-removal:
  checkout: allow "checkout -m path" to unmerge removed paths
  checkout/restore: add basic tests for --merge
  checkout/restore: refuse unmerging paths unless checking out of the index
  update-index: remove stale fallback code for "--unresolve"
  update-index: use unmerge_index_entry() to support removal
  resolve-undo: allow resurrecting conflicted state that resolved to deletion
  update-index: do not read HEAD and MERGE_HEAD unconditionally

Documentation/git-checkout.txt

@@ -12,8 +12,10 @@ SYNOPSIS
 'git checkout' [-q] [-f] [-m] --detach [<branch>]
 'git checkout' [-q] [-f] [-m] [--detach] <commit>
 'git checkout' [-q] [-f] [-m] [[-b|-B|--orphan] <new-branch>] [<start-point>]
-'git checkout' [-f|--ours|--theirs|-m|--conflict=<style>] [<tree-ish>] [--] <pathspec>...
-'git checkout' [-f|--ours|--theirs|-m|--conflict=<style>] [<tree-ish>] --pathspec-from-file=<file> [--pathspec-file-nul]
+'git checkout' [-f] <tree-ish> [--] <pathspec>...
+'git checkout' [-f] <tree-ish> --pathspec-from-file=<file> [--pathspec-file-nul]
+'git checkout' [-f|--ours|--theirs|-m|--conflict=<style>] [--] <pathspec>...
+'git checkout' [-f|--ours|--theirs|-m|--conflict=<style>] --pathspec-from-file=<file> [--pathspec-file-nul]
 'git checkout' (-p|--patch) [<tree-ish>] [--] [<pathspec>...]
 
 DESCRIPTION
@@ -260,7 +262,8 @@ and mark the resolved paths with `git add` (or `git rm` if the merge
 should result in deletion of the path).
 +
 When checking out paths from the index, this option lets you recreate
-the conflicted merge in the specified paths.
+the conflicted merge in the specified paths.  This option cannot be
+used when checking out paths from a tree-ish.
 +
 When switching branches with `--merge`, staged changes may be lost.
 

Documentation/git-restore.txt

@@ -78,6 +78,8 @@ all modified paths.
 --theirs::
 	When restoring files in the working tree from the index, use
 	stage #2 ('ours') or #3 ('theirs') for unmerged paths.
+	This option cannot be used when checking out paths from a
+	tree-ish (i.e. with the `--source` option).
 +
 Note that during `git rebase` and `git pull --rebase`, 'ours' and
 'theirs' may appear swapped. See the explanation of the same options
@@ -87,6 +89,8 @@ in linkgit:git-checkout[1] for details.
 --merge::
 	When restoring files on the working tree from the index,
 	recreate the conflicted merge in the unmerged paths.
+	This option cannot be used when checking out paths from a
+	tree-ish (i.e. with the `--source` option).
 
 --conflict=<style>::
 	The same as `--merge` option above, but changes the way the

builtin/checkout.c

@@ -523,6 +523,15 @@ static int checkout_paths(const struct checkout_opts *opts,
 			    "--merge", "--conflict", "--staged");
 	}
 
+	/*
+	 * recreating unmerged index entries and writing out data from
+	 * unmerged index entries would make no sense when checking out
+	 * of a tree-ish.
+	 */
+	if ((opts->merge || opts->writeout_stage) && opts->source_tree)
+		die(_("'%s', '%s', or '%s' cannot be used when checking out of a tree"),
+		    "--merge", "--ours", "--theirs");
+
 	if (opts->patch_mode) {
 		enum add_p_mode patch_mode;
 		const char *rev = new_branch_info->name;
@@ -560,6 +569,8 @@ static int checkout_paths(const struct checkout_opts *opts,
 
 	if (opts->source_tree)
 		read_tree_some(opts->source_tree, &opts->pathspec);
+	if (opts->merge)
+		unmerge_index(&the_index, &opts->pathspec, CE_MATCHED);
 
 	ps_matched = xcalloc(opts->pathspec.nr, 1);
 
@@ -583,10 +594,6 @@ static int checkout_paths(const struct checkout_opts *opts,
 	}
 	free(ps_matched);
 
-	/* "checkout -m path" to recreate conflicted state */
-	if (opts->merge)
-		unmerge_marked_index(&the_index);
-
 	/* Any unmerged paths? */
 	for (pos = 0; pos < the_index.cache_nr; pos++) {
 		const struct cache_entry *ce = the_index.cache[pos];

builtin/update-index.c

@@ -609,9 +609,6 @@ static const char * const update_index_usage[] = {
 	NULL
 };
 
-static struct object_id head_oid;
-static struct object_id merge_head_oid;
-
 static struct cache_entry *read_one_ent(const char *which,
 					struct object_id *ent, const char *path,
 					int namelen, int stage)
@@ -642,84 +639,17 @@ static struct cache_entry *read_one_ent(const char *which,
 
 static int unresolve_one(const char *path)
 {
-	int namelen = strlen(path);
-	int pos;
-	int ret = 0;
-	struct cache_entry *ce_2 = NULL, *ce_3 = NULL;
+	struct string_list_item *item;
+	int res = 0;
 
-	/* See if there is such entry in the index. */
-	pos = index_name_pos(&the_index, path, namelen);
-	if (0 <= pos) {
-		/* already merged */
-		pos = unmerge_index_entry_at(&the_index, pos);
-		if (pos < the_index.cache_nr) {
-			const struct cache_entry *ce = the_index.cache[pos];
-			if (ce_stage(ce) &&
-			    ce_namelen(ce) == namelen &&
-			    !memcmp(ce->name, path, namelen))
-				return 0;
-		}
-		/* no resolve-undo information; fall back */
-	} else {
-		/* If there isn't, either it is unmerged, or
-		 * resolved as "removed" by mistake.  We do not
-		 * want to do anything in the former case.
-		 */
-		pos = -pos-1;
-		if (pos < the_index.cache_nr) {
-			const struct cache_entry *ce = the_index.cache[pos];
-			if (ce_namelen(ce) == namelen &&
-			    !memcmp(ce->name, path, namelen)) {
-				fprintf(stderr,
-					"%s: skipping still unmerged path.\n",
-					path);
-				goto free_return;
-			}
-		}
-	}
-
-	/* Grab blobs from given path from HEAD and MERGE_HEAD,
-	 * stuff HEAD version in stage #2,
-	 * stuff MERGE_HEAD version in stage #3.
-	 */
-	ce_2 = read_one_ent("our", &head_oid, path, namelen, 2);
-	ce_3 = read_one_ent("their", &merge_head_oid, path, namelen, 3);
-
-	if (!ce_2 || !ce_3) {
-		ret = -1;
-		goto free_return;
-	}
-	if (oideq(&ce_2->oid, &ce_3->oid) &&
-	    ce_2->ce_mode == ce_3->ce_mode) {
-		fprintf(stderr, "%s: identical in both, skipping.\n",
-			path);
-		goto free_return;
-	}
-
-	remove_file_from_index(&the_index, path);
-	if (add_index_entry(&the_index, ce_2, ADD_CACHE_OK_TO_ADD)) {
-		error("%s: cannot add our version to the index.", path);
-		ret = -1;
-		goto free_return;
-	}
-	if (!add_index_entry(&the_index, ce_3, ADD_CACHE_OK_TO_ADD))
-		return 0;
-	error("%s: cannot add their version to the index.", path);
-	ret = -1;
- free_return:
-	discard_cache_entry(ce_2);
-	discard_cache_entry(ce_3);
-	return ret;
-}
-
-static void read_head_pointers(void)
-{
-	if (read_ref("HEAD", &head_oid))
-		die("No HEAD -- no initial commit yet?");
-	if (read_ref("MERGE_HEAD", &merge_head_oid)) {
-		fprintf(stderr, "Not in the middle of a merge.\n");
-		exit(0);
-	}
+	if (!the_index.resolve_undo)
+		return res;
+	item = string_list_lookup(the_index.resolve_undo, path);
+	if (!item)
+		return res; /* no resolve-undo record for the path */
+	res = unmerge_index_entry(&the_index, path, item->util, 0);
+	FREE_AND_NULL(item->util);
+	return res;
 }
 
 static int do_unresolve(int ac, const char **av,
@@ -728,11 +658,6 @@ static int do_unresolve(int ac, const char **av,
 	int i;
 	int err = 0;
 
-	/* Read HEAD and MERGE_HEAD; if MERGE_HEAD does not exist, we
-	 * are not doing a merge, so exit with success status.
-	 */
-	read_head_pointers();
-
 	for (i = 1; i < ac; i++) {
 		const char *arg = av[i];
 		char *p = prefix_path(prefix, prefix_length, arg);
@@ -751,6 +676,7 @@ static int do_reupdate(const char **paths,
 	int pos;
 	int has_head = 1;
 	struct pathspec pathspec;
+	struct object_id head_oid;
 
 	parse_pathspec(&pathspec, 0,
 		       PATHSPEC_PREFER_CWD,

rerere.c

@@ -1112,7 +1112,7 @@ int rerere_forget(struct repository *r, struct pathspec *pathspec)
 	 * recover the original conflicted state and then
 	 * find the conflicted paths.
 	 */
-	unmerge_index(r->index, pathspec);
+	unmerge_index(r->index, pathspec, 0);
 	find_conflict(r, &conflict);
 	for (i = 0; i < conflict.nr; i++) {
 		struct string_list_item *it = &conflict.items[i];

resolve-undo.c

@@ -117,86 +117,59 @@ void resolve_undo_clear_index(struct index_state *istate)
 	istate->cache_changed |= RESOLVE_UNDO_CHANGED;
 }
 
-int unmerge_index_entry_at(struct index_state *istate, int pos)
+int unmerge_index_entry(struct index_state *istate, const char *path,
+			struct resolve_undo_info *ru, unsigned ce_flags)
 {
-	const struct cache_entry *ce;
-	struct string_list_item *item;
-	struct resolve_undo_info *ru;
-	int i, err = 0, matched;
-	char *name;
+	int i = index_name_pos(istate, path, strlen(path));
 
-	if (!istate->resolve_undo)
-		return pos;
-
-	ce = istate->cache[pos];
-	if (ce_stage(ce)) {
-		/* already unmerged */
-		while ((pos < istate->cache_nr) &&
-		       ! strcmp(istate->cache[pos]->name, ce->name))
-			pos++;
-		return pos - 1; /* return the last entry processed */
+	if (i < 0) {
+		/* unmerged? */
+		i = -i - 1;
+		if (i < istate->cache_nr &&
+		    !strcmp(istate->cache[i]->name, path))
+			/* yes, it is already unmerged */
+			return 0;
+		/* fallthru: resolved to removal */
+	} else {
+		/* merged - remove it to replace it with unmerged entries */
+		remove_index_entry_at(istate, i);
 	}
-	item = string_list_lookup(istate->resolve_undo, ce->name);
-	if (!item)
-		return pos;
-	ru = item->util;
-	if (!ru)
-		return pos;
-	matched = ce->ce_flags & CE_MATCHED;
-	name = xstrdup(ce->name);
-	remove_index_entry_at(istate, pos);
+
 	for (i = 0; i < 3; i++) {
-		struct cache_entry *nce;
+		struct cache_entry *ce;
 		if (!ru->mode[i])
 			continue;
-		nce = make_cache_entry(istate,
-				       ru->mode[i],
-				       &ru->oid[i],
-				       name, i + 1, 0);
-		if (matched)
-			nce->ce_flags |= CE_MATCHED;
-		if (add_index_entry(istate, nce, ADD_CACHE_OK_TO_ADD)) {
-			err = 1;
-			error("cannot unmerge '%s'", name);
+		ce = make_cache_entry(istate, ru->mode[i], &ru->oid[i],
+				      path, i + 1, 0);
+		ce->ce_flags |= ce_flags;
+		if (add_index_entry(istate, ce, ADD_CACHE_OK_TO_ADD))
+			return error("cannot unmerge '%s'", path);
 	}
-	}
-	free(name);
-	if (err)
-		return pos;
+	return 0;
+}
+
+void unmerge_index(struct index_state *istate, const struct pathspec *pathspec,
+		   unsigned ce_flags)
+{
+	struct string_list_item *item;
+
+	if (!istate->resolve_undo)
+		return;
+
+	/* TODO: audit for interaction with sparse-index. */
+	ensure_full_index(istate);
+
+	for_each_string_list_item(item, istate->resolve_undo) {
+		const char *path = item->string;
+		struct resolve_undo_info *ru = item->util;
+		if (!item->util)
+			continue;
+		if (!match_pathspec(istate, pathspec,
+				    item->string, strlen(item->string),
+				    0, NULL, 0))
+			continue;
+		unmerge_index_entry(istate, path, ru, ce_flags);
 		free(ru);
 		item->util = NULL;
-	return unmerge_index_entry_at(istate, pos);
-}
-
-void unmerge_marked_index(struct index_state *istate)
-{
-	int i;
-
-	if (!istate->resolve_undo)
-		return;
-
-	/* TODO: audit for interaction with sparse-index. */
-	ensure_full_index(istate);
-	for (i = 0; i < istate->cache_nr; i++) {
-		const struct cache_entry *ce = istate->cache[i];
-		if (ce->ce_flags & CE_MATCHED)
-			i = unmerge_index_entry_at(istate, i);
-	}
-}
-
-void unmerge_index(struct index_state *istate, const struct pathspec *pathspec)
-{
-	int i;
-
-	if (!istate->resolve_undo)
-		return;
-
-	/* TODO: audit for interaction with sparse-index. */
-	ensure_full_index(istate);
-	for (i = 0; i < istate->cache_nr; i++) {
-		const struct cache_entry *ce = istate->cache[i];
-		if (!ce_path_match(istate, ce, pathspec, NULL))
-			continue;
-		i = unmerge_index_entry_at(istate, i);
 	}
 }

resolve-undo.h

@@ -17,8 +17,7 @@ void record_resolve_undo(struct index_state *, struct cache_entry *);
 void resolve_undo_write(struct strbuf *, struct string_list *);
 struct string_list *resolve_undo_read(const char *, unsigned long);
 void resolve_undo_clear_index(struct index_state *);
-int unmerge_index_entry_at(struct index_state *, int);
-void unmerge_index(struct index_state *, const struct pathspec *);
-void unmerge_marked_index(struct index_state *);
+int unmerge_index_entry(struct index_state *, const char *, struct resolve_undo_info *, unsigned);
+void unmerge_index(struct index_state *, const struct pathspec *, unsigned);
 
 #endif

t/t2030-unresolve-info.sh

@@ -37,11 +37,17 @@ prime_resolve_undo () {
 	git checkout second^0 &&
 	test_tick &&
 	test_must_fail git merge third^0 &&
-	echo merge does not leave anything &&
 	check_resolve_undo empty &&
-	echo different >fi/le &&
-	git add fi/le &&
-	echo resolving records &&
+
+	# how should the conflict be resolved?
+	case "$1" in
+	remove)
+		rm -f file/le && git rm fi/le
+		;;
+	*) # modify
+		echo different >fi/le && git add fi/le
+		;;
+	esac
 	check_resolve_undo recorded fi/le initial:fi/le second:fi/le third:fi/le
 }
 
@@ -122,6 +128,37 @@ test_expect_success 'add records checkout -m undoes' '
 test_expect_success 'unmerge with plumbing' '
 	prime_resolve_undo &&
 	git update-index --unresolve fi/le &&
+	git ls-files --resolve-undo fi/le >actual &&
+	test_must_be_empty actual &&
+	git ls-files -u >actual &&
+	test_line_count = 3 actual
+'
+
+test_expect_success 'unmerge can be done even after committing' '
+	prime_resolve_undo &&
+	git commit -m "record to nuke MERGE_HEAD" &&
+	git update-index --unresolve fi/le &&
+	git ls-files --resolve-undo fi/le >actual &&
+	test_must_be_empty actual &&
+	git ls-files -u >actual &&
+	test_line_count = 3 actual
+'
+
+test_expect_success 'unmerge removal' '
+	prime_resolve_undo remove &&
+	git update-index --unresolve fi/le &&
+	git ls-files --resolve-undo fi/le >actual &&
+	test_must_be_empty actual &&
+	git ls-files -u >actual &&
+	test_line_count = 3 actual
+'
+
+test_expect_success 'unmerge removal after committing' '
+	prime_resolve_undo remove &&
+	git commit -m "record to nuke MERGE_HEAD" &&
+	git update-index --unresolve fi/le &&
+	git ls-files --resolve-undo fi/le >actual &&
+	test_must_be_empty actual &&
 	git ls-files -u >actual &&
 	test_line_count = 3 actual
 '

t/t2070-restore.sh

@@ -137,11 +137,78 @@ test_expect_success 'restore --staged invalidates cache tree for deletions' '
 	test_must_fail git rev-parse HEAD:new1
 '
 
-test_expect_success 'restore with merge options rejects --staged' '
+test_expect_success 'restore --merge to unresolve' '
+	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 $O 1	file" &&
+		echo "100644 $A 2	file" &&
+		echo "100644 $B 3	file"
+	} | git update-index --index-info &&
+	echo nothing >file &&
+	git restore --worktree --merge file &&
+	cat >expect <<-\EOF &&
+	<<<<<<< ours
+	ourside
+	=======
+	theirside
+	>>>>>>> theirs
+	EOF
+	test_cmp expect file
+'
+
+test_expect_success 'restore --merge to unresolve after (mistaken) resolution' '
+	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 $O 1	file" &&
+		echo "100644 $A 2	file" &&
+		echo "100644 $B 3	file"
+	} | git update-index --index-info &&
+	echo nothing >file &&
+	git add file &&
+	git restore --worktree --merge file &&
+	cat >expect <<-\EOF &&
+	<<<<<<< ours
+	ourside
+	=======
+	theirside
+	>>>>>>> theirs
+	EOF
+	test_cmp expect file
+'
+
+test_expect_success 'restore --merge to unresolve after (mistaken) resolution' '
+	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 $O 1	file" &&
+		echo "100644 $A 2	file" &&
+		echo "100644 $B 3	file"
+	} | git update-index --index-info &&
+	git rm -f file &&
+	git restore --worktree --merge file &&
+	cat >expect <<-\EOF &&
+	<<<<<<< ours
+	ourside
+	=======
+	theirside
+	>>>>>>> theirs
+	EOF
+	test_cmp expect file
+'
+
+test_expect_success 'restore with merge options are incompatible with certain options' '
 	for opts in \
 		"--staged --ours" \
 		"--staged --theirs" \
 		"--staged --merge" \
+		"--source=HEAD --ours" \
+		"--source=HEAD --theirs" \
+		"--source=HEAD --merge" \
 		"--staged --conflict=diff3" \
 		"--staged --worktree --ours" \
 		"--staged --worktree --theirs" \
@@ -149,7 +216,7 @@ test_expect_success 'restore with merge options rejects --staged' '
 		"--staged --worktree --conflict=zdiff3"
 	do
 		test_must_fail git restore $opts . 2>err &&
-		grep "cannot be used with --staged" err || return
+		grep "cannot be used" err || return
 	done
 '
 

t/t7201-co.sh

@@ -497,6 +497,11 @@ test_expect_success 'checkout unmerged stage' '
 	test ztheirside = "z$(cat file)"
 '
 
+test_expect_success 'checkout path with --merge from tree-ish is a no-no' '
+	setup_conflicting_index &&
+	test_must_fail git checkout -m HEAD -- file
+'
+
 test_expect_success 'checkout with --merge' '
 	setup_conflicting_index &&
 	echo "none of the above" >sample &&
@@ -517,6 +522,48 @@ test_expect_success 'checkout with --merge' '
 	test_cmp merged file
 '
 
+test_expect_success 'checkout -m works after (mistaken) resolution' '
+	setup_conflicting_index &&
+	echo "none of the above" >sample &&
+	cat sample >fild &&
+	cat sample >file &&
+	cat sample >filf &&
+	# resolve to something
+	git add file &&
+	git checkout --merge -- fild file filf &&
+	{
+		echo "<<<<<<< ours" &&
+		echo ourside &&
+		echo "=======" &&
+		echo theirside &&
+		echo ">>>>>>> theirs"
+	} >merged &&
+	test_cmp expect fild &&
+	test_cmp expect filf &&
+	test_cmp merged file
+'
+
+test_expect_success 'checkout -m works after (mistaken) resolution to remove' '
+	setup_conflicting_index &&
+	echo "none of the above" >sample &&
+	cat sample >fild &&
+	cat sample >file &&
+	cat sample >filf &&
+	# resolve to remove
+	git rm file &&
+	git checkout --merge -- fild file filf &&
+	{
+		echo "<<<<<<< ours" &&
+		echo ourside &&
+		echo "=======" &&
+		echo theirside &&
+		echo ">>>>>>> theirs"
+	} >merged &&
+	test_cmp expect fild &&
+	test_cmp expect filf &&
+	test_cmp merged file
+'
+
 test_expect_success 'checkout with --merge, in diff3 -m style' '
 	git config merge.conflictstyle diff3 &&
 	setup_conflicting_index &&