Issue 2663 most uses of tentativelyAddToPending are wrong

Title most uses of tentativelyAddToPending are wrong
Priority bug Status resolved
Milestone Resolved in
Superseder Nosy List bfrk
Assigned To

Created on 2020-10-26.12:25:53 by bfrk, last changed 2023-07-16.21:23:54 by bfrk.

msg22502 (view) Author: bfrk 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.
msg22968 (view) Author: bfrk Date: 2022-04-12.13:24:00
resolved by

patch 8237940978023934f829ef3d5d449007b867d55f
Author: Ben Franksen <ben.franksen@online.de>
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 
at the
  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.
msg23203 (view) Author: bfrk Date: 2023-03-27.15:21:01
See patch2222
Date User Action Args
2020-10-26 12:25:53bfrkcreate
2022-04-12 13:24:00bfrksetstatus: unknown -> has-patch
priority: bug
messages: + msg22968
2023-03-27 15:21:01bfrksetmessages: + msg23203
2023-07-16 21:23:54bfrksetstatus: has-patch -> resolved