[darcs-users] darcs patch: resolve issue628: reuse the long comment code from Dar...

Eric Kow kowey at darcs.net
Fri Oct 24 10:22:53 UTC 2008

This is nice and minimal.  Now let's shave it down some more!

On Fri, Oct 24, 2008 at 10:18:02 +0200, Christian Kellermann wrote:
> Now let's start the nitpicking :D

ok! :-)

resolve issue628: reuse the long comment code from Darcs.record
> +import Data.List ( isPrefixOf )

David: I confirm that this is in GHC 6.6.

> +import Darcs.Lock ( world_readable_temp )

(as used by record and amend record... the clutter left behind when
I cancel these commands is a bit of a nuisance, but then again, that
is more a feature than a bug, so I won't complain)

> -       tentativelyAddPatch repository opts $ n2pia $ adddeps mypatch deps
> -       finalizeRepositoryChanges repository
> -       when (CheckPoint `elem` opts) $ write_recorded_checkpoint repository myinfo
> -       putStrLn $ "Finished tagging patch '"++name++"'"

Other reviewers: this appears to be indentation-only.

> -\end{code}
> -Each tagged version has a version name.
> -\begin{code}

Note that you've lost this bit of user manual

> -get_patchname :: [DarcsFlag] -> IO String
> -get_patchname (PatchName n:_) = return $ "TAG "++n
> -get_patchname (_:flags) = get_patchname flags
> -get_patchname [] = do
> -    n <- askUser "What is the version name? "
> -    if n == "" then get_patchname []
> -               else return $ "TAG "++n

Unless I am mistaken, by using Darcs.Record.get_log, we now no longer
ask "What is the version name?" but "What is the patch name?"

> +  where  get_name_log :: [DarcsFlag] -> [String] -> IO (String, [String])

This is going to sound like the opposite of what I suggested last
time, is there a reason why get_name_log is in a where block and not a
top-level function?  It's not always easy to get a feel for when to do
one or the other, so it'll be good see your thoughts on it.  I was
mainly asking because not moving it may allow you make an even more
minimal patch, but you have had some design-thinking in there...

If you want to put it in a where block, maybe we can get rid of some
arguments, like the [DarcsFlag] one and just use the one from the
surrounding function.

> +         get_name_log o [] = do (name, comment, _) <- get_log o Nothing (world_readable_temp "darcs-tag") NilFL
> +                                return (add_tag name, comment)

> +         get_name_log o a = do let name = add_tag $ unwords a
> +                               (comment_name , comment, _) <- get_log (add_patch_name o name) Nothing (world_readable_temp "darcs-tag") NilFL
> +                               putStrLn $ "Comment " ++ comment_name ++ unlines comment
> +                               return (name, comment)

Also, is the putStrLn deliberate, or is it left-over debuggin information?

Now it looks to me like you can make this a bit clearer by combining the
two cases

get_name_log o a = do let o2 = if null a then o else (add_patch_name o (unwords a))
                      (name, comment, _) <- get_log o2 Nothing (world_readable_temp "darcs-tag") NilFL
                      return (add_tag name, comment)

Would something like that work?  Or is there some interaction with the
opts that I have missed?

> +         add_tag :: String -> String
> +         add_tag [] = []
> +         add_tag n | "TAG " `isPrefixOf` n = n
> +                   | otherwise = "TAG " ++ n

Note that this represents a change in behaviour.  Now you can no longer
name your tags TAG something (which would be silly to do anyway on the
other hand).

You could just replace the add_tag above by "TAG " ++

> +         add_patch_name :: [DarcsFlag] -> String -> [DarcsFlag]
> +         add_patch_name o a| has_patch_name o = o
> +                           | otherwise = [PatchName a] ++ o

So if the user specified a patch name with -m, we ignore the patch
name supplied on the command line args... fair enough, I guess :-)

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: 189 bytes
Desc: Digital signature
Url : http://lists.osuosl.org/pipermail/darcs-users/attachments/20081024/7d4ea51f/attachment.pgp 

More information about the darcs-users mailing list