This is a first series of patches pulled from by experimental branch. They
are all about unrevert and not supposed to change behavior, it's pure
refactors.

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

patch f603d95c27a5f143a6ec10593ad0c96876ba5713
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Oct 23 11:50:51 CEST 2020
  * make writeUnrevert take its arguments in a more natural order

patch 768c25e2f0de602588714759f62d831dcbadd440
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Oct 23 15:33:20 CEST 2020
  * unrevert: rename some local variables

patch 5a7b9512e4db8bfdd93b3a8d31c4ffdbf606cfbb
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Oct 23 15:31:52 CEST 2020
  * writeUnrevert: remove the 'pending' argument
  
  This is a preparation for moving writeUnrevert to the Repository layer. The
  code we move from writeUnrevert into the command implementation is asking
  the user, so belongs to the UI layer.

patch 0e2c0bc21b7ee1d8e9b963a6ed7635d2295b6d5b
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Oct 23 15:45:22 CEST 2020
  * move writeUnrevert and unrevertPatchBundle to Darcs.Repository.Unrevert

patch a22a4eb3d1e49231e917c3931cf550b114c9a0dd
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Oct 23 15:59:33 CEST 2020
  * cleanup imports in Darcs.UI.Commands.Unrevert

patch 39ef756740bf8770f8bc119bb8314e935e7e1500
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Oct 23 16:34:26 CEST 2020
  * move call to readRepo out of writeUnrevert
  
  This prepares for moving removeFromUnrevertContext from D.R.Hashed to
  D.R.Unrevert. It also makes it utterly clear that unrevert is unsafe: we
  read the repo and then modify the unrevert bundle /after/ it the changes
  have been finalized.

patch 86c2fefcc71d98a24fab89fb580fea07fdbdabea
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Oct 23 18:14:30 CEST 2020
  * unrevert: use promptYorn instead of askUser

patch 6530cb226cb232608008afdc66f2214135588cee
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Nov  3 08:37:35 CET 2020
  * simplify removeFromUnrevertContext

patch 10c7370af487b2842527697fb57b3dfc9e893415
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Dec 19 15:51:36 CET 2020
  * move removeFromUnrevertContext to D.R.Unrevert

Attachments

• make-writeunrevert-take-its-arguments-in-a-more-natural-order.dpatch
• patch-preview.txt
• unnamed

>   * make writeUnrevert take its arguments in a more natural order

OK

>   * unrevert: rename some local variables

OK

>   * writeUnrevert: remove the 'pending' argument
>   
>   This is a preparation for moving writeUnrevert to the Repository layer. The
>   code we move from writeUnrevert into the command implementation is asking
>   the user, so belongs to the UI layer.

OK, though I'm not sure if I like seeing yet more patch manipulation
code in the Commands layer. I don't have a good alternative though.
Perhaps we need data structures to abstract interactivity.

>   * move writeUnrevert and unrevertPatchBundle to Darcs.Repository.Unrevert

I like this direction much better :-)

>   * cleanup imports in Darcs.UI.Commands.Unrevert

OK


> patch 39ef756740bf8770f8bc119bb8314e935e7e1500
> Author: Ben Franksen <ben.franksen@online.de>
> Date:   Fri Oct 23 16:34:26 CEST 2020
>   * move call to readRepo out of writeUnrevert
>   
>   This prepares for moving removeFromUnrevertContext from D.R.Hashed to
>   D.R.Unrevert. It also makes it utterly clear that unrevert is unsafe: we
>   read the repo and then modify the unrevert bundle /after/ it the changes
>   have been finalized.

>     hunk ./src/Darcs/Repository/Unrevert.hs 27
>     -writeUnrevert repository pristine ps = do
>     -  rep <- readRepo repository
>     +writeUnrevert recorded pristine ps = do

>     hunk ./src/Darcs/UI/Commands/Revert.hs 176
>     -                   deps :> torevert' :> _ ->
>     -                      writeUnrevert repository recorded (deps +>>+ torevert')
>     +                   deps :> torevert' :> _ -> do
>     +                      ps <- readRepo repository
>     +                      writeUnrevert ps recorded (deps +>>+ torevert')

The parameter naming is confusing here. In the old code, something
called 'recorded' was being passed to a parameter named 'pristine'.
In the new code, that still happens but we also pass 'ps' to a parameter
named 'recorded'. Could you clean that up a bit, as you already did
further down? [I haven't read ahead yet, so perhaps you do so in a later
bundle]
>   * unrevert: use promptYorn instead of askUser

OK

>   * simplify removeFromUnrevertContext

OK (I suspect this came from a time when GADT type inference/matching
didn't work so well)

>   * move removeFromUnrevertContext to D.R.Unrevert

OK

>>   * writeUnrevert: remove the 'pending' argument
>>   
>>   This is a preparation for moving writeUnrevert to the Repository layer. The
>>   code we move from writeUnrevert into the command implementation is asking
>>   the user, so belongs to the UI layer.
> 
> OK, though I'm not sure if I like seeing yet more patch manipulation
> code in the Commands layer.

It is debatable whether this is the case here, but apart from that I am
not sure I agree. IMO patch manipulation and then passing patches (FL,
RL, Fork, PatchSet, ...) is clearly superior compared to the
alternative, which is passing the Repository and then (re-)reading
patches from disk.

> I don't have a good alternative though.
> Perhaps we need data structures to abstract interactivity.

Yes, I had a similar idea. Something like passing down a 'decision
procedure' i.e. something of type IO Bool. One instance where we could
try that out is with  the interactive parts of tentativelyMergePatches.

>>   * move call to readRepo out of writeUnrevert
>>   
>>   This prepares for moving removeFromUnrevertContext from D.R.Hashed to
>>   D.R.Unrevert. It also makes it utterly clear that unrevert is unsafe: we
>>   read the repo and then modify the unrevert bundle /after/ it the changes
>>   have been finalized.
> 
>>     hunk ./src/Darcs/Repository/Unrevert.hs 27
>>     -writeUnrevert repository pristine ps = do
>>     -  rep <- readRepo repository
>>     +writeUnrevert recorded pristine ps = do
> 
>>     hunk ./src/Darcs/UI/Commands/Revert.hs 176
>>     -                   deps :> torevert' :> _ ->
>>     -                      writeUnrevert repository recorded (deps +>>+ torevert')
>>     +                   deps :> torevert' :> _ -> do
>>     +                      ps <- readRepo repository
>>     +                      writeUnrevert ps recorded (deps +>>+ torevert')
> 
> The parameter naming is confusing here. In the old code, something
> called 'recorded' was being passed to a parameter named 'pristine'.
> In the new code, that still happens but we also pass 'ps' to a parameter
> named 'recorded'. Could you clean that up a bit, as you already did
> further down? [I haven't read ahead yet, so perhaps you do so in a later
> bundle]

Good point! I remember being dissatisfied with this but then forgot to
do something about it. Will fix.

Related: we may also want to rename readRecorded to readPristine, which
is less ambiguous ("recorded" can mean the recorded patches or the
recorded state). There are many places where we say something like

  pristine <- readRecorded repository

Furthermore, readRepo should be named readRecorded or perhaps
readRecordedPatches.

Following up on review.

1 patch for repository http://darcs.net/screened:

patch 2218cf697a523b9d9886236e49eca5e27546a3d3
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Dec 22 11:09:18 CET 2020
  * revert: make naming more consistent with D.R.Unrevert and with other commands

Attachments

• patch-preview.txt
• revert_-make-naming-more-consistent-with-d_r_unrevert-and-with-other-commands.dpatch
• unnamed

Sorry, I should have included this with my other follow-up patch. It makes a
similar change for the unrevert command.

1 patch for repository http://darcs.net/screened:

patch 1b8ffca3589e8b44b93503a17fe4a20a781043a8
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Dec 22 11:31:43 CET 2020
  * unrevert: make naming more consistent with D.R.Unrevert and with other commands

Attachments

• patch-preview.txt
• unnamed
• unrevert_-make-naming-more-consistent-with-d_r_unrevert-and-with-other-commands.dpatch

I have screened both follow-ups.

>> OK, though I'm not sure if I like seeing yet more patch manipulation
>> code in the Commands layer.
> 
> It is debatable whether this is the case here, but apart from that I am
> not sure I agree. IMO patch manipulation and then passing patches (FL,
> RL, Fork, PatchSet, ...) is clearly superior compared to the
> alternative, which is passing the Repository and then (re-)reading
> patches from disk.

Yes, true.

>> I don't have a good alternative though.
>> Perhaps we need data structures to abstract interactivity.
> 
> Yes, I had a similar idea. Something like passing down a 'decision
> procedure' i.e. something of type IO Bool. One instance where we could
> try that out is with  the interactive parts of tentativelyMergePatches.

Passing down IO Bool wouldn't work when the interaction required is in a
different monad. I was vaguely wondering about some kind of structure like

  data RevertResult = OK (IO ()) | CantUnrevert (IO ())

but that doesn't seem great either.

> Related: we may also want to rename readRecorded to readPristine, which
> is less ambiguous ("recorded" can mean the recorded patches or the
> recorded state). There are many places where we say something like
> 
>   pristine <- readRecorded repository
> 
> Furthermore, readRepo should be named readRecorded or perhaps
> readRecordedPatches.

Sounds good.