[darcs-devel] darcs patch: Add a 'commit' command stub. (and 6 more)

[David Roundy]

On Wed, Jul 25, 2007 at 11:14:50PM +0200, Eric Kow wrote:
> The thing of interest here is "Implement amend-record --edit-long-comment and
> --author.'
> 
> Rather clumsily executed though.
...
> +                       let fn `or_default_to` d = fromMaybe d `fmap` fn opts
> +                       firstname <- get_patchname_discreetly `or_default_to` old_name
> +                       let get_log_discreetly fs =
> +                               if (PromptLongComment `elem` fs || EditLongComment `elem` fs)
> +                                  then do let make_log = world_readable_temp "darcs-amend-record"
> +                                          (n, l, _) <- get_log fs firstname make_log chs
> +                                          return $ Just (n, l)
> +                                  else return Nothing
> +                           get_author_discreetly (Author a:_) = return $ Just a
> +                           get_author_discreetly (_:as) = get_author_discreetly as
> +                           get_author_discreetly []     = return Nothing
> +                       new_author          <- get_author_discreetly `or_default_to` old_author
> +                       (new_name, new_log) <- get_log_discreetly `or_default_to` (firstname, old_log)
> +                       --
> +                       let maybe_invert = if is_inverted old_pinf then invert_name else id
> +                           new_pinf = maybe_invert $ patchinfo date new_name new_author new_log
> +                           newp = fixp oldp chs new_pinf
>                         when (want_to_do_test opts) $
>                              do testproblem <- test_patch opts (join_patches chs)
>                                 when (testproblem /= ExitSuccess) $

...

> +get_patchname_discreetly :: [DarcsFlag] -> IO (Maybe String)
> +get_patchname_discreetly (PatchName n:_) | take 4 n == "TAG " = return $ Just $ '.':n
> +                                         | otherwise = return $ Just n
> +get_patchname_discreetly (LogFile f:fs) =
>      do t <- (lines `fmap` readBinFile f) `catch` \_ -> return []
> +       case t of []    -> get_patchname_discreetly fs
> +                 (n:_) -> return $ Just n
> +get_patchname_discreetly (_:flags) = get_patchname_discreetly flags
> +get_patchname_discreetly [] = return Nothing

Why not put get_log_discreetly directly in Record.lhs under the name of
get_patchname_discreetly? It isn't clear to me how their functionality
differs, or why we want the one rather than the other.

Specifically, it seems like editing the existing patchname is something
that we want even when recording (particularly if we're using a stored log
file from a previous record, we might want to specify both --log-file and
--edit-long-description, or whatever the flags are called).

I wonder if we couldn't just have a single

get_patchinfo :: [DarcsFlag] -> IO PatchInfo

which works identically whether we're recording or amend-recording? Or
maybe something like:

get_patchinfo :: [DarcsFlag] -> Maybe PatchInfo -> IO PatchInfo

in which the Maybe PatchInfo input provides the "old" patchinfo for
amend-record.  I'm not sure.  I'm afraid your code (as you point out) gave
me a bit of a headache to review, so it'd be nice to clean up... (as you
suggest).
-- 
David Roundy
Department of Physics
Oregon State University