[darcs-devel] [patch2131] make writeUnrevert take its arguments in... (and 8 more)

Ganesh Sittampalam bugs at darcs.net
Tue Dec 22 00:46:13 UTC 2020


Ganesh Sittampalam <ganesh at earth.li> added the comment:

>   * 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 at 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

__________________________________
Darcs bug tracker <bugs at darcs.net>
<http://bugs.darcs.net/patch2131>
__________________________________


More information about the darcs-devel mailing list