Re: [PATCH] rerere: enable by default

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Mon, Jun 07 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> On Sun, Jun 06 2021, Junio C Hamano wrote:
>>
>>> By default, the rerere machinery has been disabled since a bug in
>>> the machinery could screw up the end user's data at the most
>>> stressful time during the end user's workday (i.e. during conflict
>>> resolution).
>>
>> What was that bug & in what commit was it fixed? Makes sense to note
>> that here.
>
> There is no such bug ;-).
>
> Writing buggy code, not thinking about it carefully enough and
> jumping up and down yelling this shiny new toy must be merged down
> immediately is something we see on this list from others, but it is
> the total opposite of how I operate.  I just am extra cautious and
> even after I am reasonably sure the code would not break, I prefer
> to have volunteers to opt into testing.

Thanks. I misread that, and was (perhaps mis-)remembering the previous
thread about this discussing some past bugs...

>> 	@@ -130,7 +129,6 @@ test_expect_success 'unmerge with plumbing' '
>> 	 test_expect_success 'rerere and rerere forget' '
>> 	 	# from here on, use rerere.
>> 	 	git config rerere.enabled true &&
>> 	-	mkdir .git/rr-cache &&
>> 	 	prime_resolve_undo &&
>> 	 	echo record the resolution &&
>> 	 	git rerere &&
>>
>> So the only impact of that rerere.enabled=false early is to make sure
>> we're not creating the .git/rr-cache.
>
> Not really.  Unresolve is about recording the initial conflict in
> the index, so it is far easier to see its effect if you do not
> enable rerere, when you are manually debugging these earlier tests.
>
> And later test do check how it works with rerere enabled, but the
> way the original sequence of tests enable it is with the "mkdir".
> I.e. "if rerere.enabled is not set either way, presence of the
> directory means it is already enabled".  The new test sequence
> uses the configuration variable explicitly, because in the new world
> order, the presence of the directory does not mean a thing.

I mean you added "from here on, use rerere", but if I instrument the
tests to set rerere.enabled=false they also pass, sans the .git/rr-cache
directory being created.

So we weren't "using rerere", we were just writing the data on the side.

We *do* get a failure in test #3, "rm records reset clears", if we set
rerere.autoUpdate=true. So it seems to me that what those first tests
would benefit more from not having the addition of your
rerere.enabled=false line early on.

After all it's more interesting to test that the merge resolution is not
different in any way under rerere.enabled=true & rerere.autoUpdate=false
than to not write the rr-cache data at all. It seems like having just
one test with rerere.enabled=false & "I did not write .git/rr-cache"
would make for better coverage.

I.e. this on top works:
	
	diff --git a/t/t2030-unresolve-info.sh b/t/t2030-unresolve-info.sh
	index 8c571ddf92..b010f504b0 100755
	--- a/t/t2030-unresolve-info.sh
	+++ b/t/t2030-unresolve-info.sh
	@@ -46,11 +46,12 @@ prime_resolve_undo () {
	 }
	 
	 test_expect_success setup '
	-	git config rerere.enabled false &&
	 	mkdir fi &&
	 	printf "a\0a" >binary &&
	 	git add binary &&
	+	! test_path_is_dir .git/rr-cache &&
	 	test_commit initial fi/le first &&
	+	test_path_is_dir .git/rr-cache &&
	 	git branch side &&
	 	git branch another &&
	 	printf "a\0b" >binary &&
	@@ -128,9 +129,6 @@ test_expect_success 'unmerge with plumbing' '
	 '
	 
	 test_expect_success 'rerere and rerere forget' '
	-	# from here on, use rerere.
	-	git config rerere.enabled true &&
	-	mkdir .git/rr-cache &&
	 	prime_resolve_undo &&
	 	echo record the resolution &&
	 	git rerere &&
	@@ -156,7 +154,6 @@ test_expect_success 'rerere and rerere forget' '
	 
	 test_expect_success 'rerere and rerere forget (subdirectory)' '
	 	rm -fr .git/rr-cache &&
	-	mkdir .git/rr-cache &&
	 	prime_resolve_undo &&
	 	echo record the resolution &&
	 	(cd fi && git rerere) &&

As an aside when testing this I found that I could make it segfault by
not doing the mkdir() in setup_rerere() and without:

	diff --git a/rerere.c b/rerere.c
	index 83e740d730..06fb86d5b7 100644
	--- a/rerere.c
	+++ b/rerere.c
	@@ -731,7 +731,10 @@ static void do_rerere_one_path(struct index_state *istate,
	 	/* Has the user resolved it already? */
	 	if (variant >= 0) {
	 		if (!handle_file(istate, path, NULL, NULL)) {
	-			copy_file(rerere_path(id, "postimage"), path, 0666);
	+			const char *dst = rerere_path(id, "postimage");
	+			if (copy_file(dst, path, 0666))
	+				die_errno(_("could not copy '%s' to '%s'"),
	+					  path, dst);
	 			id->collection->status[variant] |= RR_HAS_POSTIMAGE;
	 			fprintf_ln(stderr, _("Recorded resolution for '%s'."), path);
	 			free_rerere_id(rr_item);
	

I.e. we make the hard assumption that the directory has been created,
and don't check the return value(s) of various subsequent file copying
etc. So there's a rare segfault in the wild if the "setup_rerere()"
races with something that removes the .git/rr-cache (perhaps git-gc will
remove it entirely?).