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

[Johannes Sixt <j6t@kdbg.org>]

Am 21.01.2016 um 00:33 schrieb Junio C Hamano:
> * 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));
 }