[darcs-users] darcs patch: add substitution mechanism for PatchChoices (and 4 more)

Ganesh Sittampalam ganesh at earth.li
Sun Sep 27 21:30:00 UTC 2009


On Thu, 24 Sep 2009, Eric Kow wrote:

> On Sat, Sep 19, 2009 at 11:54:08 +0100, Ganesh Sittampalam wrote:
>> Sat Sep 19 01:40:53 BST 2009  Ganesh Sittampalam <ganesh at earth.li>
>>   * Resolve issue291: add (basic) interactive patch splitting
>
> Finally,  I get to comment on this one!
>
> This is with my usual thinking aloud approach to review, which may be a
> bit noisy.  If it helps, I'll prefix anything addressed to Ganesh with
> COMMENT.

Have tried to improve the docs in response to these comments (see 
separately submitted patch), or otherwise answered questions inline. 
Please do prod me with anything I didn't answer properly one way or the 
other.

> I think this is ready for me to push so that people can try out.
>
> The UI needs hammering out.

Absolutely. This is very much a first cut in that area. What I think I 
have done properly is to deliver a general framework we can use.

>> +import Data.List ( intersperse )
>
>> +-- There's no immediate application for a splitter for anything other than
>> +-- Prim (you shouldn't go editing named patches, you'll break them!)
>> +-- However you might want to compose splitters for FilePatchType to make
>> +-- splitters for Prim etc, and the generality doesn't cost anything.
>> +data Splitter p
>> +  = Splitter {
>> +              applySplitter :: FORALL(x y) p C(x y)
>> +                              -> Maybe (B.ByteString,
>> +                                        B.ByteString -> Maybe (FL p C(x y)))
>
> COMMENT: Maybe some lightweight haddock here would help.

Added.

>> +              -- canonization is needed to undo the effects of splitting
>> +             ,canonizeSplit :: FORALL(x y) FL p C(x y) -> FL p C(x y)
>
> Not sure I understand this

Haddocked.

>> +-- This generic splitter just lets the user edit the printed representation of the patch
>> +-- Should not be used expect for testing and experimentation.
>
> COMMENT: Shouldn't this be haddock?

Doesn't hurt, I guess.

> Using the raw splitter you edit the Darcs representation of the patch
> directly.
>
> The simplest thing to do with what the user gives us back is to pretend
> we're doing a merge, hence withEditedHead.  (The difference between this
> and a merge is that we don't commute out the inverse patch)

I wouldn't really characterise it as anything like a merge, but that's 
just quibbling over terminology really.

>> +-- The real (but still quite basic) functionality. Split a primitive hunk patch up
>> +-- by allowing the user to edit both the before and after lines, then insert fixup patches
>> +-- to clean up the mess.
>> +doPrimSplit :: Prim C(x y) -> Maybe (B.ByteString, B.ByteString -> Maybe (FL Prim C(x y)))
>> +doPrimSplit (FP fn (Hunk n before after))
>> + = Just (B.concat $ intersperse (BC.pack "\n") $ (helptext ++ [sep] ++ before ++ [sep] ++ after ++ [sep]),
>> +         \bs -> do let ls = BC.split '\n' bs
>
>> +                   (_, ls') <- breakSep ls
>> +                   (before', ls'') <- breakSep ls'
>> +                   (after', _) <- breakSep ls''
>
> If the user messes up the separators, this just fails, which is A-OK
>
> COMMENT: It looks like this would get confused if the user has hunks
> that coincidentally include our separator.

Agreed.

> We could try to make this less likely by using an unusual separator but 
> that's not really going to work either (consider the user who wants to 
> write a book about Darcs).

Such a book is unlikely to actually have the separator on a bare line.

> This makes me lean towards more of a UI that keeps the plus and minus
> signs which may end up being more flexible if we're careful.  But maybe
> there's another way out still.  Anyway, we can design this later.

I did originally plan on +/-, but the problem is that it makes it a real 
pain to move stuff from one side to the other.

>> +    where sep = BC.pack "==="
>> +          helptext = map BC.pack ["Interactive hunk edit:",
>> +                                  " - Edit the first set of lines to insert a new change before the current one.",
>> +                                  " - Edit the second set of lines to insert a new change after the current one."]
>
> COMMENT: If we go with separators, I'd like one separator to say
> "before" and one to say "after".
>
> In any case, I think I'd also like instructions on how to split a patch.

Fair enough. Someone should write some :-) (I'll try to at some point if 
noone else does).

> COMMENT: should with_selected_changes even take a splitter?

Not quite sure how we can avoid that. My original implementation just put 
the splitter in the Patchy type class, and only had it present for Prim. I 
think that's wrong design, as it takes away control from the command as to 
whether to offer splitting or not.

>> -  with_selected_changes_reversed "depend on" (filter askdep_allowed opts) ps'
>> +  with_selected_changes_reversed "depend on" (filter askdep_allowed opts) Nothing ps'
>
> No splitting in askdeps either

Or anything else involving Named patches.

>> -    _ -> with_selected_last_changes_to_files' "revert" opts
>> +    _ -> with_selected_last_changes_to_files' "revert" opts Nothing
>
>> -       with_selected_last_changes_to_files' "rollback" opts
>> +       with_selected_last_changes_to_files' "rollback" opts Nothing
>
>> -      with_selected_changes_to_files' "unrevert" opts [] pw $
>> +      with_selected_changes_to_files' "unrevert" opts Nothing [] pw $
>
> COMMENT: three places where it may be nice to have splitters in the
> future

Happy to add them if people feel they would be useful. It's trivial to do 
and arguably cleaner to have them than not.

>> hunk ./src/Darcs/SelectChanges.hs 87
>>  type WithPatches p a C(x y) =
>>          String              -- jobname
>>       -> [DarcsFlag]         -- opts
>> +     -> Maybe (Splitter p)  -- for interactive editing
>>       -> FL p C(x y)         -- patches to select among
>>       -> ((FL p :> FL p) C(x y) -> IO a) -- job
>>       -> IO a                -- result of running job
>
> I wish there was a way we could clean this module up.  It seems like
> there is an inherent tension between (a) having one massive function
> that handles everything but accepts a gazillion arguments and is
> unreadable and (b) having lots of little functions that seem kind of
> hard to tell apart.

Agreed. I think a record for the arguments (with default values that can 
be overridden) would help.

>> +-- After selecting with a splitter, the results may not be canonical
>> +canonizeWith :: Maybe (Splitter p) -> (FL p :> FL p) C(x y) -> (FL p :> FL p) C(x y)
>> +canonizeWith Nothing xy = xy
>> +canonizeWith (Just spl) (x :> y) = canonizeSplit spl x :> canonizeSplit spl y
>
> selected_patches_foo returns something like chosen :> skipped
>
> COMMENT: is there any danger of running this on patches that are not the
> result of splitting?  Danger here can include "something safe but
> totally unintuitive happens".

I don't think so, because the normal diffs presented by record etc should 
already be canonical. It's only splitting that introduces the need for 
cleaning up after.

>  Hmm: one thing which may happen is that
> in editing patches, the user somehow manages to make two previously
> non-touching patches touch which causes them to get coalesced.

That was intentional: I didn't think it should be possible to record some 
stream of patches that couldn't have been obtained by stopping and 
editing the files instead. I'm not completely sure that this 
is the right approach, but it feels like a better idea than allowing some 
weird patches to be recorded.

>> hunk ./src/Darcs/SelectChanges.hs 251
>>           then job $ ps_to_consider :> other_ps
>> -         else do pc <- tentatively_text_select "" jobname (Noun "patch") Last crit
>> +         else do pc <- tentatively_text_select splitter "" jobname (Noun "patch") Last crit
>>                                                opts ps_len 0 NilRL init_tps init_pc
>> hunk ./src/Darcs/SelectChanges.hs 253
>> -                 job $ selected_patches_last rejected_ps pc
>> +                 job $ canonizeWith splitter $ selected_patches_last rejected_ps pc
>
>> hunk ./src/Darcs/SelectChanges.hs 269
>> -                 job $ selected_patches_first rejected_ps pc
>> +                 job $ canonizeWith splitter $ selected_patches_first rejected_ps pc
>
>> hunk ./src/Darcs/SelectChanges.hs 286
>> -                 job $ selected_patches_first_reversed rejected_ps pc
>> +                 job $ canonizeWith splitter $ selected_patches_first_reversed rejected_ps pc
>
> COMMENT: shouldn't this really happen in text_select after splitting is
> called?  Oh maybe it can't because it would cause those patches that the
> user took pains to split to be magically coalesced?

Right.

> If this our splitter knows how to handle this patch, then accept the
> 'e' keystroke when it happens.
>
> COMMENT: maybe we can be fancy and react to the user trying to split
> patches we don't know how to split, eg "can't split a replace patch"

Perhaps - this would involve turning one of the Maybes in the splitter 
into an Either String, I guess.

Ganesh


More information about the darcs-users mailing list