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

Eric Kow kowey at darcs.net
Thu Sep 24 05:54:33 UTC 2009


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.

I think this is ready for me to push so that people can try out.

The UI needs hammering out.  I have some questions below marked COMMENT,
but I haven't spotted anything in particular.

---------------------------------------------------------
Resolve issue291: add (basic) interactive patch splitting
Part I : Darcs.Patch.Split
---------------------------------------------------------

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

Here are some heavier comments

p C(x y) : patch to split

Maybe : so that splitters can control what kind of patches they will
        operate on (for example, Just for hunks, Nothing for other)

ByteString : what to send to the text editor

ByteString -> Maybe (FL p C(x y)) : how to deal with what the user
                                    sends back

> +              -- 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

> +-- 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?

> +rawSplitter :: (ShowPatch p, ReadPatch p, Invert p) => Splitter p
> +rawSplitter = Splitter {
> +                  applySplitter =
> +                     \p -> Just (renderPS . showPatch $ p,
> +                                 \str -> case parse_strictly (readPatch' False) str of
> +                                          Just (Just (Sealed res), _) -> Just (withEditedHead p res)
> +                                          _ -> Nothing
> +                                )
> +                 ,canonizeSplit = id
> +                }
>
> +-- Does not canonize as there is no generic operation to do this.
> +withEditedHead :: Invert p => p C(x y) -> p C(x z) -> FL p C(x y)
> +withEditedHead p res = res :>: invert res :>: p :>: NilFL

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)

> +-- 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.  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).
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.

> +                   return (hunk before before' +>+ hunk before' after' +>+ hunk after' after))

Here because we know exactly what we're dealing with, we can be a little
more fine-grained than merging.   No comments as I think this is pretty
straightforward

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

> +primSplitter :: Splitter Prim
> +primSplitter = Splitter { applySplitter = doPrimSplit, canonizeSplit = canonizeFL }

---------------------------------------------------------
Resolve issue291: add (basic) interactive patch splitting
Part II : Everything else
---------------------------------------------------------

> -              with_selected_changes_to_files' "add" (filter (==All) opts)
> +              with_selected_changes_to_files' "add" (filter (==All) opts) (Just primSplitter)

> -    with_selected_changes_to_files' "record" opts
> +    with_selected_changes_to_files' "record" opts (Just primSplitter)

Amend and record's interactive user interface are where we want splitters.
(the help text above says 'add' as in "do you want to add these changes
to the already pre-existing patch")

> -  with_selected_changes "apply" fixed_opts their_ps $
> +  with_selected_changes "apply" fixed_opts Nothing their_ps $
> -     with_selected_changes "pull" opts ps $
> +     with_selected_changes "pull" opts Nothing ps $
> -     with_selected_changes "push" opts (reverseRL us') $
> +     with_selected_changes "push" opts Nothing (reverseRL us') $
> -  with_selected_last_changes_to_files_reversed "rollback" opts existing_files
> +  with_selected_last_changes_to_files_reversed "rollback" opts Nothing existing_files
> -     with_selected_changes "send" opts our_ps $
> +     with_selected_changes "send" opts Nothing our_ps $
> -  with_selected_last_changes_reversed "unrecord" opts
> +  with_selected_last_changes_reversed "unrecord" opts Nothing

Nothing to split here.

COMMENT: should with_selected_changes even take a splitter?

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

> -    _ -> 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

---------------------------------------------------------
Resolve issue291: add (basic) interactive patch splitting
Part III : SelectChanges
---------------------------------------------------------

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

> +-- 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".  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.

> 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?

> -text_select :: forall p C(x y z). Patchy p => String -> WhichChanges
> +text_select :: forall p C(x y z). Patchy p => Maybe (Splitter p) -> String -> WhichChanges
>              ->  MatchCriterion p -> [DarcsFlag] -> Int -> Int
>              -> RL (TaggedPatch p) C(x y) -> FL (TaggedPatch p) C(y z) -> PatchChoices p C(x z)
>              -> IO ((PatchChoices p) C(x z))
> -text_select _ _ _ _ _ _ _ NilFL pc = return pc
> -text_select jn whichch crit opts n_max n
> +text_select _ _ _ _ _ _ _ _ NilFL pc = return pc
> +text_select splitter jn whichch crit opts n_max n

This is the main change

> +        split = splitter >>= flip applySplitter (tp_patch tp)

> +        options_split
> +           | Just _ <- split
> +                 = [ KeyPress 'e' ("interactively edit this "++thing) ]
> +           | otherwise = []

Only show the 'e' keypress if we have a splitter defined for this case

> hunk ./src/Darcs/SelectChanges.hs 471
>              'y' -> do_next $ force_yes (tag tp) pc
>              'n' -> do_next $ force_no (tag tp) pc
>              'w' -> do_next $ make_uncertain (tag tp) pc
> +            'e' | Just (text, parse) <- split

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"

> +                -> do newText <- edit_text "darcs-patch-edit" text

Edit the patch representation proposed to use by the splitter

> +                      case parse newText of
> +                        Nothing -> repeat_this
> +                        Just ps -> do let tps_new = snd $ patch_choices_tps_sub (Just (tag tp)) ps
> +                                      text_select splitter
> +                                                  jn whichch crit opts (n_max + lengthFL tps_new - 1) n
> +                                                  tps_done (tps_new+>+tps_todo')
> +                                                  (substitute (seal2 (tp :||: tps_new)) pc)

Slip the new patches into our stream (with new names).
Note: count updated by accounting for patch we got rid of and patches we
replaced it by.

> -tentatively_text_select :: Patchy p => String -> String -> Noun -> WhichChanges
> +tentatively_text_select :: Patchy p => Maybe (Splitter p) -> String -> String -> Noun -> WhichChanges
>                          -> MatchCriterion p -> [DarcsFlag]
>                          -> Int -> Int -> RL (TaggedPatch p) C(x y) -> FL (TaggedPatch p) C(y z)
>                          -> PatchChoices p C(x z)

I forget what tentatively_text_select is for but it doesn't really
matter here since we're just feeding the splitter to text_select

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
PGP Key ID: 08AC04F9
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 194 bytes
Desc: not available
URL: <http://lists.osuosl.org/pipermail/darcs-users/attachments/20090924/4b7d65ce/attachment.pgp>


More information about the darcs-users mailing list