jc/rerere-multi (was: What's cooking in git.git (Jan 2016, #04; Wed, 20))

From: Johannes Sixt <j6t@kdbg.org>
Date: Jan 21, 2016, 12:09 UTC
Message-ID: <56A0CA5D.8080407@kdbg.org>
In-Reply-To: <xmqqk2n33jxq.fsf@gitster.mtv.corp.google.com>

Am 21.01.2016 um 00:33 schrieb Junio C Hamano:

Show 14 quoted lines

> * jc/rerere-multi (2015-09-14) 7 commits
>   - rerere: do use multiple variants
>   - t4200: rerere a merge with two identical conflicts
>   - rerere: allow multiple variants to exist
>   - rerere: delay the recording of preimage
>   - rerere: handle leftover rr-cache/$ID directory and postimage files
>   - rerere: scan $GIT_DIR/rr-cache/$ID when instantiating a rerere_id
>   - rerere: split conflict ID further
> 
>   "git rerere" can encounter two or more files with the same conflict
>   signature that have to be resolved in different ways, but there was
>   no way to record these separate resolutions.
> 
>   Needs review.

I finally found some time to test and review this series. I have one case where there are many identical conflicts (up to 15!) that rerere was unable to resolve. But with this series applied, all of them are now resolved automatically and correctly. That's a nice achievement!

Tested-by: Johannes Sixt <j6t@kdbg.org>

I don't have the original submission anymore. So, I'm responding here.

Generally, the patches make sense.

Except for 510936082eb4 "handle leftover rr-cache/$ID directory and postimage files": After the subsequent e2a6344cca47 "delay the recording of preimage" is in place, nothing of what the former patch changed (except test cases) remains, and the problem that the former solved is still solved, and in addition the NEEDSWORK that the former introduced is resolved by the latter. I think the two should be squashed together.

e2a6344cca47 (rerere: delay the recording of preimage) needs this fixup, I think:

diff --git a/rerere.c b/rerere.c
index c0482b8..33b1348 100644
--- a/rerere.c
+++ b/rerere.c
@@ -765,7 +765,7 @@ static void do_rerere_one_path(struct string_list_item *rr_item,
 			const char *path = rerere_path(id, "postimage");
 			if (unlink(path))
 				die_errno("cannot unlink stray '%s'", path);
-			id->collection->status &= ~RR_HAS_PREIMAGE;
+			id->collection->status &= ~RR_HAS_POSTIMAGE;
 		}
 		id->collection->status |= RR_HAS_PREIMAGE;
 		fprintf(stderr, "Recorded preimage for '%s'\n", path);

and perhaps this change:

diff --git a/rerere.c b/rerere.c
index fbdade8..df6beb9 100644
--- a/rerere.c
+++ b/rerere.c
@@ -1005,11 +1005,6 @@ static void unlink_rr_item(struct rerere_id *id)
 	unlink(rerere_path(id, "thisimage"));
 	unlink(rerere_path(id, "preimage"));
 	unlink(rerere_path(id, "postimage"));
-	/*
-	 * NEEDSWORK: what if this rmdir() fails?  Wouldn't we then
-	 * assume that we already have preimage recorded in
-	 * do_plain_rerere()?
-	 */
 	rmdir(rerere_path(id, NULL));
 }