On Mon, Jun 07 2021, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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?).
