This is an idea that came out of reviewing patch1913,
specifically
"annotate: push inversion down to the prim level"

The type names could be improved - I originally came up
with AnnotateRP thinking of MatchableRP, but they don't
actually have the same structure.

1 patch for repository darcs-unstable@darcs.net:screened:

patch b6bfea77cac3089829da695b8ebc182e5386965f
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Tue Sep 24 19:21:14 BST 2019
  * WIP: remove instances of Annotate RepoPatch

Attachments

• patch-preview.txt
• unnamed
• wip_-remove-instances-of-annotate-repopatch.dpatch

Nice!

> The type names could be improved - I originally came up
> with AnnotateRP thinking of MatchableRP, but they don't
> actually have the same structure.

Yeah, it's not perfectly consistent. But AnnotateRP is concise and
pretty much conveys the idea. We can rename it when we have a better idea.

BTW, you could use AnnotateRP to shorten the constraint for annotateRP :-)

Final version for screened.

It look me a little while to realise how I'd managed
to not use AnnotateRP for annotateRP :-) (it originated from
the default in the Annotate class)

1 patch for repository darcs-unstable@darcs.net:screened:

patch c4872621a4b2e17df32c5a6903088a1867f32b44
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Tue Sep 24 21:42:29 BST 2019
  * remove instances of Annotate for repo patches
  
  They are all expected to use the default anyway, so
  it's better to just encode that directly.

Attachments

• patch-preview.txt
• remove-instances-of-annotate-for-repo-patches.dpatch
• unnamed

This is much better than the default methods and RepoPatch instances 
we had before which I really didn't like but saw no way to avoid.

Turns out this breaks the annotate tests. I didn't run them because
I thought it was clearly safe :-)

I've found the bug after some head-scratching. I should have
been more suspicious about why I could remove the instances
for Named/PatchInfoAnd.

But I want to think a bit more about the right fix, as it
was quite subtle. This quick fix demonstrated what the
problem was, but I am more inclined to promote AnnotateRP
to be a typeclass instead, to avoid any future surprises
like it.

Apologies if the breakage in screened is causing any
trouble.

1 patch for repository darcs-unstable@darcs.net:screened:

patch 540b726979f01607d7f97df89f69a1c09ede385b
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Wed Sep 25 23:36:48 BST 2019
  * Bugfix for "remove instances of Annotate for repo patches"
  
  We were inadvertently calling annotateRP on PatchInfoAnd,
  and omitting the reverseFL that was previously called in
  instance Annotate Named.

Attachments

• bugfix-for-_remove-instances-of-annotate-for-repo-patches_.dpatch
• patch-preview.txt
• unnamed

> I've found the bug after some head-scratching. I should have
> been more suspicious about why I could remove the instances
> for Named/PatchInfoAnd.

I was briefly wondering about that one but didn't investigate. And then
I didn't run the tests either. So I completely failed as reviewer.

We should automate running the tests when we push to reviewed! That will
slow it down considerably but perhaps for reviewed this is okay?

> We should automate running the tests when we push to reviewed! That will
> slow it down considerably but perhaps for reviewed this is okay?

I always run the tests before pushing to reviewed, that's why the
accepted-pending-tests status exists - it means I (or anyone else) can
decouple reading/understanding the code from actually running the tests.

I guess ideally we'd have some kind of CI setup but then we'd have to
maintain it. Nowadays for me running the tests is a single command-line
invocation anyway (cabal v2-run test:darcs-test -- -j8) so a script
wouldn't really add anything.

So the fundamental problem here is that what is actually
going on in Annotate seems a bit complicated.

Going back to before my patch, a "correct" Annotate instance on a
prim would act "forwards" on the annotate state, in the sense that
something like annotate (Hunk old new) takes an annotate state
with old in it and replaces it with new. But then Annotate on a
RepoPatch inverts the effect, so it operates backwards on the annotate 
state, which is actually how annotate is supposed to work in the end.
And there's a reverseFL in the Named instance which is also
important in conjunction with a later mapRL call (and is what I'd lost).

Clearly my refactoring doesn't look so clever now given that it
silently caused a breakage, so one sensible option would be to
simply roll it back completely.

Another option would be to push forward and try to simplify
annotatePIAP that I introduced to replace the old PatchInfoAnd/Named
instances:

  annotatePIAP =
    sequence_ . mapRL annotateRP . reverseFL . patchcontents . hopefully

If you inlined the annotateRP definition and the Annotate RL instance, 
you get:

 sequence_ .
   mapRL (sequence_ . mapRL annotate . invertFL . effect) . reverseFL . 
   patchcontents . hopefully

after a fair amount of experimenting and informal equational reasoning,
I simplified that to:

  sequence_ .
    mapFL annotate . invert . effect .
    patchcontents . hopefully

at least if we don't care about the treatment of conflicts (because
they can have multiple prim patches in the effect of a single repo 
patch).

That final version does pass the two tests with 'annotate' in the name
and seems a lot simpler than any of the logic we had before (once 
disentangled), so I'm inclined to go with it if it passes all the tests. 
But comments welcome.

Longer-term annotate seems like a good use case for more witnesses and/or
an indexed monad.

The annotatePIAP refactor does pass all the tests. Let me
know what you think.

In an ideal world I would write some tests for annotating
conflicts and see if we've regressed (or vice-versa) but I'm
not too keen on spending the effort right now.

2 patches for repository darcs-unstable@darcs.net:screened:

patch 540b726979f01607d7f97df89f69a1c09ede385b
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Wed Sep 25 23:36:48 BST 2019
  * Bugfix for "remove instances of Annotate for repo patches"
  
  We were inadvertently calling annotateRP on PatchInfoAnd,
  and omitting the reverseFL that was previously called in
  instance Annotate Named.

patch 4d368fbb25a148c5cab6d8a19b63629be28cac8f
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Thu Sep 26 16:40:52 BST 2019
  * inline annotateRP and instance Annotate RL and simplify
  
  The new definition should be equivalent on unconflicted
  patches. For conflicts it may not be identical but this
  is probably not a well-supported case anyway and we don't
  have any tests to specify the expected behaviour.

Attachments

• bugfix-for-_remove-instances-of-annotate-for-repo-patches_.dpatch
• patch-preview.txt
• unnamed

>> We should automate running the tests when we push to reviewed! That will
>> slow it down considerably but perhaps for reviewed this is okay?
> 
> I always run the tests before pushing to reviewed, that's why the
> accepted-pending-tests status exists - it means I (or anyone else) can
> decouple reading/understanding the code from actually running the tests.

I understand and this is how I do it, too.

> I guess ideally we'd have some kind of CI setup but then we'd have to
> maintain it. Nowadays for me running the tests is a single command-line
> invocation anyway (cabal v2-run test:darcs-test -- -j8) so a script
> wouldn't really add anything.

I meant on the server as an apply posthook, so we reject applying it in
case someone (like me) one day forgets to run them or gets confused and
thinks they ran them when in fact they didn't. But this means the push
will hang for several minutes while the tests are running.

> I meant on the server as an apply posthook, so we reject applying it in
> case someone (like me) one day forgets to run them or gets confused and
> thinks they ran them when in fact they didn't. But this means the push
> will hang for several minutes while the tests are running.

My feeling is that it'd be more pain than it's worth, particularly as
I've seen at least one test (issue1465_ortryrunning) fail occasionally
even on Linux. But I don't mind if you want to give it a go.

Looks good to me.

> So the fundamental problem here is that what is actually
> going on in Annotate seems a bit complicated.

You name it.

> Going back to before my patch, a "correct" Annotate instance on a
> prim would act "forwards" on the annotate state, in the sense that
> something like annotate (Hunk old new) takes an annotate state
> with old in it and replaces it with new. But then Annotate on a
> RepoPatch inverts the effect, so it operates backwards on the annotate 
> state, which is actually how annotate is supposed to work in the end.
> And there's a reverseFL in the Named instance which is also
> important in conjunction with a later mapRL call (and is what I'd lost).

This became worse due to my patch that pushes the inversion down to the
prim level. Before that we inverted the whole sequence of PatchInfoAnds
inside the command implementation and everything inside D.P.Annotate
worked in "forward mode". (Not too obvious either I guess.)

> Clearly my refactoring doesn't look so clever now given that it
> silently caused a breakage, so one sensible option would be to
> simply roll it back completely.

I don't like this option. Splitting things into separate implementations
for the various patch types in our "stack" only obscures what is going on.

> That final version does pass the two tests with 'annotate' in the name
> and seems a lot simpler than any of the logic we had before (once 
> disentangled), so I'm inclined to go with it if it passes all the tests. 

I agree and have accepted your fix and the simplification.

> Longer-term annotate seems like a good use case for more witnesses and/or
> an indexed monad.

Yes. This applies (no pun intended) to all stateful operations we do
with patches (apply, applyToPaths, annotate). They all suffer from the
rather uninformative type of the monadic operation that changes the
state. States should be indexed with a witness, but how to make that
work with monadically updating the state is an open problem. Perhaps for
things like annotate we can get away with passing the state explicitly
instead of using a state monad.

Trivial follow-up, will self-accept.

1 patch for repository http://darcs.net/screened:

patch 90c63ed246b70c8adb7b0042af061d283c34e982
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Sep 26 23:32:51 CEST 2019
  * remove redundant import in D.P.Annotate

Attachments

• patch-preview.txt
• remove-redundant-import-in-d_p_annotate.dpatch
• unnamed

>   * remove redundant import in D.P.Annotate

Thanks, I completely forgot to remove that when I finished experimenting
with the logic.