Issue 2663 most uses of tentativelyAddToPending are wrong

Title most uses of tentativelyAddToPending are wrong
Priority Status unknown
Milestone Resolved in
Superseder Nosy List bf
Assigned To

Created on 2020-10-26.12:25:53 by bf, last changed 2020-10-26.12:25:53 by bf.

msg22502 (view) Author: bf Date: 2020-10-26.12:25:47
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 

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.
Date User Action Args
2020-10-26 12:25:53bfcreate