diff mbox series

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

Message ID 20230731224409.4181277-4-gitster@pobox.com (mailing list archive)
State Accepted
Commit 35901f1c2461640019e70bbefecdce1f59f660d2
Headers show
Series unresolve removal | expand

Commit Message

Junio C Hamano July 31, 2023, 10:44 p.m. UTC
"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(-)

Comments

Junio C Hamano July 31, 2023, 11:14 p.m. UTC | #1
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
>  '
diff mbox series

Patch

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
 '