This bundle cleans up various weird behaviour in the patch parser,
that would often hide parse errors.

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

Fri Oct 22 06:49:00 BST 2010  Ganesh Sittampalam <ganesh@earth.li>
  * don't silently throw away remaining parse input

Fri Oct 22 06:53:59 BST 2010  Ganesh Sittampalam <ganesh@earth.li>
  * drop the unused and often ignored "want eof" parameter to readPatch'

Fri Oct 22 06:58:29 BST 2010  Ganesh Sittampalam <ganesh@earth.li>
  * drop the nested Maybes in the patch parser

Attachments

• don_t-silently-throw-away-remaining-parse-input.dpatch
• unnamed

This patch bundle (with 3 patches) was just applied to the repository http://darcs.net/screened.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_screened.html#bundle-431d04abc3f4c56e9fed25dac731894355846800

Hmm, there's a conflict here with unstable, not sure what (issue1911 
dogfood).  Is there a resolution floating in the tracker somewhere by 
chance?

On Fri, 10 Dec 2010, Eric Kow wrote:

> Eric Kow <kowey@darcs.net> added the comment:
>
> Hmm, there's a conflict here with unstable, not sure what (issue1911
> dogfood).  Is there a resolution floating in the tracker somewhere by
> chance?

[Sun Nov 21 18:46:59 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
   * resolve conflict between patch reading changes and case -> maybe cleanup]

Ganesh

I guess it would have been more helpful to have sent the patch itself to
the tracker from the beginning, but I don't in general have a good workflow
for that - I just resolve conflicts in screened as they arise. Anyway,
here it is:

1 patch for repository /home/ganesh/darcs-temp2:

Sun Nov 21 18:46:59 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
  * resolve conflict between patch reading changes and case -> maybe cleanup

Attachments

• resolve-conflict-between-patch-reading-changes-and-case-__-maybe-cleanup.dpatch
• unnamed

On Sat, Oct 23, 2010 at 21:01:34 +0000, Ganesh Sittampalam wrote:
> Fri Oct 22 06:49:00 BST 2010  Ganesh Sittampalam <ganesh@earth.li>
>   * don't silently throw away remaining parse input
> 
> Fri Oct 22 06:53:59 BST 2010  Ganesh Sittampalam <ganesh@earth.li>
>   * drop the unused and often ignored "want eof" parameter to readPatch'
> 
> Fri Oct 22 06:58:29 BST 2010  Ganesh Sittampalam <ganesh@earth.li>
>   * drop the nested Maybes in the patch parser

This looks good.  I hope there wasn't anything subtle/tricky in there,
because I certainly didn't spot it.

I reviewed the three and then glanced through the full diff with
conflict resolution.  All pushed.

don't silently throw away remaining parse input
-----------------------------------------------
These are the active ingredients of the patch (Darcs.Patch.Read)

> -readPatch :: ReadPatch p => B.ByteString -> Maybe (Sealed (p C(x )), B.ByteString)
> -readPatch ps = case parseStrictly (readPatch' False) ps of
> -                   Just (Just p, ps') -> Just (p, ps')
> -                   _ -> Nothing
> +readPatchPartial :: ReadPatch p => B.ByteString -> Maybe (Sealed (p C(x )), B.ByteString)
> +readPatchPartial ps
> +    = case parseStrictly (readPatch' False) ps of
> +         Just (Just p, ps') -> Just (p, ps')
> +         _ -> Nothing

Just a rename and whitespace change.

> hunk ./src/Darcs/Patch/Read.hs 59
> +readPatch :: ReadPatch p => B.ByteString -> Maybe (Sealed (p C(x )))
> +readPatch ps
> +    = case readPatchPartial ps of
> +         Just (p, ps') | B.null (dropSpace ps') -> Just p
> +         _ -> Nothing

And the new behaviour.  Before we were using readPatchPartial everywhere
(sigh!) and now we're a bit more careful.  In cases where we aren't
expecting leftovers other than whitespace, parsing should fail if we do
spot it.

I'm sort of thinking of this patch as correcting for confirmation bias
("I'm thinking of a rule. The sequence 2, 4, 6 follows my rule..."
http://www.devpsy.org/teaching/method/confirmation_bias.html ). [Or
maybe there's some other psych phenomenon that's more appropriate here].
So now instead of just being happy the parser succeeds, we're also more
carefully making sure it isn't succeeding for the wrong reasons.

...

For the most part, the rest of this patch switches over to readPatch.
(If we're wrong here, the consequences will be darcs complaining about
not being able to read some patch file).  I didn't put any thought into
making sure we weren't overshooting though, sorry! It seemed to make
sense at a glance.

The two cases where we still let ourselves using readPatchPartial are
Darcs.Patch.Bundle (that really needs to be renamed PatchBundle)
getPatches and parsePatches, which I guess makes sense because patch
bundles have lots of stuff after the patches themselves.

drop the unused and often ignored "want eof" parameter to readPatch'
--------------------------------------------------------------------
Skipping down to the active ingredients (and ignoring the
straightforward consequences of said ingredients)

>  instance ReadPatch p => ReadPatch (Braced p) where
> -    readPatch' eof =
> -       do mps <- bracketedFL (readPatch' False) '{' '}'
> +    readPatch' =
> +       do mps <- bracketedFL readPatch' '{' '}'
>            case mps of
> hunk ./src/Darcs/Patch/Read.hs 69
> -            Just (Sealed ps) -> do when eof lexEof
> -                                   return $ Just $ Sealed $ Braced ps
> -            Nothing -> liftM (fmap (mapSeal Singleton)) $ readPatch' eof
> +            Just (Sealed ps) -> return $ Just $ Sealed $ Braced ps
> +            Nothing -> liftM (fmap (mapSeal Singleton)) $ readPatch'

There's little bits and pieces of code in these parsers like that which
optionally eats whitespace + eof... only we *never* use it.

All the readPatch' calls either thread their want_eof through, or just
pass False.  Maybe YAGNI violation, maybe cruft, doesn't matter.  Gone!

drop the nested Maybes in the patch parser
------------------------------------------
This one was a little trickier for me to review, but it seems good.
I'm always happier to see code become simpler/more transparent.

(Wikibooks reminded me of MonadPlus)

>  instance (ReadPatch p, PatchListFormat p) => ReadPatch (PatchInfoAnd p) where
> -    readPatch' =
> -        do x <- readPatch'
> -           case x of
> -             Just (Sealed p) -> return $ Just $ Sealed $ n2pia p
> -             Nothing -> return Nothing
> +    readPatch' = mapSeal n2pia <$> readPatch'

I'll ignore the bits of the code like this which are just consequences
of less double-layer Maybe bureaucracy to deal with

> hunk ./src/Darcs/Patch/Read.hs 53
>      readPatch'
> -        :: ParserM m => m (Maybe (Sealed (p C(x ))))
> +        :: ParserM m => m (Sealed (p C(x )))

Here's our big change.  I'm slightly concerned about inner Nothing
meaning something important, but it looks like we address that below.

>  instance ReadPatch p => ReadPatch (Braced p) where
>      readPatch' =
> -       do mps <- bracketedFL readPatch' '{' '}'
> +       do mps <- (Just <$> bracketedFL readPatch' '{' '}') <|> return Nothing

Here's the whole thing again

| instance ReadPatch p => ReadPatch (Braced p) where
|     readPatch' =
|        do mps <- (Just <$> bracketedFL readPatch' '{' '}') <|> return Nothing
|           case mps of
|             Just (Sealed ps) -> return $ Sealed $ Braced ps
|             Nothing -> mapSeal Singleton <$> readPatch'

OK, now this is a more interesting bit, not just bureaucracy removal.
The way I currently understand it is

BEFORE: functions like bracketedFL used to give (Just Nothing) on
        failure rather than Nothing, which I sort of read as "it's OK
        for this parsing to fail; we'll just tell you it happened
        instead of stopping".

AFTER: such functions just give us plain old Nothing

However the code above still assumes we're using this permission-to-fail
model and doing something on said failure (ie. handling Nothing).  So we
basically we have to restore this permission-to-fail by giving return
Nothing as an alternative to the parser succeeding.

So
 1) ps <- bracketedFL readPatch' ...
    - just failing if bracketedFL fails, bad
 2) mps <- Just <$> bracketedFL readPatch' ...
    - worse, now we're pretending we handle the failure
 3) mps <- Just <$> bracketedFL readPatch ... <|> return Nothing
    - much better

If I've understood correctly, the following formulations should be equivalent.

|A| instance ReadPatch p => ReadPatch (Braced p) where
|A|     readPatch' =
|A|        do mps <- (Just <$> bracketedFL readPatch' '{' '}') <|> return Nothing
|A|           case mps of
|A|             Just (Sealed ps) -> return $ Sealed $ Braced ps
|A|             Nothing -> mapSeal Singleton <$> readPatch'

|B| instance ReadPatch p => ReadPatch (Braced p) where
|B|     readPatch' =
|B|             (mapSeal Braced <$> bracketedFL readPatch' '{' '}')
|B|         <|> (mapSeal Singleton <$> readPatch')

Not actually proposing B (though it does seem a bit nicer)
Just cross-checking to make sure I've understood.

> hunk ./src/Darcs/Patch/Read.hs 100
> -    peekforc pre bfl (return Nothing)
> +    peekforc pre bfl mzero

I think that's one example of withdrawing that permission-to-fail
  
>     = skipSpace >> choice
>       [ return' $ readHunk x
> hunk ./src/Darcs/Patch/Read.hs 133
>       , return' $ readTok x
>       , return' $ readBinary x
>       , return'   readIdentity
> -     , liftM Just $ readSplit x
> +     , readSplit x
>       , return' $ readChangePref
> hunk ./src/Darcs/Patch/Read.hs 135
> -     , return Nothing

And that's another case

The rest of it is just more of the same.

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
For a faster response, try +44 (0)1273 64 2905 or
xmpp:kowey@jabber.fr (Jabber or Google Talk only)

On Sat, Dec 11, 2010 at 22:24:40 +0000, Eric Kow wrote:
> |B| instance ReadPatch p => ReadPatch (Braced p) where
> |B|     readPatch' =
> |B|             (mapSeal Braced <$> bracketedFL readPatch' '{' '}')
> |B|         <|> (mapSeal Singleton <$> readPatch')
> 
> Not actually proposing B (though it does seem a bit nicer)
> Just cross-checking to make sure I've understood.

Oh! And that's what your simplify readPatch' for Braced in that
other bundle does, now pushed.  Hooray, I understood something!

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
For a faster response, try +44 (0)1273 64 2905 or
xmpp:kowey@jabber.fr (Jabber or Google Talk only)

This patch bundle (with 3 patches) was just applied to the repository http://darcs.net/.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_.html#bundle-431d04abc3f4c56e9fed25dac731894355846800

This patch bundle (with 1 patches) was just applied to the repository http://darcs.net/.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_.html#bundle-da2deed8326afe57032ddd4cc88e6ca7659de9f5

This patch bundle (with 3 patches) was just applied to the repository http://darcs.net/reviewed.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_reviewed.html#bundle-431d04abc3f4c56e9fed25dac731894355846800

This patch bundle (with 1 patches) was just applied to the repository http://darcs.net/reviewed.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_reviewed.html#bundle-da2deed8326afe57032ddd4cc88e6ca7659de9f5