2 patches for repository http://darcs.net/screened:

patch 2831f357496865c76860dc9438f195dcb8b13fec
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Mar  2 14:26:11 CET 2021
  * rename addPendingDiffToPending to unsafeAddToPending

patch 8237940978023934f829ef3d5d449007b867d55f
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Mar  2 16:06:25 CET 2021
  * remove the unsafe tentativelyAddToPending, replace with addToPending

  The fact that this type checks witnesses the fact that indeed using
  tentativelyAddToPending was wrong: the patches we pass all start at the
  unrecorded state and thus should be commuted past the difference between
  pending and working before appending them to pending. This is what
  addToPending does which is why it is the safer choice here.

Attachments

• patch-preview.txt
• rename-addpendingdifftopending-to-unsafeaddtopending.dpatch

Looks ok. Was there a user-visible bug here? For many things we put
in pending, we could reorder them without commuting and get away with
it, but not for renames. Given it was only used in rebase unsuspend,
rollback and revert I could imagine we might not have noticed before.

> Was there a user-visible bug here? For many things we put
> in pending, we could reorder them without commuting and get away with
> it, but not for renames.

We are talking about commuting changes we want to add to pending with 
unrecorded changes that are /not/ in pending (typically just hunks or 
file removes). While commuting a rename with a hunk can modify the 
latter, this is not problematic here, since we don't store the result as 
patches (unless we record them). Of course this is just hand-waving and 
not a proof. But it may help explain why failures (if they were actually 
possible) have been rare and perhaps obscure enough to slip by.

> Given it was only used in rebase unsuspend,
> rollback and revert I could imagine we might not have noticed before.

I agree that this is possible, though I never instestigated it. It would 
be yet another reason why handling the pending patch was always very 
defensive.