Re: [PATCH] rerere: enable by default

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

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.

> It however has been in wide use without causing much trouble (other
> than, obviously, replaying a broken conflict resolution that was
> recorded earlier when the user made a mismerge), and it is about
> time to enable it by default.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  Documentation/config/rerere.txt |  5 ++---
>  rerere.c                        | 12 +++---------
>  t/t2030-unresolve-info.sh       |  3 +++
>  3 files changed, 8 insertions(+), 12 deletions(-)
>
> diff --git a/Documentation/config/rerere.txt b/Documentation/config/rerere.txt
> index 40abdf6a6b..45e97fc0fa 100644
> --- a/Documentation/config/rerere.txt
> +++ b/Documentation/config/rerere.txt
> @@ -7,6 +7,5 @@ rerere.enabled::
>  	Activate recording of resolved conflicts, so that identical
>  	conflict hunks can be resolved automatically, should they be
>  	encountered again.  By default, linkgit:git-rerere[1] is
> -	enabled if there is an `rr-cache` directory under the
> -	`$GIT_DIR`, e.g. if "rerere" was previously used in the
> -	repository.
> +	enabled, but this configuration can be set to 'false' to
> +	turn it off.
> diff --git a/rerere.c b/rerere.c
> index d83d58df4f..83e740d730 100644
> --- a/rerere.c
> +++ b/rerere.c
> @@ -18,8 +18,7 @@
>  #define THREE_STAGED 2
>  void *RERERE_RESOLVED = &RERERE_RESOLVED;
>  
> -/* if rerere_enabled == -1, fall back to detection of .git/rr-cache */
> -static int rerere_enabled = -1;
> +static int rerere_enabled = 1; /* default to true */
>  
>  /* automatically update cleanly resolved paths to the index */
>  static int rerere_autoupdate;
> @@ -852,16 +851,11 @@ static GIT_PATH_FUNC(git_path_rr_cache, "rr-cache")
>  
>  static int is_rerere_enabled(void)
>  {
> -	int rr_cache_exists;
> -
>  	if (!rerere_enabled)
>  		return 0;
>  
> -	rr_cache_exists = is_directory(git_path_rr_cache());
> -	if (rerere_enabled < 0)
> -		return rr_cache_exists;
> -
> -	if (!rr_cache_exists && mkdir_in_gitdir(git_path_rr_cache()))
> +	if (!is_directory(git_path_rr_cache()) &&
> +	    mkdir_in_gitdir(git_path_rr_cache()))
>  		die(_("could not create directory '%s'"), git_path_rr_cache());
>  	return 1;
>  }
> diff --git a/t/t2030-unresolve-info.sh b/t/t2030-unresolve-info.sh
> index be6c84c52a..8c571ddf92 100755
> --- a/t/t2030-unresolve-info.sh
> +++ b/t/t2030-unresolve-info.sh
> @@ -46,6 +46,7 @@ prime_resolve_undo () {
>  }
>  
>  test_expect_success setup '
> +	git config rerere.enabled false &&
>  	mkdir fi &&
>  	printf "a\0a" >binary &&
>  	git add binary &&
> @@ -127,6 +128,8 @@ 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 &&

It seems strange to disable it at the very beginning, but then not have
any test until you turn it on actually rely on that except that "mkdir"
in the second hunk, i.e. this on top works:
	
	diff --git a/t/t2030-unresolve-info.sh b/t/t2030-unresolve-info.sh
	index 8c571ddf92..7195ded9f9 100755
	--- a/t/t2030-unresolve-info.sh
	+++ b/t/t2030-unresolve-info.sh
	@@ -46,7 +46,6 @@ prime_resolve_undo () {
	 }
	 
	 test_expect_success setup '
	-	git config rerere.enabled false &&
	 	mkdir fi &&
	 	printf "a\0a" >binary &&
	 	git add binary &&
	@@ -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.

Those tests will fail if we turned on rerere.autoUpdate=true there, so
isn't it a better test coverage to assert that even with
rerere.enabled=true we don't impact these merge resolutions in the
earlier tests?

It also seems to make sense to change the main rerere tests to assert
that we have the default set to this new value, i.e.:
	
	diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh
	index 9f8c76dffb..f319f08852 100755
	--- a/t/t4200-rerere.sh
	+++ b/t/t4200-rerere.sh
	@@ -89,8 +89,8 @@ test_expect_success 'activate rerere, old style (conflicting merge)' '
	 '
	 
	 test_expect_success 'rerere.enabled works, too' '
	+	test_path_is_dir .git/rr-cache &&
	 	rm -rf .git/rr-cache &&
	-	git config rerere.enabled true &&
	 	git reset --hard &&
	 	test_must_fail git merge first &&
	 
	@@ -100,8 +100,8 @@ test_expect_success 'rerere.enabled works, too' '
	 '
	 
	 test_expect_success 'set up rr-cache' '
	+	test_path_is_dir .git/rr-cache &&
	 	rm -rf .git/rr-cache &&
	-	git config rerere.enabled true &&
	 	git reset --hard &&
	 	test_must_fail git merge first &&
	 	sha1=$(perl -pe "s/	.*//" .git/MERGE_RR) &&
	@@ -668,7 +668,6 @@ test_expect_success 'test simple stage 1 handling' '
	 	(
	 		cd stage_1_handling &&
	 
	-		git config rerere.enabled true &&
	 		git checkout A^0 &&
	 		test_must_fail git merge B^0
	 	)

As an aside: This pre-dates your change here, but I thought the config
reading in rerere.c was a bit odd, why do we call the one-off config
reading functions when we're about to iterate over the whole set anyway?
Turns out we did it the way I'd expect before 633e5ad326 (rerere.c:
replace `git_config()` with `git_config_get_*()` family, 2014-08-07).