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
unrecorded changes.
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
state.
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.
|
