For discussion for now.

I'm not sure if this is an improvement or not, but I found
the existing comments "inverted [effect,conflict,identity] a
bit confusing. So I reworded it according to my
understanding to see if that also makes sense to others.

Whilst "inverted effect" is accurate in that
it is the inverse of the effect of the Rotcilfnoc, and
perhaps the identity can also be seen as being inverted,
relative to the identity of the Rotcilfnoc, I don't think
it really makes sense to talk about the Rotcilfnoc conflicting
with the inverse of the conflicting patches.

I did also have a quick play with abstracting out the triple
(effect, conflicts, identity) into a separate type to reduce
the duplication, but I don't think it really helps with code
reuse or factoring and it just makes things more verbose.

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

patch 1fe95add7d2b4ba171abafac14fe4bc89232723c
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Sun Aug 11 17:52:58 BST 2019
  * improve doc comments for Rotcilfnoc

Attachments

• improve-doc-comments-for-rotcilfnoc.dpatch
• patch-preview.txt
• unnamed

Your critique is justified in that "invert" is not a defined notion for
a Contexted patch, so the wording "inverted identity" and "inverted
conflicts" has to be taken with a grain of salt.

If you take a sequence of patches that contains a Conflictor and at
least all patches referenced by it, and then invert the whole sequence,
then you get an exact mirror image of the original sequence: the
Rotcilfnoc then conflicts with and references (inverted) patches *in its
future*. In this sense, the "inverted" in the docs made sense (to me).

Now, the whole Conflictor business only plays out well if we impose the
global invariant that all patches in a repo must be positive. The reason
is that otherwise we cannot uphold that common patches of a forking pair
of sequences can be commuted to the left, and without that everything
breaks down.

And in this sense Conflictors really aren't invertible!

I have often wondered if it wouldn't be cleaner if we dropped the Invert
instances for RepoPatches and Named patches. Of course this requires
that we turn CleanMerge into a primitive notion (because at the level of
prim patches, inversion does make sense and is needed to establish the
relation between commute and clean merge), but this is something I want
to do that anyway. The global invariant would no longer be needed then
(all non-prim patches would be positive by definition).

On 12/08/2019 19:46, Ben Franksen wrote:

> I have often wondered if it wouldn't be cleaner if we dropped the Invert
> instances for RepoPatches and Named patches. Of course this requires
> that we turn CleanMerge into a primitive notion (because at the level of
> prim patches, inversion does make sense and is needed to establish the
> relation between commute and clean merge), but this is something I want
> to do that anyway. The global invariant would no longer be needed then
> (all non-prim patches would be positive by definition).

Every so often I play with trying to track positive/negative in the type
system, and then give up when it gets too hard. Yesterday was one such
time :-)

The rough approach I tried was to add an Inverse type family to Invert,
i.e. the inverse of a patch is not necessarily the same type. Then add
constraints all over the place for primitive patches to say that they
actually are self-inverse, and for non-primitive patches try to avoid
that constraint. But it always gets tricky at the boundaries between
those layers.

Simply dropping the Invert instance entirely may well be easier.

Let me know if you're happy with my changed version or would prefer to
keep your original docs.

>> I have often wondered if it wouldn't be cleaner if we dropped the Invert
>> instances for RepoPatches and Named patches. Of course this requires
>> that we turn CleanMerge into a primitive notion (because at the level of
>> prim patches, inversion does make sense and is needed to establish the
>> relation between commute and clean merge), but this is something I want
>> to do that anyway. The global invariant would no longer be needed then
>> (all non-prim patches would be positive by definition).
> 
> Every so often I play with trying to track positive/negative in the type
> system, and then give up when it gets too hard. Yesterday was one such
> time :-)
> 
> The rough approach I tried was to add an Inverse type family to Invert,
> i.e. the inverse of a patch is not necessarily the same type. Then add
> constraints all over the place for primitive patches to say that they
> actually are self-inverse, and for non-primitive patches try to avoid
> that constraint. But it always gets tricky at the boundaries between
> those layers.
> 
> Simply dropping the Invert instance entirely may well be easier.

I hope so. The problematic piece will probably be patch selection. I
believe we display some patches in inverted form e.g. for rollback. I'll
just have to try and see if I can refactor any remaining uses of invert.

> Let me know if you're happy with my changed version or would prefer to
> keep your original docs.

No, I think your's is better, overall. Before you push to screened,
please make sure cabal new-haddock works, I dimly remember that
annotating constructors is still missing, even with a haddock that comes
with recent ghc versions, and results in a syntax error :(  ...and while
we're at it we should check which ghc version we require nowadays, with
extensions like DerivingVia and whatnot. I am using ghc-8.6 since quite
a while, but I think we shouldn't require it if that's not necessary.

> Before you push to screened,
> please make sure cabal new-haddock works, I dimly remember that
> annotating constructors is still missing, even with a haddock that comes
> with recent ghc versions, and results in a syntax error :( 

Thanks for the reminder, I hadn't been paying attention to this. It
turns out that the haddock I was using that comes with GHC 8.2 indeed
doesn't support annotating GADT constructors, so even the existing docs
don't work. But the haddock with GHC 8.6 does work, and both the
existing docs and my new docs are ok (The problem was fixed about a year
ago : https://github.com/haskell/haddock/pull/709)

> ...and while
> we're at it we should check which ghc version we require nowadays, with
> extensions like DerivingVia and whatnot. I am using ghc-8.6 since quite
> a while, but I think we shouldn't require it if that's not necessary.

Re GHC versions, as mentioned I'm using GHC 8.2. The minimum version of
base we support is 4.10 which is what came with 8.2, so I think we're
fine. That said, I have no objection to bumping up our minimum
dependency to 8.4 or 8.6 if a reason to do so emerges. If doing so we
should probably have one eye on our planned release timing, as we don't
want to make the latest released version too difficult to install.

>> Simply dropping the Invert instance entirely may well be easier.
> 
> I hope so. The problematic piece will probably be patch selection. I
> believe we display some patches in inverted form e.g. for rollback.

Indeed this is the only real use of invert above the Darcs.Patch layer.
Whenever we select patches starting at the opposite end relative to the
final position of those we select, we first invert the whole input
sequence and afterwards invert everything back. This is very bad for
performance and only needed because the PatchChoices type has this
inherent asymmetry. Fixing this asymmetry has been on my todo list for a
long time. I know exactly what to do here but every time I start doing
it I become overwhelmed with the amount of changes that are required to
pull this through.

On 14/08/2019 20:29, Ben Franksen wrote:
> 
> Ben Franksen <ben.franksen@online.de> added the comment:
> 
>>> Simply dropping the Invert instance entirely may well be easier.
>>
>> I hope so. The problematic piece will probably be patch selection. I
>> believe we display some patches in inverted form e.g. for rollback.
> 
> Indeed this is the only real use of invert above the Darcs.Patch layer.
> Whenever we select patches starting at the opposite end relative to the
> final position of those we select, we first invert the whole input
> sequence and afterwards invert everything back. This is very bad for
> performance and only needed because the PatchChoices type has this
> inherent asymmetry. Fixing this asymmetry has been on my todo list for a
> long time. I know exactly what to do here but every time I start doing
> it I become overwhelmed with the amount of changes that are required to
> pull this through.

Could we just define an Invertible type to restore inverses with
explicit wrapping?

data Invertible p wX wY where
   Fwd :: p wX wY -> Invertible p wX wY
   Rev :: p wX wY -> Invertible p wY wX

instance Invert (Invertible p) where
  invert (Fwd p) = Rev p
  invert (Rev p) = Fwd p

One difficulty is what instances to define and how to implement them for
the 'Rev' case or whether to just use an error there.

The rather hacky RebaseSelect/RebaseChanges types in
Darcs.Patch.Rebase.Viewing are in a similar position of needing to
implement a fake inverse.

>>>> Simply dropping the Invert instance entirely may well be easier.
>>>
>>> I hope so. The problematic piece will probably be patch selection. I
>>> believe we display some patches in inverted form e.g. for rollback.
>>
>> Indeed this is the only real use of invert above the Darcs.Patch layer.
> 
> Could we just define an Invertible type to restore inverses with
> explicit wrapping?
> 
> data Invertible p wX wY where
>    Fwd :: p wX wY -> Invertible p wX wY
>    Rev :: p wX wY -> Invertible p wY wX
> 
> instance Invert (Invertible p) where
>   invert (Fwd p) = Rev p
>   invert (Rev p) = Fwd p

This is an interesting idea. If we can limit the wrapping and unwrapping
to SelectChanges it would not be too invasive either.

It's still a temporary measure. Our final goal should be to make
PatchChoices symmetric, so it becomes irrelevant from which end we start
to traverse, and then remove the inversion from SelectChanges. At that
point we'd no longer need the Invertible type and its instances.

> One difficulty is what instances to define and how to implement them for
> the 'Rev' case or whether to just use an error there.

I think instances should be added only as needed. Let the type checker
tell us which instances are missing and add those and only those.

> The rather hacky RebaseSelect/RebaseChanges types in
> Darcs.Patch.Rebase.Viewing are in a similar position of needing to
> implement a fake inverse.

Yes. I think refactoring these types (i.e removing their Rev
constructors) would be a good test bed for a refactor with a new
Invertible type. We'll see then how that works out.

On the other hand, I think we currently have no pressing need to remove
Invert instances for the higher level patch types. And I am not yet 100%
sure removing Invert is the right thing from a theoretical POV (see my
comments to the Theory patch I sent).