Re: [PATCH] rerere: enable by default
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Jun 7, 2021, 12:56 UTC
- Message-ID
- <87v96p4w3f.fsf@evledraar.gmail.com>
- In-Reply-To
- <xmqqpmwyue8q.fsf@gitster.g>
On Mon, Jun 07 2021, Junio C Hamano wrote:
Show 20 quoted lines
> Æ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...
Show 22 quoted lines
>> @@ -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?).