darcs

Patch 576 Forward-port of a new annotate implement... (and 1 more)

Title Forward-port of a new annotate implement... (and 1 more)
Superseder Nosy List mornfall
Related Issues
Status accepted Assigned To
Milestone

Created on 2011-04-02.21:04:19 by mornfall, last changed 2011-08-12.11:29:46 by ganesh.

Files
File name Status Uploaded Type Edit Remove
forward_port-of-a-new-annotate-implementation_.dpatch mornfall, 2011-04-02.21:04:19 application/x-darcs-patch
forward_port-of-a-new-annotate-implementation_.dpatch dead mornfall, 2011-04-09.12:07:30 application/x-darcs-patch
forward_port-of-a-new-annotate-implementation_.dpatch dead mornfall, 2011-04-09.15:21:05 application/x-darcs-patch
forward_port-of-a-new-annotate-implementation_.dpatch mornfall, 2011-04-09.15:42:39 application/x-darcs-patch
unnamed mornfall, 2011-04-02.21:04:19 text/x-darcs-patch
unnamed mornfall, 2011-04-02.21:04:19
unnamed mornfall, 2011-04-09.12:07:30 text/x-darcs-patch
unnamed mornfall, 2011-04-09.12:07:30
unnamed mornfall, 2011-04-09.15:21:05 text/x-darcs-patch
unnamed mornfall, 2011-04-09.15:21:05
unnamed mornfall, 2011-04-09.15:42:39 text/x-darcs-patch
unnamed mornfall, 2011-04-09.15:42:39
See mailing list archives for discussion on individual patches.
Messages
msg13873 (view) Author: mornfall Date: 2011-04-02.21:04:19
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
msg13888 (view) Author: ganesh Date: 2011-04-03.22:14:33
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
msg13891 (view) Author: mornfall Date: 2011-04-03.22:34:56
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
msg13918 (view) Author: mornfall Date: 2011-04-09.12:07:30
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
msg13919 (view) Author: mornfall Date: 2011-04-09.13:28:06
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
msg13920 (view) Author: mornfall Date: 2011-04-09.15:21:05
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
msg13921 (view) Author: mornfall Date: 2011-04-09.15:42:39
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
msg13933 (view) Author: ganesh Date: 2011-04-17.21:48:04
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
History
Date User Action Args
2011-04-02 21:04:19mornfallcreate
2011-04-03 22:14:34ganeshsetmessages: + msg13888
title: Forward-port of a new annotate implement... (and 1 more) -> Forward-port of a new annotate implement... (and 1 more)
2011-04-03 22:34:56mornfallsetmessages: + msg13891
2011-04-09 12:07:30mornfallsetfiles: + unnamed, forward_port-of-a-new-annotate-implementation_.dpatch, unnamed
messages: + msg13918
2011-04-09 13:28:06mornfallsetmessages: + msg13919
2011-04-09 15:21:07mornfallsetfiles: + unnamed, forward_port-of-a-new-annotate-implementation_.dpatch, unnamed
messages: + msg13920
2011-04-09 15:42:39mornfallsetfiles: + unnamed, forward_port-of-a-new-annotate-implementation_.dpatch, unnamed
messages: + msg13921
2011-04-17 21:48:05ganeshsetmessages: + msg13933
2011-06-07 06:30:24ganeshsetstatus: needs-review -> accepted-pending-tests
2011-08-12 11:29:46ganeshsetstatus: accepted-pending-tests -> accepted