This function has the following warning attached:
-- This fuction is unsafe because it accepts a patch that works on the
-- tentative pending and we don't currently track the state of the
-- tentative pending.
What it does is to append the given prims to the tentative pending patch.
Indeed, all uses of this function would produce a type error if we tracked
the end state of pending. The only use case that is strictly correct is the
one in rebase unsuspend, where we previously checked that there are no
For obliterate and rebase suspend we pass it prims that start at the recorded
state; for unrevert and rollback we pass it prims that start at the working
Why doesn't this lead to crashes all over the place? Finalizing the tentative
pending includes a test that the new pending can be applied to the (new,
already finalized) recorded state, so we should run into errors a la "There
was an attempt to write an invalid pending!".
I believe these crashes are avoided in practice because we apply
siftForPending to the tentative pending patch before we check that it can be
applied to the pristine state. Indeed, if I disable the call(s) to
siftForPending I can create a scenario where e.g. rollback crashes because
the tentative pending contains a hunk that cannot be applied to pristine.
I haven't checked that for the other 3 commands yet. However, it should be
obvious that relying on sifting alone to ensure validity of the pending patch is a very bad idea, even if it happens to work.
So instead of spending any more effort to prove that the current code works
in all cases (which is pretty difficult), I will remove
tentativelyAddToPending in its current form and replace it with something
that takes prims that start at a known state (recorded, tentative, or
working), inserting the nessecary commutations to ensure the result is valid.
Author: Ben Franksen <firstname.lastname@example.org>
Date: Tue Mar 2 16:06:25 CET 2021
* remove the unsafe tentativelyAddToPending, replace with
The fact that this type checks witnesses the fact that indeed
tentativelyAddToPending was wrong: the patches we pass all start
unrecorded state and thus should be commuted past the difference
pending and working before appending them to pending. This is what
addToPending does which is why it is the safer choice here.