Hi,

as discussed on IRC, this is a forward-port of the fast(er) (and
human-readable) annotate. I am throwing in dead code elimination as a free
bonus.  The code is uglier than necessary in handling of paths, probably since
it is more or less straight port from adventure. Or so I guess. If anyone has
the patience to find out more correct ways to handle paths with current darcs
code, please be my guest.

Yours,
   Petr

PS: I know that features are important, but I also think that cleaning up the
code is more so. Giving priority to features over code restructuring is
probably going to backfire. IMHO, anyway.

2 patches for repository http://darcs.net:

Sat Apr  2 22:48:20 CEST 2011  Petr Rockai <me@mornfall.net>
  * Forward-port of a new annotate implementation.

Sat Apr  2 22:48:26 CEST 2011  Petr Rockai <me@mornfall.net>
  * Slash unused Population code.

Attachments

• forward_port-of-a-new-annotate-implementation_.dpatch
• unnamed
• unnamed

On 02/04/2011 22:04, Petr Ročkai wrote:

> Sat Apr  2 22:48:20 CEST 2011  Petr Rockai <me@mornfall.net>
>   * Forward-port of a new annotate implementation.

Generally looks fine.

> +instance MonadPlus AnnotatedM where -- :(
> +    mplus a b = a >> b -- this is wrong, but we can't catch here

I think this should be mplus a b = a, if anything, but you'll be pleased
to know I've now got rid of the MonadPlus requirement for
Readable/WriteableDirectory (patch to be submitted soon) so it's not
necessary at all.

> hunk ./src/Darcs/Match.hs 440
>  withRecordedMatch r job = do createPristineDirectoryTree r "."
>                               readRepo r >>= job
>  
> -applyInvRL :: (Patchy p) => RL (PatchInfoAnd p) C(x r) -> IO ()
> -applyInvRL = applyPatches . invertRL -- this gives nicer feedback
> +applyInvRL :: (WriteableDirectory m, Patchy p) => RL (PatchInfoAnd p) C(x r) -> m ()
> + -- we forego feedback here to be able to live in "m" as opposed to IO
> +applyInvRL = apply . invertRL

Would be nice to keep the friendly behaviour at least when m = IO. The
IO is used for unsafePerformIO-free progress, for catching errors and
for printing to stderr. Of course the removal of MonadPlus means we
can't catch errors in general :-)

What do you think about adding something to WriteableDirectory to
optionally provide feedback, using something like the below?:

-- |a monadic action, annotated with a progress message that could be
printed out
-- while running the action, and a message that could be printed out on
error.
-- Actually printing out these messages is optional to allow non-IO
monads to
-- just run the action.
data ProgressAction m a =
  ProgressAction
   {paAction :: m a
   ,paMessage :: Doc
   ,paOnError :: Doc
   }

-- |run a list of 'ProgressAction's, using the progress and on error
messages
withFeedback :: String -> [ProgressAction IO ()] -> IO ()


-- |run a list of 'ProgressAction's without any feedback messages
withoutFeedback :: Monad m => [ProgressAction m ()] -> m ()

> Sat Apr  2 22:48:26 CEST 2011  Petr Rockai <me@mornfall.net>
>   * Slash unused Population code.

Excellent!

Ganesh

Hey,

Ganesh Sittampalam <bugs@darcs.net> writes:

>> +instance MonadPlus AnnotatedM where -- :(
>> +    mplus a b = a >> b -- this is wrong, but we can't catch here
>
> I think this should be mplus a b = a, if anything, but you'll be pleased
> to know I've now got rid of the MonadPlus requirement for
> Readable/WriteableDirectory (patch to be submitted soon) so it's not
> necessary at all.

well, I wanted to keep the instance empty, but that fails... Indeed,
mplus a _ = a may be better.

>> hunk ./src/Darcs/Match.hs 440
>>  withRecordedMatch r job = do createPristineDirectoryTree r "."
>>                               readRepo r >>= job
>>  
>> -applyInvRL :: (Patchy p) => RL (PatchInfoAnd p) C(x r) -> IO ()
>> -applyInvRL = applyPatches . invertRL -- this gives nicer feedback
>> +applyInvRL :: (WriteableDirectory m, Patchy p) => RL (PatchInfoAnd p) C(x r) -> m ()
>> + -- we forego feedback here to be able to live in "m" as opposed to IO
>> +applyInvRL = apply . invertRL
>
> Would be nice to keep the friendly behaviour at least when m = IO. The
> IO is used for unsafePerformIO-free progress, for catching errors and
> for printing to stderr. Of course the removal of MonadPlus means we
> can't catch errors in general :-)

I am not very fond of the progress reporting either way.

> What do you think about adding something to WriteableDirectory to
> optionally provide feedback, using something like the below?:

I am a bit wary of that, since it feels a bit like "fail" in Monad. It
may be convenient, but it doesn't belong. I would prefer a MonadProgress
class instead, which would make it fairly easy to provide or not provide
feedback depending on who you are.

Yours,
    Petr

-- 
id' Ash = Ash; id' Dust = Dust; id' _ = undefined

Hey there,

this should resolve some of the issues with the previous iteration, by
forward-porting the apply monad.

Yours,
   Petr

4 patches for repository http://darcs.net/screened:

Sat Apr  2 22:48:20 CEST 2011  Petr Rockai <me@mornfall.net>
  * Forward-port of a new annotate implementation.

Sat Apr  2 22:48:26 CEST 2011  Petr Rockai <me@mornfall.net>
  * Slash unused Population code.

Wed Apr  6 14:36:05 CEST 2011  Petr Rockai <me@mornfall.net>
  * Fix a couple of shadowing warnings in Darcs.Annotate.

Sat Apr  9 13:53:05 CEST 2011  Petr Rockai <me@mornfall.net>
  * Forward-port ApplyMonad.

Attachments

• forward_port-of-a-new-annotate-implementation_.dpatch
• unnamed
• unnamed

Petr Ročkai <bugs@darcs.net> writes:

> this should resolve some of the issues with the previous iteration, by
> forward-porting the apply monad.

scratch that, this version doesn't build... I'll get you one that's more
properly merged with MonadProgress in a bit

Petr

-- 
id' Ash = Ash; id' Dust = Dust; id' _ = undefined

Hi again,

this should finally build and work as promised. I am throwing in a faster
"darcs show contents" as a bonus.

(unfortunately, `fp2fn $ ("./" ++) $ anchorPath "" x` seems to be the right way
to go from AnchoredPath to FileName; similar problems apply in the annotate
code; a hashed-storage 0.6 port / path refactor had (would; I suppose it needs
to be redone anyway) solve these)

Petr

5 patches for repository http://darcs.net/screened:

Sat Apr  2 22:48:20 CEST 2011  Petr Rockai <me@mornfall.net>
  * Forward-port of a new annotate implementation.

Sat Apr  2 22:48:26 CEST 2011  Petr Rockai <me@mornfall.net>
  * Slash unused Population code.

Wed Apr  6 14:36:05 CEST 2011  Petr Rockai <me@mornfall.net>
  * Fix a couple of shadowing warnings in Darcs.Annotate.

Sat Apr  9 16:16:28 CEST 2011  Petr Rockai <me@mornfall.net>
  * Forward-port ApplyMonad.

Sat Apr  9 17:02:17 CEST 2011  Petr Rockai <me@mornfall.net>
  * Forward-port a darcs show contents optimisation.

Attachments

• forward_port-of-a-new-annotate-implementation_.dpatch
• unnamed
• unnamed

Another try, this time also including the new file.

5 patches for repository http://darcs.net/screened:

Sat Apr  2 22:48:20 CEST 2011  Petr Rockai <me@mornfall.net>
  * Forward-port of a new annotate implementation.

Sat Apr  2 22:48:26 CEST 2011  Petr Rockai <me@mornfall.net>
  * Slash unused Population code.

Wed Apr  6 14:36:05 CEST 2011  Petr Rockai <me@mornfall.net>
  * Fix a couple of shadowing warnings in Darcs.Annotate.

Sat Apr  9 17:31:00 CEST 2011  Petr Rockai <me@mornfall.net>
  * Forward-port ApplyMonad.

Sat Apr  9 17:31:05 CEST 2011  Petr Rockai <me@mornfall.net>
  * Forward-port a darcs show contents optimisation.

Attachments

• forward_port-of-a-new-annotate-implementation_.dpatch
• unnamed
• unnamed

I forgot to CC the bug tracker with this comment. Also, I'd like to add 
that your code is full of warnings, grmph :-)

Hi,

I've given this a fairly light review (in particular I have not read
every line of code) and will apply it to screened if it isn't already
there. I won't push to reviewed straight away to give an opportunity for
anyone else who wants to do a more thorough review, but in the absence
of that I suggest it goes to reviewed in a few days or so. I also
haven't forgotten about putting out an alpha as agreed at the sprint and
will probably do that once it's in.

One interesting change is that WriteableDirectory has been replaced by
ApplyMonad, and ReadableDirectory has been dropped completely. Grepping
the code, there is no significant code that declares only the
ReadableDirectory constraint, so this doesn't seem like a significant 
loss.

Ganesh