[darcs-devel] darcs patch: show patch numbers instead of dots on get

[David Roundy]

On Wed, Aug 03, 2005 at 09:58:36PM -0400, Matt Lavin wrote:
> Here is my attempt at fixing bug 212.  The bug report said that the dots 
> should be replaced with patch counts like the copy_repo_patches code does 
> and that is exactly what this patch does.  
> 
> However, the bug report also mentions that check, optimize, and other
> places could also use the improved user feedback.  I didn't want to guess
> at exactly what this meant since I'm pretty new to darcs development.
> Get is is only command which used '.'s for feedback, and all other
> commands didn't show any feedback unless --verbose was specified.  Was
> the intention to show more feedback by default on the other commands, or
> only to include the patch counts in the verbose feedback?  Either way
> would very possible for me to implement, I just didn't want to waste any
> time making an incorrect assumption.

Yeah, that's what I was thinking, to include counts by default, nothing if
--quiet, and patch names if verbose on optimize --checkpoint, check, etc.

> Wed Aug  3 21:36:49 EDT 2005  Matt Lavin <matt.lavin at gmail.com>
>   * show patch numbers instead of dots on get

Looks nice, thanks!

> hunk ./DarcsRepo.lhs 573
> +apply_patches_with_feedback :: [DarcsFlag] -> (Int -> Int -> PatchInfo -> IO()) -> (Doc -> IO ())
> +                            -> [(PatchInfo, Maybe Patch)] -> IO ()

It might be nice to define either here in DarcsRepo, or perhaps elsewhere a

simple_feedback :: String -> [DarcsFlag] -> Int -> Int -> PatchInfo -> IO ()

where the string would be the verb to use in the message "Finished
applying...", and the [DarcsFlag] would determine whether to use patch
counts, patch names or nothing.

If you implemented this, you might change

apply_patches_with_feedback ::
         -> Either (String, [DarcsFlag))
                   (Int -> Int -> PatchInfo -> IO()) -> (Doc -> IO ())
         -> [(PatchInfo, Maybe Patch)] -> IO ()

which would allow commands to just give their name and their flags and let
apply_patches_with_feedback decide what to display.

> +apply_patches_with_feedback _ feedback putInfo patches = do
> +    apply_cautiously patches 1
> +    where patch_count = length patches

This could be a bit dangerous, if computing the length were to break a
necesary lazy evaluation.  I can't forsee any time this would cause
trouble, but the it's worth keeping in mind.

> +          apply_cautiously ((pinfo, Nothing) : more_patches) index =
> +             do errorDoc $ text "Couldn't read patch" <+> human_friendly pinfo
> +                apply_cautiously more_patches (index+1)

This strikes me as a bit odd.  The apply_cautiously (index+1) here will
never be called, because of the errorDoc.  It looks like you directly
translated my mapM_, but didn't realize that errorDoc always throws an
exception, so the rest of the list doesn't get applied (that being the
cautiousness in apply_cautiously).

So I'd prefer to drop the do here and the apply_cautiously to make the code
clearer to read.
-- 
David Roundy
http://www.darcs.net