[darcs-users] darcs patch: resolve issue628: reuse the long comment code from Dar...
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
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.
> -Each tagged version has a version name.
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
> + 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
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
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
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