[PATCH 0/7] unresolve removal

[PATCH 0/7] unresolve removal

[Junio C Hamano]

This series is a culmination of various draft patches I have sent
out recently and have queued in two topics near the tip of 'seen'.

The primary objective is to allow undoing conflict resolution that
was done via "git rm".  When a conflicted path is resolved with "git
add" (i.e. replace higher stage index entries with stage #0 entry),
"git checkout -m -- path" and "git update-index --unresolve path"
worked using the resolve-undo information stored in the index, but a
conflicted path that is resolved with "git rm" (i.e. remove higher
stage index, without adding any stage #0 entry for the path), these
two commands did not work at all, or did not function correctly.


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

 Documentation/git-checkout.txt |   9 ++-
 Documentation/git-restore.txt  |   4 ++
 builtin/checkout.c             |  15 +++--
 builtin/update-index.c         |  98 ++++----------------------------
 rerere.c                       |   2 +-
 resolve-undo.c                 | 101 ++++++++++++---------------------
 resolve-undo.h                 |   5 +-
 t/t2030-unresolve-info.sh      |  45 +++++++++++++--
 t/t2070-restore.sh             |  71 ++++++++++++++++++++++-
 t/t7201-co.sh                  |  47 +++++++++++++++
 10 files changed, 230 insertions(+), 167 deletions(-)

-- 
2.41.0-478-gee48e70a82

[PATCH 1/7] update-index: do not read HEAD and MERGE_HEAD unconditionally

[Junio C Hamano]

When "update-index --unresolve $path" cannot find the resolve-undo
record for the path the user requested to unresolve, it stuffs the
blobs from HEAD and MERGE_HEAD to stage #2 and stage #3 as a
fallback.  For this reason, the operation does not even start unless
both "HEAD" and "MERGE_HEAD" exist.

This is suboptimal in a few ways:

 * It does not recreate stage #1.  Even though it is a correct
   design decision not to do so (because it is impossible to
   recreate in general cases, without knowing how we got there,
   including what merge strategy was used), it is much less useful
   not to have that information in the index.

 * It limits the "unresolve" operation only during a conflicted "git
   merge" and nothing else.  Other operations like "rebase",
   "cherry-pick", and "switch -m" may result in conflicts, and the
   user may want to unresolve the conflict that they incorrectly
   resolved in order to redo the resolution, but the fallback would
   not kick in.

 * Most importantly, the entire "unresolve" operation is disabled
   after a conflicted merge is committed and MERGE_HEAD is removed,
   even though the index has perfectly usable resolve-undo records.

By lazily reading the HEAD and MERGE_HEAD only when we need to go to
the fallback codepath, we will allow cases where resolve-undo
records are available (which is 100% of the time, unless the user is
reading from an index file created by Git more than 10 years ago) to
proceed even after a conflicted merge was committed, during other
mergy operations that do not use MERGE_HEAD, or after the result of
such mergy operations has been committed.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/update-index.c    | 46 ++++++++++++++++++++++++---------------
 t/t2030-unresolve-info.sh |  8 +++++++
 2 files changed, 36 insertions(+), 18 deletions(-)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index 5fab9ad2ec..47cd68e9d5 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -639,6 +639,21 @@ static struct cache_entry *read_one_ent(const char *which,
 	return ce;
 }
 
+static int read_head_pointers(void)
+{
+	static int result = -2; /* unknown yet */
+
+	if (result == -2) {
+		result = -1;
+		if (read_ref("HEAD", &head_oid))
+			return error("No HEAD -- no initial commit yet?");
+		if (read_ref("MERGE_HEAD", &merge_head_oid))
+			return error("Not in the middle of a merge");
+		result = 0;
+	}
+	return result;
+}
+
 static int unresolve_one(const char *path)
 {
 	int namelen = strlen(path);
@@ -677,10 +692,20 @@ static int unresolve_one(const char *path)
 		}
 	}
 
-	/* Grab blobs from given path from HEAD and MERGE_HEAD,
-	 * stuff HEAD version in stage #2,
-	 * stuff MERGE_HEAD version in stage #3.
+	/*
+	 * We are not using resolve-undo information but just
+	 * populating the stages #2 and #3 from HEAD and MERGE_HEAD.
+	 *
+	 * This is a flawed replacement of true "unresolve", as we do
+	 * not have a way to recreate the stage #1 for the common
+	 * ancestor (which may not be a unique merge-base between the
+	 * two).
 	 */
+	if (read_head_pointers()) {
+		ret = -1;
+		goto free_return;
+	}
+
 	ce_2 = read_one_ent("our", &head_oid, path, namelen, 2);
 	ce_3 = read_one_ent("their", &merge_head_oid, path, namelen, 3);
 
@@ -711,27 +736,12 @@ static int unresolve_one(const char *path)
 	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);
-	}
-}
-
 static int do_unresolve(int ac, const char **av,
 			const char *prefix, int prefix_length)
 {
 	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);
diff --git a/t/t2030-unresolve-info.sh b/t/t2030-unresolve-info.sh
index 2d8c70b03a..d4e7760df5 100755
--- a/t/t2030-unresolve-info.sh
+++ b/t/t2030-unresolve-info.sh
@@ -126,6 +126,14 @@ test_expect_success 'unmerge with plumbing' '
 	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 -u >actual &&
+	test_line_count = 3 actual
+'
+
 test_expect_success 'rerere and rerere forget' '
 	mkdir .git/rr-cache &&
 	prime_resolve_undo &&
-- 
2.41.0-478-gee48e70a82

[PATCH 2/7] resolve-undo: allow resurrecting conflicted state that resolved to deletion

[Junio C Hamano]

The resolve-undo index extension records up to three (mode, object
name) tuples for non-zero stages for each path that was resolved,
to be used to recreate the original conflicted state later when the
user requests.

The unmerge_index_entry_at() function uses the resolve-undo data to
do so, but it assumes that the path for which the conflicted state
needs to be recreated can be specified by the position in the
active_cache[] array.  This obviously cannot salvage the state of
conflicted paths that were resolved by removing them.  For example,
a delete-modify conflict, in which the change whose "modify" side
made is a trivial typofix, may legitimately be resolved to remove
the path, and resolve-undo extension does record the two (mode,
object name) tuples for the common ancestor version and their
version, lacking our version.  But after recording such a removal of
the path, you should be able to use resolve-undo data to recreate
the conflicted state.

Introduce a new unmerge_index_entry() helper function that takes the
path (which does not necessarily have to exist in the active_cache[]
array) and resolve-undo data, and use it to reimplement unmerge_index()
public function that is used by "git rerere".

The limited interface is still kept for now, as it is used by "git
checkout -m" and "git update-index --unmerge", but these two codepaths
will be updated to lift the assumption to allow conflicts that resolved
to deletion can be recreated.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 resolve-undo.c | 48 +++++++++++++++++++++++++++++++++++++++++++-----
 resolve-undo.h |  1 +
 2 files changed, 44 insertions(+), 5 deletions(-)

diff --git a/resolve-undo.c b/resolve-undo.c
index 70a6db526d..3b0244e210 100644
--- a/resolve-undo.c
+++ b/resolve-undo.c
@@ -182,19 +182,57 @@ void unmerge_marked_index(struct index_state *istate)
 	}
 }
 
+int unmerge_index_entry(struct index_state *istate, const char *path,
+			struct resolve_undo_info *ru)
+{
+	int i = index_name_pos(istate, path, strlen(path));
+
+	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);
+	}
+
+	for (i = 0; i < 3; i++) {
+		struct cache_entry *ce;
+		if (!ru->mode[i])
+			continue;
+		ce = make_cache_entry(istate, ru->mode[i], &ru->oid[i],
+				      path, i + 1, 0);
+		if (add_index_entry(istate, ce, ADD_CACHE_OK_TO_ADD))
+			return error("cannot unmerge '%s'", path);
+	}
+	return 0;
+}
+
 void unmerge_index(struct index_state *istate, const struct pathspec *pathspec)
 {
-	int i;
+	struct string_list_item *item;
 
 	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))
+
+	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;
-		i = unmerge_index_entry_at(istate, i);
+		unmerge_index_entry(istate, path, ru);
+		free(ru);
+		item->util = NULL;
 	}
 }
diff --git a/resolve-undo.h b/resolve-undo.h
index c5deafc92f..1ae321c88b 100644
--- a/resolve-undo.h
+++ b/resolve-undo.h
@@ -18,6 +18,7 @@ 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);
+int unmerge_index_entry(struct index_state *, const char *, struct resolve_undo_info *);
 void unmerge_index(struct index_state *, const struct pathspec *);
 void unmerge_marked_index(struct index_state *);
 
-- 
2.41.0-478-gee48e70a82

[PATCH 3/7] update-index: use unmerge_index_entry() to support removal

[Junio C Hamano]

"update-index --unresolve" uses the unmerge_index_entry_at() that
assumes that the path to be unresolved must be in the index, which
makes it impossible to unresolve a path that was resolved as removal.

Rewrite unresolve_one() to use the unmerge_index_entry() to support
unresolving such a path.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/update-index.c    | 38 ++++++++++++++++++++++----------------
 t/t2030-unresolve-info.sh | 37 +++++++++++++++++++++++++++++++++----
 2 files changed, 55 insertions(+), 20 deletions(-)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index 47cd68e9d5..ecd1c0c2d3 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -660,26 +660,31 @@ static int unresolve_one(const char *path)
 	int pos;
 	int ret = 0;
 	struct cache_entry *ce_2 = NULL, *ce_3 = NULL;
+	struct resolve_undo_info *ru = NULL;
+
+	if (the_index.resolve_undo) {
+		struct string_list_item *item;
+		item = string_list_lookup(the_index.resolve_undo, path);
+		if (item) {
+			ru = item->util;
+			item->util = NULL;
+		}
+	}
+
+	/* resolve-undo record exists for the path */
+	if (ru) {
+		ret = unmerge_index_entry(&the_index, path, ru);
+		free(ru);
+		return ret;
+	}
 
 	/* 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 */
+		; /* resolve-undo record was used already -- 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;
+		/* Is it unmerged? */
+		pos = -pos - 1;
 		if (pos < the_index.cache_nr) {
 			const struct cache_entry *ce = the_index.cache[pos];
 			if (ce_namelen(ce) == namelen &&
@@ -687,9 +692,10 @@ static int unresolve_one(const char *path)
 				fprintf(stderr,
 					"%s: skipping still unmerged path.\n",
 					path);
-				goto free_return;
 			}
+			goto free_return;
 		}
+		/* No, such a path does not exist -- removed */
 	}
 
 	/*
diff --git a/t/t2030-unresolve-info.sh b/t/t2030-unresolve-info.sh
index d4e7760df5..3eda385ca2 100755
--- a/t/t2030-unresolve-info.sh
+++ b/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,8 @@ 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
 '
@@ -130,6 +138,27 @@ 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
 '
-- 
2.41.0-478-gee48e70a82

Re: [PATCH 3/7] update-index: use unmerge_index_entry() to support removal

[Junio C Hamano]

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

> @@ -122,6 +128,8 @@ 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
>  '

This addition, and matching invocations of "ls-files --resolve-undo"
in the next tests, are not explained in the proposed log message.

The reason for the addition is because "update-index --unresolve"
tests did not make sure that resolve-undo records that are used to
recreate conflicted states are removed from the index, like simiar
tests for "checkout --merge".  As we are changing the implementation
to unmerge index entries, we do not want to regress and these are
filling the gap of the test coverage.

I've clarified it in the draft for the next iteration.

	Side note: careful audience may have noticed this already,
	but yes, this is setting an example of the recent update to
	"my patch was sent, now what?" document, where we discourage
	sending rerolls without giving readers enough time to digest
	the first iteration, and instead tell them to note what will
	change in their next iteration.

> @@ -130,6 +138,27 @@ 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
>  '

[PATCH 4/7] update-index: remove stale fallback code for "--unresolve"

[Junio C Hamano]

The "update-index --unresolve" is a relatively old feature that was
introduced in Git v1.4.1 (June 2006), which predates the
resolve-undo extension introduced in Git v1.7.0 (February 2010).
The original code that was limited only to work during a merge (and
not during a rebase or a cherry-pick) has been kept as the fallback
codepath to be used as a transition measure.

By now, for more than 10 years we have stored resolve-undo extension
in the index file, and the fallback code way outlived its usefulness.

Remove it, together with two file-scope static global variables.
One of these variables is still used by surviving function, but it
does not have to be a global at all, so move it to local to that
function.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/update-index.c | 114 +++++------------------------------------
 1 file changed, 12 insertions(+), 102 deletions(-)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index ecd1c0c2d3..def7f98504 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -608,9 +608,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)
@@ -639,107 +636,19 @@ static struct cache_entry *read_one_ent(const char *which,
 	return ce;
 }
 
-static int read_head_pointers(void)
-{
-	static int result = -2; /* unknown yet */
-
-	if (result == -2) {
-		result = -1;
-		if (read_ref("HEAD", &head_oid))
-			return error("No HEAD -- no initial commit yet?");
-		if (read_ref("MERGE_HEAD", &merge_head_oid))
-			return error("Not in the middle of a merge");
-		result = 0;
-	}
-	return result;
-}
-
 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 resolve_undo_info *ru = NULL;
-
-	if (the_index.resolve_undo) {
-		struct string_list_item *item;
-		item = string_list_lookup(the_index.resolve_undo, path);
-		if (item) {
-			ru = item->util;
-			item->util = NULL;
-		}
-	}
-
-	/* resolve-undo record exists for the path */
-	if (ru) {
-		ret = unmerge_index_entry(&the_index, path, ru);
-		free(ru);
-		return ret;
-	}
-
-	/* See if there is such entry in the index. */
-	pos = index_name_pos(&the_index, path, namelen);
-	if (0 <= pos) {
-		; /* resolve-undo record was used already -- fall back */
-	} else {
-		/* Is it unmerged? */
-		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;
-		}
-		/* No, such a path does not exist -- removed */
-	}
-
-	/*
-	 * We are not using resolve-undo information but just
-	 * populating the stages #2 and #3 from HEAD and MERGE_HEAD.
-	 *
-	 * This is a flawed replacement of true "unresolve", as we do
-	 * not have a way to recreate the stage #1 for the common
-	 * ancestor (which may not be a unique merge-base between the
-	 * two).
-	 */
-	if (read_head_pointers()) {
-		ret = -1;
-		goto free_return;
-	}
-
-	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;
+	struct string_list_item *item;
+	int res = 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);
+	FREE_AND_NULL(item->util);
+	return res;
 }
 
 static int do_unresolve(int ac, const char **av,
@@ -766,6 +675,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,
-- 
2.41.0-478-gee48e70a82

[PATCH 5/7] checkout/restore: refuse unmerging paths unless checking out of the index

[Junio C Hamano]

Recreating unmerged index entries using resolve-undo data,
recreating conflicted working tree files using unmerged index
entries, and writing data out of unmerged index entries, make
sense only when we are checking paths out of the index and not when
we are checking paths out of a tree-ish.

Add an extra check to make sure "--merge" and "--ours/--theirs"
options are rejected when checking out from a tree-ish, update the
document (especially the SYNOPSIS section) to highlight that they
are incompatible, and add a few tests to make sure the combination
fails.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-checkout.txt | 9 ++++++---
 Documentation/git-restore.txt  | 4 ++++
 builtin/checkout.c             | 9 +++++++++
 t/t2070-restore.sh             | 7 +++++--
 t/t7201-co.sh                  | 5 +++++
 5 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index 4af0904f47..a30e3ebc51 100644
--- a/Documentation/git-checkout.txt
+++ b/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.
 
diff --git a/Documentation/git-restore.txt b/Documentation/git-restore.txt
index 5964810caa..c70444705b 100644
--- a/Documentation/git-restore.txt
+++ b/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
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 715eeb5048..b8dfba57c6 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -520,6 +520,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;
diff --git a/t/t2070-restore.sh b/t/t2070-restore.sh
index c5d19dd973..fd775807e7 100755
--- a/t/t2070-restore.sh
+++ b/t/t2070-restore.sh
@@ -137,11 +137,14 @@ 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 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 +152,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
 '
 
diff --git a/t/t7201-co.sh b/t/t7201-co.sh
index 61ad47b0c1..23d4dadbcc 100755
--- a/t/t7201-co.sh
+++ b/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 &&
-- 
2.41.0-478-gee48e70a82

[PATCH 6/7] checkout/restore: add basic tests for --merge

[Junio C Hamano]

Even though "checkout --merge -- paths" had some tests, we never
made sure it worked to recreate the conflicted state _after_ the
resolution was recorded in the index.  Also "restore --merge" did
not even have any tests.

Currently these commands use the unmerge_marked_index() interface
that cannot handle paths that have been resolved as removal, and
tests for that case are marked with test_expect_failure; these
should eventually be fixed, but not in this patch.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t2070-restore.sh | 64 ++++++++++++++++++++++++++++++++++++++++++++++
 t/t7201-co.sh      | 42 ++++++++++++++++++++++++++++++
 2 files changed, 106 insertions(+)

diff --git a/t/t2070-restore.sh b/t/t2070-restore.sh
index fd775807e7..d97ecc2483 100755
--- a/t/t2070-restore.sh
+++ b/t/t2070-restore.sh
@@ -137,6 +137,70 @@ test_expect_success 'restore --staged invalidates cache tree for deletions' '
 	test_must_fail git rev-parse HEAD:new1
 '
 
+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_failure '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" \
diff --git a/t/t7201-co.sh b/t/t7201-co.sh
index 23d4dadbcc..4b07a26c14 100755
--- a/t/t7201-co.sh
+++ b/t/t7201-co.sh
@@ -522,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_failure '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 &&
-- 
2.41.0-478-gee48e70a82

[PATCH 7/7] checkout: allow "checkout -m path" to unmerge removed paths

[Junio C Hamano]

"git checkout -m -- path" uses the unmerge_marked_index() API, whose
implementation is incapable of unresolving a path that was resolved
as removed.  Extend the unmerge_index() API function so that we can
mark the ce_flags member of the cache entries we add to the index as
unmerged, and replace use of unmerge_marked_index() with it.

Now, together with its unmerge_index_entry_at() helper function,
unmerge_marked_index() function is no longer called by anybody, and
can safely be removed.

This makes two known test failures in t2070 and t7201 to succeed.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/checkout.c     |  6 ++--
 builtin/update-index.c |  2 +-
 rerere.c               |  2 +-
 resolve-undo.c         | 75 +++---------------------------------------
 resolve-undo.h         |  6 ++--
 t/t2070-restore.sh     |  2 +-
 t/t7201-co.sh          |  2 +-
 7 files changed, 13 insertions(+), 82 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index b8dfba57c6..98fcf1220a 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -566,6 +566,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);
 
@@ -589,10 +591,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];
diff --git a/builtin/update-index.c b/builtin/update-index.c
index def7f98504..69fe9c8fcb 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -646,7 +646,7 @@ static int unresolve_one(const char *path)
 	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);
+	res = unmerge_index_entry(&the_index, path, item->util, 0);
 	FREE_AND_NULL(item->util);
 	return res;
 }
diff --git a/rerere.c b/rerere.c
index e968d413d6..b525dd9230 100644
--- a/rerere.c
+++ b/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];
diff --git a/resolve-undo.c b/resolve-undo.c
index 3b0244e210..8e5a8072ed 100644
--- a/resolve-undo.c
+++ b/resolve-undo.c
@@ -115,75 +115,8 @@ 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)
-{
-	const struct cache_entry *ce;
-	struct string_list_item *item;
-	struct resolve_undo_info *ru;
-	int i, err = 0, matched;
-	char *name;
-
-	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 */
-	}
-	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;
-		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);
-		}
-	}
-	free(name);
-	if (err)
-		return pos;
-	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);
-	}
-}
-
 int unmerge_index_entry(struct index_state *istate, const char *path,
-			struct resolve_undo_info *ru)
+			struct resolve_undo_info *ru, unsigned ce_flags)
 {
 	int i = index_name_pos(istate, path, strlen(path));
 
@@ -206,13 +139,15 @@ int unmerge_index_entry(struct index_state *istate, const char *path,
 			continue;
 		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);
 	}
 	return 0;
 }
 
-void unmerge_index(struct index_state *istate, const struct pathspec *pathspec)
+void unmerge_index(struct index_state *istate, const struct pathspec *pathspec,
+		   unsigned ce_flags)
 {
 	struct string_list_item *item;
 
@@ -231,7 +166,7 @@ void unmerge_index(struct index_state *istate, const struct pathspec *pathspec)
 				    item->string, strlen(item->string),
 				    0, NULL, 0))
 			continue;
-		unmerge_index_entry(istate, path, ru);
+		unmerge_index_entry(istate, path, ru, ce_flags);
 		free(ru);
 		item->util = NULL;
 	}
diff --git a/resolve-undo.h b/resolve-undo.h
index 1ae321c88b..f3f8462751 100644
--- a/resolve-undo.h
+++ b/resolve-undo.h
@@ -17,9 +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);
-int unmerge_index_entry(struct index_state *, const char *, struct resolve_undo_info *);
-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
diff --git a/t/t2070-restore.sh b/t/t2070-restore.sh
index d97ecc2483..16d6348b69 100755
--- a/t/t2070-restore.sh
+++ b/t/t2070-restore.sh
@@ -180,7 +180,7 @@ test_expect_success 'restore --merge to unresolve after (mistaken) resolution' '
 	test_cmp expect file
 '
 
-test_expect_failure 'restore --merge to unresolve after (mistaken) resolution' '
+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) &&
diff --git a/t/t7201-co.sh b/t/t7201-co.sh
index 4b07a26c14..df582295df 100755
--- a/t/t7201-co.sh
+++ b/t/t7201-co.sh
@@ -543,7 +543,7 @@ test_expect_success 'checkout -m works after (mistaken) resolution' '
 	test_cmp merged file
 '
 
-test_expect_failure 'checkout -m works after (mistaken) resolution to remove' '
+test_expect_success 'checkout -m works after (mistaken) resolution to remove' '
 	setup_conflicting_index &&
 	echo "none of the above" >sample &&
 	cat sample >fild &&
-- 
2.41.0-478-gee48e70a82

Re: [PATCH 0/7] unresolve removal

[Taylor Blau]

On Mon, Jul 31, 2023 at 03:44:02PM -0700, Junio C Hamano wrote:
>  Documentation/git-checkout.txt |   9 ++-
>  Documentation/git-restore.txt  |   4 ++
>  builtin/checkout.c             |  15 +++--
>  builtin/update-index.c         |  98 ++++----------------------------
>  rerere.c                       |   2 +-
>  resolve-undo.c                 | 101 ++++++++++++---------------------
>  resolve-undo.h                 |   5 +-
>  t/t2030-unresolve-info.sh      |  45 +++++++++++++--
>  t/t2070-restore.sh             |  71 ++++++++++++++++++++++-
>  t/t7201-co.sh                  |  47 +++++++++++++++
>  10 files changed, 230 insertions(+), 167 deletions(-)

I am not so familiar with this area, so take my review with a healthy
grain of salt, but this all LGTM.

Thanks,
Taylor