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

[Ben Franksen]

Ben Franksen <ben.franksen at online.de> added the comment:

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

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