darcs

Patch 2131 make writeUnrevert take its arguments in... (and 8 more)

Title make writeUnrevert take its arguments in... (and 8 more)
Superseder Nosy List bf
Related Issues
Status accepted-pending-tests Assigned To bf
Milestone

Created on 2020-12-20.07:15:01 by bf, last changed 2020-12-23.23:08:19 by ganesh.

Files
File name Status Uploaded Type Edit Remove
make-writeunrevert-take-its-arguments-in-a-more-natural-order.dpatch bf, 2020-12-20.07:14:59 application/x-darcs-patch
patch-preview.txt bf, 2020-12-20.07:14:58 text/x-darcs-patch
patch-preview.txt bf, 2020-12-22.10:11:44 text/x-darcs-patch
patch-preview.txt bf, 2020-12-22.10:16:44 text/x-darcs-patch
revert_-make-naming-more-consistent-with-d_r_unrevert-and-with-other-commands.dpatch bf, 2020-12-22.10:11:44 application/x-darcs-patch
unnamed bf, 2020-12-20.07:14:59 text/plain
unnamed bf, 2020-12-22.10:11:44 text/plain
unnamed bf, 2020-12-22.10:16:44 text/plain
unrevert_-make-naming-more-consistent-with-d_r_unrevert-and-with-other-commands.dpatch bf, 2020-12-22.10:16:44 application/x-darcs-patch
See mailing list archives for discussion on individual patches.
Messages
msg22574 (view) Author: bf Date: 2020-12-20.07:14:59
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
msg22594 (view) Author: ganesh Date: 2020-12-22.00:46:12
>   * 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
msg22595 (view) Author: bf Date: 2020-12-22.10:11:11
>>   * 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.
msg22596 (view) Author: bf Date: 2020-12-22.10:11:44
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
msg22597 (view) Author: bf Date: 2020-12-22.10:16:44
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
msg22598 (view) Author: bf Date: 2020-12-22.10:18:52
I have screened both follow-ups.
msg22604 (view) Author: ganesh Date: 2020-12-23.23:07:43
>> 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.
History
Date User Action Args
2020-12-20 07:15:01bfcreate
2020-12-20 07:17:02bfsetstatus: needs-screening -> needs-review
2020-12-22 00:46:09ganeshsetstatus: needs-review -> followup-requested
assignedto: bf
2020-12-22 00:46:13ganeshsetmessages: + msg22594
2020-12-22 10:11:13bfsetmessages: + msg22595
2020-12-22 10:11:45bfsetfiles: + patch-preview.txt, revert_-make-naming-more-consistent-with-d_r_unrevert-and-with-other-commands.dpatch, unnamed
messages: + msg22596
2020-12-22 10:16:44bfsetfiles: + patch-preview.txt, unrevert_-make-naming-more-consistent-with-d_r_unrevert-and-with-other-commands.dpatch, unnamed
messages: + msg22597
2020-12-22 10:18:53bfsetmessages: + msg22598
2020-12-23 23:07:44ganeshsetmessages: + msg22604
2020-12-23 23:08:19ganeshsetstatus: followup-requested -> accepted-pending-tests