This bundle *almost* gets us rid of all Invert instances for non-prim patch
types, using your (Ganesh's) idea of an Invertible wrapper in a few places
(e.g. for patch selection).

The only remaining stumbling block is the use of invert for the underlying
RepoPatch in Rebase.Viewing to implement the force-commute. I think
patch1911 may be the way to get rid of that last use of invert for
non-prims.

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

patch cfc73aaf7cb42795fd6abec946ace2aa20147570
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Aug 29 18:39:18 CEST 2019
  * re-export all imported classes (with members) from Darcs.Patch.RepoPatch

patch 6c34c6f7d916c1bf22cd806b8160841c8604e652
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Aug 30 10:04:59 CEST 2019
  * matching a patch should be invariant under inversion
  
  This adds a property (only in the haddocks) to D.P.Match.matchAPatch and
  removes re-inversion of patches when we apply a match criterion from
  D.UI.SelectChanges.

patch 2b18986569ec2db53f2f21b3e74ac9ebf390b89d
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Aug 30 10:15:13 CEST 2019
  * rename repr to reInvert and fix its haddocks

patch 6b263ce0e12e8f81dd25ea721139a609f756c375
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Aug 31 00:19:22 CEST 2019
  * remove Invert constraint from Matchable and MatchableRP
  
  This means we need to add it to a a few function that actually require it.
  We do this as a preparation for eventually removing Invert instances from
  all the higher level patch types.

patch 24da2bf06a2444a6b4e0b26caae85cc4c41d1041
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Aug 30 15:47:44 CEST 2019
  * add Darcs.Patch.Invertible
  
  This is a wrapper type to make an arbirary patch type formally invertible.
  We define only instances that will be needed to statisfy Invert constraints
  that are currently required in the Repository and UI subsystem. Many of the
  class methods defined for Invertible assume the patch is actually positive.

patch 10c5d5a64582d54ac764713c7fd5a687dea61d4b
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Aug 30 15:48:25 CEST 2019
  * use Invertible to generalize runSelection
  
  The new runSelection no longer requires an Invert instance for the patches.
  This is done by wrapping them internally with Invertible. We keep the old
  function under the new name runInvertibleSelection so we can use it for
  selecting prims, since these are naturally invertible and we cannot and will
  not use Splitters with wrapped Invertible patches.

patch bdafe71437bc257d756729462e94a3082d69b1e4
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Aug 30 23:56:32 CEST 2019
  * use Invertible when calling lookTouch in log command
  
  We call lookTouch with the patches inverted. To get rid of the Invert
  constraint, we wrap the patch as an Invertible patch.

patch 6e72aa1695ffdc7fd5430cd1748d2d5424e1651d
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Sep  1 22:03:34 CEST 2019
  * use showPatch ForStorage in V3 error messages

patch 19e6f76fc9146cca5c615ac78bb7118aa540de34
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Aug 30 20:33:49 CEST 2019
  * re-add Effect instances for RebaseFixup, RebaseChange, and RebaseSelect
  
  They are needed so that we can wrap RebaseSelect and RebaseChange with
  Invertible and get a useful Apply instance.

patch 9c05261a68e98fb0cf36ffaddee08aff06df37a1
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Sep 10 10:01:26 CEST 2019
  * remove reverse constructors from RebaseSelect and RebaseChange

patch c1be3d805cadcee2e7707521bdbdebb5d7ce360f
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Sep 10 10:17:42 CEST 2019
  * remove bogus ReadPatch instances for RebaseSelect and RebaseChange

patch 97ab5fb888bc9c670ef78b57131a1edf20b840ad
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Sep 10 12:01:55 CEST 2019
  * remove superclass Commute from class Merge

patch 3553e65674aa8598d0b0d4c5b331aadffe4ff504
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Sep 10 11:52:24 CEST 2019
  * remove lots of redundant constraints

patch 6a37fd022710f904ddb06ae7f141b44fed770102
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Sep 10 12:19:44 CEST 2019
  * avoid redundant constraints warnings for default methods of FromPrim

patch b6b1dd872766c9288ff42040ffcbfed60190c698
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Jul 30 10:36:58 CEST 2019
  * add class CleanMerge & make it super class of Merge
  
  This does not yet replace CommuteNoConflicts. Instead, instances for
  CleanMerge are, for now, defined in terms of mergeNoConflicts.

patch 7f84e5d32e24959f255cc2ea3b0f80015d8a41d8
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Jul 30 12:41:43 CEST 2019
  * replace CommuteNoConflicts with CleanMerge for prim patch types
  
  This patch also cleans up and clarifies instances for Merge and Cleanmerge:
  We no longer supply a default definition for cleanMergeFLs and mergeFLs,
  instead these must be defined explicitly. This avoids accidentally using the
  default version when this is not appropriate. We also explicitly call error
  in definitions of cleanMerge and merge if the patch type has an Ident
  instance and we try to merge two identical patches, since this is an
  undefined operation.

patch 25f924168aef2d93a05440b1bc00ef8157b1ab7b
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Sep 10 23:01:17 CEST 2019
  * use cleanMerge to implement partitionConflictingFL
  
  In order to simplify this change, it no longer takes a commuter as argument
  but only works with plain FLs. This necessitates an upstream change to
  filterOutConflicts, which while (we're at it) now gets the repositoiry
  argument first, like all similar functions.

patch 14f60a26b6a5860e8ab65eca367e6d3a4a141065
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Sep 10 23:18:25 CEST 2019
  * remove CommuteNoConflicts from RepoPatch

patch 94bc236a6c76797cabae1eab172e04b0c1c1638e
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Sep 11 08:46:53 CEST 2019
  * annotate: push inversion down to the prim level
  
  We must take care that we call the annotate method only on inverted prims
  and that we traverse the history (of prims) in reverse order. To avoid
  mistakes when defining instances (outside of Darcs.Patch.Annotate), we now
  use a default method with a default signature.

patch 1304fcdd16ad9e42115ca0b81f207fb589faa9ee
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Sep 11 10:56:08 CEST 2019
  * use effect to avoid inversion of non-prims in externalResolution

patch 223bc3a128290b303d0dae6ef70ebef314cba85b
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Sep 11 08:53:14 CEST 2019
  * add unapply method to class Apply
  
  This allows us to eliminate direct use of inversion for the named patch
  types (Named, PatchInfoAnd) in all of Darcs.Repository and Darcs.UI. It
  also allows us to clean up the Apply instance for Invertible, which no
  longer has to call applyPrimFL on the effect.

patch ee466763d0522249509c0102971f2bce58f37588
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Sep 11 09:33:46 CEST 2019
  * eliminate Invert instances for Named, WrappedNamed, and PatchInfoAnd

patch 7f2e2d6d85e16efd9773c82a97a177147ef4fb16
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Sep 11 11:40:47 CEST 2019
  * rename patches_context selection_context in rebase command

Attachments

• patch-preview.txt
• re_export-all-imported-classes-_with-members_-from-darcs_patch_repopatch.dpatch
• unnamed

I should note that this is not for screened yet.

An updated bundle. Sorry for the many patches. It passes all tests and now
actually removes the Rotcilfnoc constructor and the Invert instance for
RepoPatchV3 (but not for V1 or V2).

It is still not for screened yet, but can serve as a basis for discussion.
There are a number of conflicts with intermediate work and I chose not to
rebase my patches but resolve the conflicts, because that more reliably
avoids loosing changes. But I can rebase the new patches now that things
have settled a bit.

There are a number of changes to Darcs.Patch.Rebase.* which probably
conflict with ongoing work of yours.

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

patch cfc73aaf7cb42795fd6abec946ace2aa20147570
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Aug 29 18:39:18 CEST 2019
  * re-export all imported classes (with members) from Darcs.Patch.RepoPatch

patch 6c34c6f7d916c1bf22cd806b8160841c8604e652
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Aug 30 10:04:59 CEST 2019
  * matching a patch should be invariant under inversion
  
  This adds a property (only in the haddocks) to D.P.Match.matchAPatch and
  removes re-inversion of patches when we apply a match criterion from
  D.UI.SelectChanges.

patch 2b18986569ec2db53f2f21b3e74ac9ebf390b89d
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Aug 30 10:15:13 CEST 2019
  * rename repr to reInvert and fix its haddocks

patch 6b263ce0e12e8f81dd25ea721139a609f756c375
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Aug 31 00:19:22 CEST 2019
  * remove Invert constraint from Matchable and MatchableRP
  
  This means we need to add it to a a few function that actually require it.
  We do this as a preparation for eventually removing Invert instances from
  all the higher level patch types.

patch 24da2bf06a2444a6b4e0b26caae85cc4c41d1041
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Aug 30 15:47:44 CEST 2019
  * add Darcs.Patch.Invertible
  
  This is a wrapper type to make an arbirary patch type formally invertible.
  We define only instances that will be needed to statisfy Invert constraints
  that are currently required in the Repository and UI subsystem. Many of the
  class methods defined for Invertible assume the patch is actually positive.

patch 10c5d5a64582d54ac764713c7fd5a687dea61d4b
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Aug 30 15:48:25 CEST 2019
  * use Invertible to generalize runSelection
  
  The new runSelection no longer requires an Invert instance for the patches.
  This is done by wrapping them internally with Invertible. We keep the old
  function under the new name runInvertibleSelection so we can use it for
  selecting prims, since these are naturally invertible and we cannot and will
  not use Splitters with wrapped Invertible patches.

patch bdafe71437bc257d756729462e94a3082d69b1e4
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Aug 30 23:56:32 CEST 2019
  * use Invertible when calling lookTouch in log command
  
  We call lookTouch with the patches inverted. To get rid of the Invert
  constraint, we wrap the patch as an Invertible patch.

patch 6e72aa1695ffdc7fd5430cd1748d2d5424e1651d
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Sep  1 22:03:34 CEST 2019
  * use showPatch ForStorage in V3 error messages

patch 19e6f76fc9146cca5c615ac78bb7118aa540de34
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Aug 30 20:33:49 CEST 2019
  * re-add Effect instances for RebaseFixup, RebaseChange, and RebaseSelect
  
  They are needed so that we can wrap RebaseSelect and RebaseChange with
  Invertible and get a useful Apply instance.

patch 9c05261a68e98fb0cf36ffaddee08aff06df37a1
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Sep 10 10:01:26 CEST 2019
  * remove reverse constructors from RebaseSelect and RebaseChange

patch c1be3d805cadcee2e7707521bdbdebb5d7ce360f
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Sep 10 10:17:42 CEST 2019
  * remove bogus ReadPatch instances for RebaseSelect and RebaseChange

patch 97ab5fb888bc9c670ef78b57131a1edf20b840ad
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Sep 10 12:01:55 CEST 2019
  * remove superclass Commute from class Merge

patch 3553e65674aa8598d0b0d4c5b331aadffe4ff504
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Sep 10 11:52:24 CEST 2019
  * remove lots of redundant constraints

patch 6a37fd022710f904ddb06ae7f141b44fed770102
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Sep 10 12:19:44 CEST 2019
  * avoid redundant constraints warnings for default methods of FromPrim

patch b6b1dd872766c9288ff42040ffcbfed60190c698
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Jul 30 10:36:58 CEST 2019
  * add class CleanMerge & make it super class of Merge
  
  This does not yet replace CommuteNoConflicts. Instead, instances for
  CleanMerge are, for now, defined in terms of mergeNoConflicts.

patch 7f84e5d32e24959f255cc2ea3b0f80015d8a41d8
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Jul 30 12:41:43 CEST 2019
  * replace CommuteNoConflicts with CleanMerge for prim patch types
  
  This patch also cleans up and clarifies instances for Merge and Cleanmerge:
  We no longer supply a default definition for cleanMergeFLs and mergeFLs,
  instead these must be defined explicitly. This avoids accidentally using the
  default version when this is not appropriate. We also explicitly call error
  in definitions of cleanMerge and merge if the patch type has an Ident
  instance and we try to merge two identical patches, since this is an
  undefined operation.

patch 25f924168aef2d93a05440b1bc00ef8157b1ab7b
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Sep 10 23:01:17 CEST 2019
  * use cleanMerge to implement partitionConflictingFL
  
  In order to simplify this change, it no longer takes a commuter as argument
  but only works with plain FLs. This necessitates an upstream change to
  filterOutConflicts, which while (we're at it) now gets the repositoiry
  argument first, like all similar functions.

patch 14f60a26b6a5860e8ab65eca367e6d3a4a141065
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Sep 10 23:18:25 CEST 2019
  * remove CommuteNoConflicts from RepoPatch

patch 94bc236a6c76797cabae1eab172e04b0c1c1638e
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Sep 11 08:46:53 CEST 2019
  * annotate: push inversion down to the prim level
  
  We must take care that we call the annotate method only on inverted prims
  and that we traverse the history (of prims) in reverse order. To avoid
  mistakes when defining instances (outside of Darcs.Patch.Annotate), we now
  use a default method with a default signature.

patch 1304fcdd16ad9e42115ca0b81f207fb589faa9ee
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Sep 11 10:56:08 CEST 2019
  * use effect to avoid inversion of non-prims in externalResolution

patch 223bc3a128290b303d0dae6ef70ebef314cba85b
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Sep 11 08:53:14 CEST 2019
  * add unapply method to class Apply
  
  This allows us to eliminate direct use of inversion for the named patch
  types (Named, PatchInfoAnd) in all of Darcs.Repository and Darcs.UI. It
  also allows us to clean up the Apply instance for Invertible, which no
  longer has to call applyPrimFL on the effect.

patch ee466763d0522249509c0102971f2bce58f37588
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Sep 11 09:33:46 CEST 2019
  * eliminate Invert instances for Named, WrappedNamed, and PatchInfoAnd

patch 7f2e2d6d85e16efd9773c82a97a177147ef4fb16
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Sep 11 11:40:47 CEST 2019
  * rename patches_context selection_context in rebase command

patch 72c8dde9f22dc8458571ffafc5480c56f8b2284b
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Sep 11 22:31:07 CEST 2019
  * resolve conflicts after 58e71a53c8c1964ee639e0c10418ebec28a92879

patch ea1228812cb4d98414211a4ff2c6b6c4eae421af
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Sep 11 22:41:04 CEST 2019
  * remove a few more redundant constraints

patch d1135c89f45516f6fe31f6fcf16f6875cea1ab1f
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Sep 12 10:58:41 CEST 2019
  * cleanups in log command

patch 5c08c3e5b60aed8920530881127a32fc1cd1d8ca
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Sep 12 11:08:46 CEST 2019
  * WIP unify RebaseChange and RebaseSelect

patch e4ca42f6fb456eb3ffcf9a961c7825c94efd2ef6
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Sep 11 14:58:53 CEST 2019
  * remove patch type parameter to RebaseName
  
  Also remove its Apply and PrimPatchBase instances.

patch 83c31f85f7f746ac93f04d512e1815ab57fe2357
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Sep 11 15:58:38 CEST 2019
  * RebaseFixup take a prim as type argument, not a RepoPatch

patch b10f3dcb5ecc1f24505fe2c9c748cebce25fd1bf
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Sep 12 12:17:23 CEST 2019
  * resolve conflicts after 83c31f85f7f746ac93f04d512e1815ab57fe2357

patch 185a844f75614fbaa5978fd37081d39f75e82ca4
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Sep 12 12:25:09 CEST 2019
  * replace complicated constraints with RepoPatch

patch e3ec894125c5d079a4a53f9ea9ff2d6cffa139a4
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Sep 12 16:21:36 CEST 2019
  * get rid of the last Invert constraints for RepoPatch in rebase

patch 515d711ba88ef7acf14bd83ce4a8947e6a1e0d04
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Sep 12 16:39:23 CEST 2019
  * remove Invert from RepoPatch constraint synonym
  
  This also disables tests for inverses of RepoPatches and fixes the
  permutivity test to no longer require it.

patch 3393227398043a389f8736cddb45b9f6f873c68b
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Sep 12 17:29:54 CEST 2019
  * WIP add fullUnwind method to class Conflict

patch 6d4a5841d16d387eaa6348f73dff44247d0bf175
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Jul 30 23:56:25 CEST 2019
  * v3: simplify merge of two Conflictors
  
  The simplification consists in using cleanMerge instead of re-implementing
  it in terms of commuteRLFL.

patch d7450b3c64a6ce1fc70e3507b2d88f056d345f18
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Sep 10 22:24:39 CEST 2019
  * remove two unneeded constraints

patch 432438798e2bb1b8851722ee751f4b82bfb5df60
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Aug 30 10:04:30 CEST 2019
  * add TODO items to Darcs.Patch.Inspect

patch 91d85f940bafa1343183ee936cd2d99aa091e47a
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Sep  3 00:04:50 CEST 2019
  * fix broken layout of Darcs.Patch.Named.readNamed

patch 416474a03456216ff0651f51a87b5f6f0063822c
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Sep 15 20:12:21 CEST 2019
  * resolve conflicts with 930c67427998afb9099a5dd479482e087d5b4ef3

patch fb380c073a68498a27a1fb2603fb34d191b18fa0
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Sep 15 20:18:41 CEST 2019
  * resolve conflicts after c63580de8d04b2b2205412597b359fd74a031642

patch 1124e7713b461dad97532a5bb4719d04d415db44
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Aug  1 17:41:24 CEST 2019
  * define instance CleanMerge RepoPatchV3 directly
  
  This means we can use it the to define the instance CommuteNoConflicts for
  the Rotcilfnoc :> Conflictor case, eliminating the only remaining place
  where we do non-trivial computation with the internals of a Rotcilfnoc.

patch ded0b27ac79f2ee81a8de273687c4abc18d3ffb6
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Sep 15 22:19:33 CEST 2019
  * remove Rotcilfnoc constructor and Invert instance for RepoPatchV3

patch 3694d7f5429f30cb75f657129869d81c5370dd24
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Sep 15 22:32:32 CEST 2019
  * RepoPatchV3: replace some uses of x, y with p, q
  
  We want to reserve these variable names for the conflicts in a conflictor.

Attachments

• patch-preview.txt
• re_export-all-imported-classes-_with-members_-from-darcs_patch_repopatch.dpatch
• unnamed

On 15/09/2019 21:51, Ben Franksen wrote:

> Sorry for the many patches.

Lots of smaller patches is great actually, when they have independent
meaning it makes reviewing much easier.

> It is still not for screened yet, but can serve as a basis for discussion.
> There are a number of conflicts with intermediate work and I chose not to
> rebase my patches but resolve the conflicts, because that more reliably
> avoids loosing changes. But I can rebase the new patches now that things
> have settled a bit.

How about starting to push the patches that are ready for screened, also
putting them in chunks in separate patches on the tracker? Once
dependencies are in screened, it makes it a lot easier to think about
what's ready next.

For example these preparatory patches all look fine to me (i.e. I've
fully reviewed them):

> * re-export all imported classes (with members) from Darcs.Patch.RepoPatch
> * matching a patch should be invariant under inversion
> * rename repr to reInvert and fix its haddocks
> * remove Invert constraint from Matchable and MatchableRP
> * use showPatch ForStorage in V3 error messages

Once we get to actually adding Invertible, it looks fine to me except
perhaps the 'Apply' instance. If you can push that together with the
introductionof 'unapply', it'd be great, but even if we have to wait I
don't think it's a big problem.

> There are a number of changes to Darcs.Patch.Rebase.* which probably
> conflict with ongoing work of yours.

That's fine. Most of my work is quite preliminary still, especially now
it turns out I can't change the underlying rebase representation to a
prim without causing a regression. Let's merge anything that moves us in
the direction we want to go merged, and then see what's left to do.

>> It is still not for screened yet, but can serve as a basis for discussion.
>> There are a number of conflicts with intermediate work and I chose not to
>> rebase my patches but resolve the conflicts, because that more reliably
>> avoids loosing changes. But I can rebase the new patches now that things
>> have settled a bit.
> 
> How about starting to push the patches that are ready for screened, also
> putting them in chunks in separate patches on the tracker? Once
> dependencies are in screened, it makes it a lot easier to think about
> what's ready next.
> 
> For example these preparatory patches all look fine to me (i.e. I've
> fully reviewed them):
> 
>> * re-export all imported classes (with members) from Darcs.Patch.RepoPatch
>> * matching a patch should be invariant under inversion
>> * rename repr to reInvert and fix its haddocks
>> * remove Invert constraint from Matchable and MatchableRP
>> * use showPatch ForStorage in V3 error messages
> 
> Once we get to actually adding Invertible, it looks fine to me except
> perhaps the 'Apply' instance. If you can push that together with the
> introductionof 'unapply', it'd be great, but even if we have to wait I
> don't think it's a big problem.

I think it makes the most sense to re-order these patches i.e. first add
unapply then add the Apply instance for Invertible. That means some
rebasing but I think I'll want to do that anyway before I screen.

>> There are a number of changes to Darcs.Patch.Rebase.* which probably
>> conflict with ongoing work of yours.
> 
> That's fine. Most of my work is quite preliminary still, especially now
> it turns out I can't change the underlying rebase representation to a
> prim without causing a regression. Let's merge anything that moves us in
> the direction we want to go merged, and then see what's left to do.

Okay. In the beginning I was quite uncertain about the whole idea.

One reason is that I wanted to structure patch theory along the notion
of groupoids and not being able to invert patches makes that impossible.

But I have came to the conclusion that loosing invertibility is the
price to pay for getting a total merge function. I like to compare this
to the move from real numbers to complex numbers: you get solutions for
all polynomial equations but you loose the "ordered field" laws. Indeed,
adding conflictors allows us to solve the equation p;x == q;y for any
given p and q of the right types (i.e. such that the equation is
well-typed to begin with).

My current thinking is that sequences of (named) prim patches (modulo
internal re-ordering) really form a groupoid, namely the groupoid
generated by the prim patches, subject to the equation p;q == q';p' if
commute (p;q) == q';p' (which by definition includes the ability to add
or remove inverse pairs at will). For mergeable patches we have to live
without inverses, except for operations that can directly be expressed
in terms of the effect of a patch. So this is only a category. Which
means any proofs that we want to be valid for all patch types should not
rely on the existence or any properties of inverses.

This is a largely rebased bundle without dependencies that are already in
screened. This is now ready for screened but I'll wait a bit just in case.

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

patch d2137a3be270d16f1cc3b0a072b2f3d9f1234e3a
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Sep 11 08:53:14 CEST 2019
  * add unapply method to class Apply
  
  This allows us to eliminate direct use of inversion for the named patch
  types (Named, PatchInfoAnd) in all of Darcs.Repository and Darcs.UI. It
  also allows us to clean up the Apply instance for Invertible, which no
  longer has to call applyPrimFL on the effect.

patch e4f4b05053495a8f23aa1824fc4a022978237641
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Aug 30 15:47:44 CEST 2019
  * add Darcs.Patch.Invertible
  
  This is a wrapper type to make an arbitrary patch type formally invertible.
  We define only instances that will be needed to statisfy Invert constraints
  that are currently required in the Repository and UI subsystem. Some of the
  class methods defined for Invertible assume the patch is actually positive.

patch c0dc0864eeed5190ea368181eedd67b7c4a4a69d
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Sep 11 08:46:53 CEST 2019
  * annotate: push inversion down to the prim level
  
  We must take care that we call the annotate method only on inverted prims
  and that we traverse the history (of prims) in reverse order. To avoid
  mistakes when defining instances (outside of Darcs.Patch.Annotate), we now
  use a default method with a default signature.

patch 43f927301245e9307584ba10ac1a310b5cb47db1
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Aug 30 23:56:32 CEST 2019
  * use Invertible when calling lookTouch in log command
  
  We call lookTouch with the patches inverted. To get rid of the Invert
  constraint, we wrap the patch as an Invertible patch.

patch aba21632d05e5199655db83f8031698e931fa606
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Sep 11 10:56:08 CEST 2019
  * use effect to avoid inversion of non-prims in externalResolution

patch 9feb8680801d4b320b611ce6bffdee681214ee08
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Aug 30 15:48:25 CEST 2019
  * use Invertible to generalize runSelection
  
  The new runSelection no longer requires an Invert instance for the patches.
  This is done by wrapping them internally with Invertible. We keep the old
  function under the new name runInvertibleSelection so we can use it for
  selecting prims, since these are naturally invertible and we cannot and will
  not use Splitters with wrapped Invertible patches.

patch 9a051bd5032f569690c0caf2ea5a966f98eaf76a
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Sep 10 10:01:26 CEST 2019
  * remove reverse constructors from RebaseSelect and RebaseChange

patch b86af43aefe0a77b0331174fa10abe816138b350
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Sep 17 13:11:50 CEST 2019
  * possible fix for D.UI.SelectChanges.selected
  
  This makes it actually do what the docs claim it does. I am not sure this is
  the correct behavior, though. It also renames it to getSelected to make its
  easier to see where it is used, since the word 'selected' appears in lots of
  places in this module, but getSelected is used only in printSelected.

patch b4ce731d99fafc9ab01e0aa55ae3c91680be1625
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Sep 17 13:16:53 CEST 2019
  * Invertible: allow showPatch etc of Rev patches
  
  Instead of calling error for Rev patches, requiring that the calling code
  first re-inverts the patch, we now do that ourselves. This means a Rev patch
  is shown in exactly the same way as a Fwd patch. This removes the need for
  reInvert in Darcs.UI.SelectPatches.

patch bc2acf85e2279eaa60b35358fad5b3f9be8bbf83
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Sep 10 11:52:24 CEST 2019
  * remove lots of redundant constraints

patch 538437fcc2faf5fa8edb9e3495c19af5594e812e
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Jul 30 10:36:58 CEST 2019
  * add class CleanMerge & make it super class of Merge
  
  This does not yet replace CommuteNoConflicts. Instead, instances for
  CleanMerge are, for now, defined in terms of mergeNoConflicts.

patch b115999ee7b78afdf07a5b7e5ccc4a14788d7410
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Jul 30 12:41:43 CEST 2019
  * replace CommuteNoConflicts with CleanMerge for prim patch types
  
  As a logical consequence this moves the definition of mergeList from
  D.P.CommuteNoConflicts to D.P.Merge. We also explicitly call error in
  definitions of cleanMerge and merge if the patch type has an Ident instance
  and we try to merge two identical patches, since this is an undefined
  operation.

patch a611edb74d78d6b1a27705e6c99e29b7435fe882
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Sep 10 23:01:17 CEST 2019
  * use cleanMerge to implement partitionConflictingFL
  
  In order to simplify this change, it no longer takes a commuter as argument
  but only works with plain FLs. This necessitates an upstream change to
  filterOutConflicts, which while (we're at it) now gets the repositoiry
  argument first, like all similar functions.

patch 84651fa9a5653e3740e83c4fd51f7f86f463f284
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Sep 10 23:18:25 CEST 2019
  * remove CommuteNoConflicts from RepoPatch

patch be67dba471de7daa823ff4f3ef9a58c068995625
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Sep 11 09:33:46 CEST 2019
  * eliminate Invert instances for Named, WrappedNamed, and PatchInfoAnd

patch 0b1b0dfc0b69e8727f7df6daf6e849e5311d60c4
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Sep 11 14:58:53 CEST 2019
  * remove patch type parameter to RebaseName
  
  Also remove its Apply and PrimPatchBase instances.

patch 21f01ae6390faa147fed76367bc36dc9506283a3
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Sep 11 15:58:38 CEST 2019
  * RebaseFixup take a prim as type argument, not a RepoPatch

patch cd0dcb97ef647a5a944aed2edb24061134f6a7b2
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Sep 12 11:08:46 CEST 2019
  * unify RebaseChange and RebaseSelect

patch 4f1e8782a1d74becc555673bd67189384a73e052
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Sep 12 16:21:36 CEST 2019
  * get rid of the last Invert constraints for RepoPatch in rebase
  
  This is done by changing the forceCommute implementation to do inversion on
  the underlying prims.

patch bcfe6a603f2c82e749a1f189079233b2f4f81d0a
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Sep 12 16:39:23 CEST 2019
  * remove Invert from RepoPatch constraint synonym
  
  This also disables tests for inverses of RepoPatches and fixes the
  permutivity test to no longer require it.

patch 8298fcebd0b9d3f868d7a83a9cdd3003d5460956
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Sep 12 12:25:09 CEST 2019
  * in D.P.Rebase.Viewing replace complicated constraints with RepoPatch

patch 4ff33b06e4dd5ad34c216233b230de58baca4129
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Wed Sep 18 18:57:35 CEST 2019
  * add a test for applying a patch with an unclean tag

patch d195fbc2e3764ecdc448ddc39be003b14afb68a2
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Sep 19 16:36:57 CEST 2019
  * remove Apply and PrimPatchBase instances in D.P.Rebase.Suspended
  
  The Apply instance was used only for legacy support in D.P.Named.Wrapped and
  we can as well declare the apply and unapply methods there. A comment
  explains the definitions (which just return ()) in terms of the witnesses
  for the RebaseP constructor.

patch 72f26bfa790b8cf483a8acc06ebf5c16b65ae5ba
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Sep 19 16:42:32 CEST 2019
  * remove Check and Repair instances in D.P.Rebase.Suspended and D.P.Rebase.Item
  
  These were only needed while we mixed the rebase patch with normal patches.

patch 452551b7653ae69d5109b2e420cf22674f48bb27
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Jul 30 23:56:25 CEST 2019
  * v3: simplify merge of two Conflictors
  
  The simplification consists in using cleanMerge instead of re-implementing
  it in terms of commuteRLFL.

patch ea54165379a9b6a7287895bc08d3d29493fea941
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Aug  1 17:41:24 CEST 2019
  * define instance CleanMerge RepoPatchV3 directly
  
  This means we can use it the to define the instance CommuteNoConflicts for
  the Rotcilfnoc :> Conflictor case, eliminating the only remaining place
  where we do non-trivial computation with the internals of a Rotcilfnoc.

patch a1b7b2987345f85ec436ae79d60c9763b686135e
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Sep 15 22:19:33 CEST 2019
  * remove Rotcilfnoc constructor and Invert instance for RepoPatchV3

patch b995bdd98e567ba7ad8845a5f02564ef0378d86b
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Sep 15 22:32:32 CEST 2019
  * RepoPatchV3: replace some uses of x, y with p, q
  
  We want to reserve these variable names for the conflicts in a conflictor.

Attachments

• add-unapply-method-to-class-apply.dpatch
• patch-preview.txt
• unnamed

>   This allows us to eliminate direct use of inversion for the named patch
>   types (Named, PatchInfoAnd) in all of Darcs.Repository and Darcs.UI. It
>   also allows us to clean up the Apply instance for Invertible, which no
>   longer has to call applyPrimFL on the effect.

This description doesn't quite make sense now you've reordered the patches.

I've read through the other descriptions but not the contents and I'm
fine with you screening this bundle (preferably first fixing the above
description).

>>   This allows us to eliminate direct use of inversion for the named patch
>>   types (Named, PatchInfoAnd) in all of Darcs.Repository and Darcs.UI. It
>>   also allows us to clean up the Apply instance for Invertible, which no
>>   longer has to call applyPrimFL on the effect.
> 
> This description doesn't quite make sense now you've reordered the patches.

Ha, thanks for catching that one.

> I've read through the other descriptions but not the contents and I'm
> fine with you screening this bundle (preferably first fixing the above
> description).

Will fix above then screen.

Final bundle with fixed description of first patch and also without 

  patch 4ff33b06e4dd5ad34c216233b230de58baca4129
  Author: Ganesh Sittampalam <ganesh@earth.li>
  Date:   Wed Sep 18 18:57:35 CEST 2019
    * add a test for applying a patch with an unclean tag

which accidentally slipped in last time. Will screen now.

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

patch 62126d1356e4d3a7361f20d520d4f054f05b33e2
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Sep 19 18:40:09 CEST 2019
  * add unapply method to class Apply
  
  The idea here is to allow to "inverse apply" a patch without that patch
  necessarily having an Invert instance.

patch c41fa34721ecc33b40a55bd66885ca3e1f51107c
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Sep 19 18:40:26 CEST 2019
  * add Darcs.Patch.Invertible
  
  This is a wrapper type to make an arbitrary patch type formally invertible.
  We define only instances that will be needed to statisfy Invert constraints
  that are currently required in the Repository and UI subsystem. Some of the
  class methods defined for Invertible assume the patch is actually positive.

patch 7c414e0bf884f9cc2798718df8c60cb0e3879daa
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Sep 19 18:40:36 CEST 2019
  * annotate: push inversion down to the prim level
  
  We must take care that we call the annotate method only on inverted prims
  and that we traverse the history (of prims) in reverse order. To avoid
  mistakes when defining instances (outside of Darcs.Patch.Annotate), we now
  use a default method with a default signature.

patch eb25c38924ea32d282e1d19d3b3cf3be01defd9a
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Sep 19 18:40:36 CEST 2019
  * use Invertible when calling lookTouch in log command
  
  We call lookTouch with the patches inverted. To get rid of the Invert
  constraint, we wrap the patch as an Invertible patch.

patch 1256cd0703921537ad6784e23f44a9c2733cb385
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Sep 19 18:40:36 CEST 2019
  * use effect to avoid inversion of non-prims in externalResolution

patch 935dff84c240e08dac099fa3af5ecdb8b418c0b3
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Sep 19 18:40:36 CEST 2019
  * use Invertible to generalize runSelection
  
  The new runSelection no longer requires an Invert instance for the patches.
  This is done by wrapping them internally with Invertible. We keep the old
  function under the new name runInvertibleSelection so we can use it for
  selecting prims, since these are naturally invertible and we cannot and will
  not use Splitters with wrapped Invertible patches.

patch e6d8e6474da18a8195f3e1293fd96f7c03127de3
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Sep 19 18:40:36 CEST 2019
  * possible fix for D.UI.SelectChanges.selected
  
  This makes it actually do what the docs claim it does. I am not sure this is
  the correct behavior, though. It also renames it to getSelected to make its
  easier to see where it is used, since the word 'selected' appears in lots of
  places in this module, but getSelected is used only in printSelected.

patch 4b5bba9bd00b7f6887ca4f9e981a837d6e16c101
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Sep 19 18:40:36 CEST 2019
  * Invertible: allow showPatch etc of Rev patches
  
  Instead of calling error for Rev patches, requiring that the calling code
  first re-inverts the patch, we now do that ourselves. This means a Rev patch
  is shown in exactly the same way as a Fwd patch. This removes the need for
  reInvert in Darcs.UI.SelectPatches.

patch 3e8336a654ecb9a98891c1dabfbbcd1a63b05ef8
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Sep 19 18:40:36 CEST 2019
  * remove reverse constructors from RebaseSelect and RebaseChange

patch fbbd4f8c0be03de894fe25079ac7a4576d402494
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Sep 19 18:40:36 CEST 2019
  * remove lots of redundant constraints

patch c08c4ef112e1211971076093915b466b9e978807
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Sep 19 18:40:37 CEST 2019
  * add class CleanMerge & make it super class of Merge
  
  This does not yet replace CommuteNoConflicts. Instead, instances for
  CleanMerge are, for now, defined in terms of mergeNoConflicts.

patch a13f20d3a0447d3ac7d6748633e4d6aabdf2b8cd
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Sep 19 18:40:37 CEST 2019
  * use cleanMerge to implement partitionConflictingFL
  
  In order to simplify this change, it no longer takes a commuter as argument
  but only works with plain FLs. This necessitates an upstream change to
  filterOutConflicts, which while (we're at it) now gets the repositoiry
  argument first, like all similar functions.

patch cdf70e5721a906547cc0aefc6ba4ed50f6222a51
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Sep 19 18:40:37 CEST 2019
  * replace CommuteNoConflicts with CleanMerge for prim patch types
  
  As a logical consequence this moves the definition of mergeList from
  D.P.CommuteNoConflicts to D.P.Merge. We also explicitly call error in
  definitions of cleanMerge and merge if the patch type has an Ident instance
  and we try to merge two identical patches, since this is an undefined
  operation.

patch d43b8e1b8622b06e9c650ff19750905e06483172
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Sep 19 18:40:37 CEST 2019
  * define instance CleanMerge RepoPatchV3 directly
  
  This means we can use it the to define the instance CommuteNoConflicts for
  the Rotcilfnoc :> Conflictor case, eliminating the only remaining place
  where we do non-trivial computation with the internals of a Rotcilfnoc.

patch ddf1bbf880cd7fd28d4d3c28fa99ab1065635995
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Sep 19 18:40:37 CEST 2019
  * remove Rotcilfnoc constructor and Invert instance for RepoPatchV3

patch ca0312fc5f0ef17595adf0dc6b0f60d4fa4045af
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Sep 19 18:40:37 CEST 2019
  * RepoPatchV3: replace some uses of x, y with p, q
  
  We want to reserve these variable names for the conflicts in a conflictor.

patch 439f5ad1eeb273d0326c34df29dd03f6dadf2776
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Sep 19 18:40:37 CEST 2019
  * v3: simplify merge of two Conflictors
  
  The simplification consists in using cleanMerge instead of re-implementing
  it in terms of commuteRLFL.

patch 47a68d1dc01a55d3afc2104d15947b4034c6d406
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Sep 19 18:40:37 CEST 2019
  * eliminate Invert instances for Named, WrappedNamed, and PatchInfoAnd

patch 8ba79fa025b1c1ad166a412c09e4659c4b2a4605
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Sep 19 18:40:38 CEST 2019
  * remove patch type parameter to RebaseName
  
  Also remove its Apply and PrimPatchBase instances.

patch 1e2bc554c273cb56b8b076cf1c6ed5666cba91e2
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Sep 19 18:40:38 CEST 2019
  * remove CommuteNoConflicts from RepoPatch

patch 9022bf87dd166a1366d23b51128eb9e53c974ff4
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Sep 19 18:40:38 CEST 2019
  * RebaseFixup take a prim as type argument, not a RepoPatch

patch 4bed37051856f149b457c936ae7d59f040db5a65
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Sep 19 18:40:38 CEST 2019
  * unify RebaseChange and RebaseSelect

patch b5ba17a2ddcca702b722a41910ae307e3ef80daa
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Sep 19 18:40:38 CEST 2019
  * get rid of the last Invert constraints for RepoPatch in rebase
  
  This is done by changing the forceCommute implementation to do inversion on
  the underlying prims.

patch 2b214d6c1e79ecfd94b6fafcd86c082de606556b
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Sep 19 18:40:38 CEST 2019
  * remove Invert from RepoPatch constraint synonym
  
  This also disables tests for inverses of RepoPatches and fixes the
  permutivity test to no longer require it.

patch f10f45f16ee7052909bada0b3149cc4b049fa1ae
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Sep 19 18:40:38 CEST 2019
  * in D.P.Rebase.Viewing replace complicated constraints with RepoPatch

patch 11e42bd4724f9d89f6f2dfd02d0984d5699c6634
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Sep 19 18:40:38 CEST 2019
  * remove Check and Repair instances in D.P.Rebase.Suspended and D.P.Rebase.Item
  
  These were only needed while we mixed the rebase patch with normal patches.

patch 6c39e86c985f1be37f5f2fffc4dd46fd602bb755
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Sep 19 18:40:38 CEST 2019
  * remove Apply and PrimPatchBase instances in D.P.Rebase.Suspended
  
  The Apply instance was used only for legacy support in D.P.Named.Wrapped and
  we can as well declare the apply and unapply methods there. A comment
  explains the definitions (which just return ()) in terms of the witnesses
  for the RebaseP constructor.

Attachments

• add-unapply-method-to-class-apply.dpatch
• patch-preview.txt
• unnamed

No longer WIP.

>   * add unapply method to class Apply

Fine.

>   * add Darcs.Patch.Invertible

Fine

>   * annotate: push inversion down to the prim level
>   
>   We must take care that we call the annotate method only on inverted prims
>   and that we traverse the history (of prims) in reverse order. To avoid
>   mistakes when defining instances (outside of Darcs.Patch.Annotate), we now
>   use a default method with a default signature.

This feels awkward. If the fundamental point is that annotating a repo
patch should always be delegated to annotating the prim, and in one
particular way, wouldn't it be safer to restrict the instances of
Annotate to prim patches only, and just use a helper function to lift it
to RepoPatches?

This doesn't work if we have a lot of code that needs to use Annotate
both on prim patches and repo patches, but we don't.

I wrote a quick proof-of-concept for this, see patch1938.

>   * use Invertible when calling lookTouch in log command

Fine

>   * use effect to avoid inversion of non-prims in externalResolution

> + -> WantGuiPause        -- ^ tell whether we want GUI pause

I guess docs are generally good, but this one seems a bit redundant :-)

> + -> FL p wY wA          -- ^ them merged (standard_resolution)

This doc is a bit cryptic, though it's better than not having one at all.

> -     nas = effectOnPaths (invert pmerged) nms
> +     nas = effectOnPaths (invert (effect pmerged)) nms

This feels a bit awkward though it doesn't really matter in practice.
Couldn't you have used mkInvertible?

>   * use Invertible to generalize runSelection

Fine

>>   * annotate: push inversion down to the prim level
>>   
>>   We must take care that we call the annotate method only on inverted prims
>>   and that we traverse the history (of prims) in reverse order. To avoid
>>   mistakes when defining instances (outside of Darcs.Patch.Annotate), we now
>>   use a default method with a default signature.
> 
> This feels awkward.

You name it. I felt exactly the same and expected this to come in
review. My thought so far was that the Prim.V1 instance should be
re-written to no longer expect the patch to be inverted. I was too lazy
to do that.

> If the fundamental point is that annotating a repo
> patch should always be delegated to annotating the prim, and in one
> particular way, wouldn't it be safer to restrict the instances of
> Annotate to prim patches only, and just use a helper function to lift it
> to RepoPatches?

Sounds like a good idea.

> This doesn't work if we have a lot of code that needs to use Annotate
> both on prim patches and repo patches, but we don't.
> 
> I wrote a quick proof-of-concept for this, see patch1938.

I like it. Shall I continue what you started there or will you?

>>   * use effect to avoid inversion of non-prims in externalResolution
> 
>> + -> WantGuiPause        -- ^ tell whether we want GUI pause
> 
> I guess docs are generally good, but this one seems a bit redundant :-)

True, but note that I didn't write that one. I just added haddocks for
the other parameters and those really could use them.

>> + -> FL p wY wA          -- ^ them merged (standard_resolution)
> 
> This doc is a bit cryptic, though it's better than not having one at all.

I would have invested more effort here if I hadn't had the feeling that
sooner or later we may want to re-write that code anyway. I just went
ahead and did that, will send a patch.

>> -     nas = effectOnPaths (invert pmerged) nms
>> +     nas = effectOnPaths (invert (effect pmerged)) nms
> 
> This feels a bit awkward though it doesn't really matter in practice.
> Couldn't you have used mkInvertible?

My feeling was that going via Invertible is a bit heavy here. But I
don't mind changing it.

> True, but note that I didn't write that one. I just added haddocks for
> the other parameters and those really could use them.

Oops, sorry. I saw that some hadn't had haddocks previously and just
assumed that about this one too.

> My feeling was that going via Invertible is a bit heavy here. But I
> don't mind changing it.

I don't think it matters too much, so change it only if you feel like it.

>   * possible fix for D.UI.SelectChanges.selected
>   
>   This makes it actually do what the docs claim it does. I am not sure this is
>   the correct behavior, though. It also renames it to getSelected to make its
>   easier to see where it is used, since the word 'selected' appears in lots of
>   places in this module, but getSelected is used only in printSelected.

Interesting. I agree that your new code is in line with the comment and
that it's not obvious that it is actually the right thing to do. If
we're showing the user what they've selected, showing it to them in the
commuted context (i.e. what will actually happen if they accept the
selection) would seem more natural to me.


>   * Invertible: allow showPatch etc of Rev patches
>   
>   Instead of calling error for Rev patches, requiring that the calling code
>   first re-inverts the patch, we now do that ourselves. This means a Rev patch
>   is shown in exactly the same way as a Fwd patch. This removes the need for
>   reInvert in Darcs.UI.SelectPatches.

OK. As discussed elsewhere it would make sense to disallow this when its
ForStorage.

I also feel that having the Rev case is an indication that Invertible is
a bit specialised for where we are actually using it, rather than having
a really clearly defined meaning of its own. Showing an "inverted" patch
uninverted would seem a bit strange in general, though in the context of
selection it's clearly the right thing to do.

>   * remove reverse constructors from RebaseSelect and RebaseChange

Woohoo!

>   * remove lots of redundant constraints

Nice.

>   * add class CleanMerge & make it super class of Merge
>   
>   This does not yet replace CommuteNoConflicts. Instead, instances for
>   CleanMerge are, for now, defined in terms of mergeNoConflicts.

Fine.


> patch a13f20d3a0447d3ac7d6748633e4d6aabdf2b8cd
> Author: Ben Franksen <ben.franksen@online.de>
> Date:   Thu Sep 19 18:40:37 CEST 2019
>   * use cleanMerge to implement partitionConflictingFL
>   
>   In order to simplify this change, it no longer takes a commuter as argument
>   but only works with plain FLs. This necessitates an upstream change to
>   filterOutConflicts, which while (we're at it) now gets the repositoiry
>   argument first, like all similar functions.

OK. I'd like to make the case for putting back the argument in the
longer-term though. Firstly, the more general type made it easier to see
which argument would be filtered, because we could see that p1 would be
returned and not p2. Secondly, when building sophisticated commuting
code we often find ourselves needing to build up complicated operations
and having a library of composable pieces already available is quite
helpful. Set against that I realise this was only being used with one
specific type so this change does reduce repetition.

>   * replace CommuteNoConflicts with CleanMerge for prim patch types

Fine

>   * define instance CleanMerge RepoPatchV3 directly

OK, though I still don't feel confident to judge whether the
implementation is correct and complete (in the sense of having all thr
right side conditions etc).

>   * remove Rotcilfnoc constructor and Invert instance for RepoPatchV3

Great :-)

>   * RepoPatchV3: replace some uses of x, y with p, q
>   
>   We want to reserve these variable names for the conflicts in a conflictor.

Fine. Maybe this convention could be documented somewhere, e.g. in the
doc comments of RepoPatchV3.

>   * v3: simplify merge of two Conflictors
>   
>   The simplification consists in using cleanMerge instead of re-implementing
>   it in terms of commuteRLFL.

>  error $ renderString $ redText "remaining effects don't commute:"

Perhaps this error message in the Nothing case of merge could be updated.

>   * eliminate Invert instances for Named, WrappedNamed, and PatchInfoAnd

Fine (except that as we subsequently discovered the error cases in
commuteNameNamed etc are wrong, but so was the old code - I will follow
up on that).

>   * remove CommuteNoConflicts from RepoPatch

Fine.

As this is the last patch I see that does things with CommuteNoConflicts
(apart from one in another bundle that removes instances for
RL/FL/Named) I also have some general questions about the latest state.

Is there anything more we can do to simplify/remove more bits of
CommuteNoConflicts or are we stuck with it?

I guess that we could in principle refactor V1/V2 on the same lines as
V3, but that's a bit more scary as those are "live" patch formats.

Are there any laws about how commuteNoConflicts and cleanMerge relate? I
like the way we don't have Rotcilfnoc any more but now I'm a bit scared
of the extra complexity elsewhere, in that we also have to understand
how cleanMerge fits in.

>>   * possible fix for D.UI.SelectChanges.selected
>>   
>>   This makes it actually do what the docs claim it does. I am not sure this is
>>   the correct behavior, though. It also renames it to getSelected to make its
>>   easier to see where it is used, since the word 'selected' appears in lots of
>>   places in this module, but getSelected is used only in printSelected.
> 
> Interesting. I agree that your new code is in line with the comment and
> that it's not obvious that it is actually the right thing to do. If
> we're showing the user what they've selected, showing it to them in the
> commuted context (i.e. what will actually happen if they accept the
> selection) would seem more natural to me.

I agree. A good example is (A=hunk ./a; B=move ./a ./b), where you
select A (i.e. the hunk) for e.g. amending, or even just to look at the
change: we want to see the change for ./b, not ./a.

Another good example is hunks in the same file: (A=hunk 3 [x] [y];
B=hunk 1 [] [z]), where we view/select A: it should be displayed at line
4, not the (uncommuted, original) line 3.

I will rollback and instead fix the docs.

>   * remove patch type parameter to RebaseName

I remembered one reason why the type parameter was useful:
commuteNamePrim can now be used unsafely to commute a RebaseName with
any patch including a Named.

However this is also a criticism of the existing
commuterIdNamed/commuterNamedId which could be combined to give a wrong
commute for Named/Named. I have patches lying around my WIP to add
comments to each to explain this.

The "right" fix would be to have independent witnesses for names, but
it's unlikely to be worth the disruption to introduce that.

>   * RebaseFixup take a prim as type argument, not a RepoPatch

Nice to see we can make this change independently of others.

>   * unify RebaseChange and RebaseSelect

This is an excellent simplification.

The comment on getLogInfoCore is now wrong:

> -- This function is split out from getLogInfo/getLogInfoFL

and I guess it could be merged back into getLogInfo. But not too
important to do immediately.

If the introduction of LogInfo had been done in a separate patch, it'd
have been a bit easier to review, and it'd have been clear that the
actual code change to switch RebaseChange is pleasingly simple.

>   * get rid of the last Invert constraints for RepoPatch in rebase

Fine

>   * remove Invert from RepoPatch constraint synonym

>   This also disables tests for inverses of RepoPatches and fixes the
>   permutivity test to no longer require it.

I guess we should just delete those test references rather than having
them commented out? (I guess the underlying tests are still there and
used for prim patches)

>   * in D.P.Rebase.Viewing replace complicated constraints with RepoPatch

Fine

>   * remove Check and Repair instances in D.P.Rebase.Suspended and D.P.Rebase.Item
>   
>   These were only needed while we mixed the rebase patch with normal patches.

Great.

Not an issue for this bundle as you were only removing dead code, but I
guess we should think about whether the rebase state should be subject
to checking/repairing. We lost that without noticing (or I've forgotten
any discussion) when rebase was moved to the repository level.

>   * remove Apply and PrimPatchBase instances in D.P.Rebase.Suspended

Fine

I'm marking this as accepted(-pending-tests) as there weren't any
major review points. If you have any followups that need review please
send them to new patches. Any that are obvious enough not to need 
review can be sent to this one.

Am 25.09.19 um 20:35 schrieb Ganesh Sittampalam:
> 
> Ganesh Sittampalam <ganesh@earth.li> added the comment:
> 
> I'm marking this as accepted(-pending-tests) as there weren't any
> major review points. If you have any followups that need review please
> send them to new patches. Any that are obvious enough not to need 
> review can be sent to this one.

Oops. Saw this message too late and sent a follow-up. Will re-send to a
new bundle. (It was about the supposed fix to getSelected).

>>   * Invertible: allow showPatch etc of Rev patches
>>   
>>   Instead of calling error for Rev patches, requiring that the calling code
>>   first re-inverts the patch, we now do that ourselves. This means a Rev patch
>>   is shown in exactly the same way as a Fwd patch. This removes the need for
>>   reInvert in Darcs.UI.SelectPatches.
> 
> I also feel that having the Rev case is an indication that Invertible is
> a bit specialised for where we are actually using it, rather than having
> a really clearly defined meaning of its own. Showing an "inverted" patch
> uninverted would seem a bit strange in general, though in the context of
> selection it's clearly the right thing to do.

Yes, this is a bit strange, generally speaking. I think the true meaning
of a Rev patch is exactly this: it pretends to be an inverse without
actually being one. Fortunately the implementation is simple enough that
it can serve as specification, too.

>> patch a13f20d3a0447d3ac7d6748633e4d6aabdf2b8cd
>> Author: Ben Franksen <ben.franksen@online.de>
>> Date:   Thu Sep 19 18:40:37 CEST 2019
>>   * use cleanMerge to implement partitionConflictingFL
>>   
>>   In order to simplify this change, it no longer takes a commuter as argument
>>   but only works with plain FLs. This necessitates an upstream change to
>>   filterOutConflicts, which while (we're at it) now gets the repositoiry
>>   argument first, like all similar functions.
> 
> OK. I'd like to make the case for putting back the argument in the
> longer-term though. Firstly, the more general type made it easier to see
> which argument would be filtered, because we could see that p1 would be
> returned and not p2. Secondly, when building sophisticated commuting
> code we often find ourselves needing to build up complicated operations
> and having a library of composable pieces already available is quite
> helpful. Set against that I realise this was only being used with one
> specific type so this change does reduce repetition.

I am not at all opposed to bringing the generality back. When I wrote
this I was far from sure the plan would work out, or whether we even
want to go that route if it did, so I couldn't be bothered to figure out
how to adapt the function in its more general form.

>>   * define instance CleanMerge RepoPatchV3 directly
> 
> OK, though I still don't feel confident to judge whether the
> implementation is correct and complete (in the sense of having all thr
> right side conditions etc).

I agree that we should document the reasoning behind the side-conditions
somewhere in the code. See below for some more details.

>>   * RepoPatchV3: replace some uses of x, y with p, q
>>   
>>   We want to reserve these variable names for the conflicts in a conflictor.
> 
> Fine. Maybe this convention could be documented somewhere, e.g. in the
> doc comments of RepoPatchV3.

Good idea. I have just recorded a patch that does this and also cleans
up and documents the naming convention for contexted prims (always
prepends a 'c') and the one for the case of effects sharing a common part.

>>   * v3: simplify merge of two Conflictors
>>   
>>   The simplification consists in using cleanMerge instead of re-implementing
>>   it in terms of commuteRLFL.
> 
>>  error $ renderString $ redText "remaining effects don't commute:"

Will fix.

>>   * remove CommuteNoConflicts from RepoPatch
> 
> Fine.
> 
> As this is the last patch I see that does things with CommuteNoConflicts
> (apart from one in another bundle that removes instances for
> RL/FL/Named) I also have some general questions about the latest state.
> 
> Is there anything more we can do to simplify/remove more bits of
> CommuteNoConflicts or are we stuck with it?

I would remove the class if it weren't for combineConflicts which uses
commuteNoConflicts. While I am pretty sure it could be replaced with a
plain commute, I am hesitating to make that change.

As for the cases themselves, they now correspond 1:1 with the
"unrelated" commute cases documented in the paper. I think there is no
way to eliminate them. The new cases for cleanMerge correspond to the
former Rotcilfnoc cases for commuteNoConflicts that we previously had.

> I guess that we could in principle refactor V1/V2 on the same lines as
> V3, but that's a bit more scary as those are "live" patch formats.

I don't think we should spend any further effort on the implementation
of V1/V2 beyond what is strictly necessary to get things to build.

I am not sure we will ever fully understand all the details of how V2
commutation is supposed to work. Any change there may have side effects
that break it in even more cases than it already does. For V1 I do have
a good idea how it is supposed to work, but fixing it to actually do
that will slow it down so much that it wouldn't be usable in practice
anymore.

> Are there any laws about how commuteNoConflicts and cleanMerge relate? I
> like the way we don't have Rotcilfnoc any more but now I'm a bit scared
> of the extra complexity elsewhere, in that we also have to understand
> how cleanMerge fits in.

On the practical side, I think that commuteNoConflicts should not be
used, except for the legacy use in combineConflicts. This is why I have
removed it from the RepoPatch constraint. For V3 we can simply regard it
as part of the implementation of commute.

That doesn't answer your question about laws relating commuteNoConflicts
with cleanMerge. I think there is a close relation, similar to the
relation between the conflicted merge and commuteConflicting.

Here are my thoughts.

Merge and commute are two sides of the same coin. For prim patches it is
clear that to ask whether two patches commute is the same as asking
whether they could have been made (recorded) independently. So p;q
commute if and only if there are some p' and q', such that p\/q' cleanly
merges to q/\p'.

For mergeable patches (i.e. with conflictors added) we must first
clarify what "made independently" means. We cannot "make" a conflicted
change. But every conflicted patch was once "made" unconflicted. I think
the meaning of commuteNoConflicts of p;q is to answer whether there are
unconflicted p';q' that commute as prims (where p and p' as well as q
and q' have the same identity). Which as we know means there are p'' and
q'' such that p'\/q'' cleanly merges to q'/\p''.

The main point now is that "being in conflict" must be an /invariant/
relation between patches, that is, it must not depend on context. Which
gives us the law

  commuteNoConflicts p;q = Just q''';p'''
<=>
  cleanMerge p\/q''' == Just q/\p'''

This doesn't give us a way to implement one in terms of the other but it
does allow us to check that they are consistent.

Does that make sense?

>>   * remove patch type parameter to RebaseName
> 
> I remembered one reason why the type parameter was useful:
> commuteNamePrim can now be used unsafely to commute a RebaseName with
> any patch including a Named.

A possible solution is to add a (redundant, commented) PrimPatch
constraint. That would be in line with the names of these functions.

> However this is also a criticism of the existing
> commuterIdNamed/commuterNamedId which could be combined to give a wrong
> commute for Named/Named. I have patches lying around my WIP to add
> comments to each to explain this.

These functions are definitely "unsafe": we cannot just operate on the
FL inside a Named patch without regard for explicit dependencies.

My view is that adding such functions to the Named API is not a good
idea because I expect such functions to respect internal invariants. I'd
prefer to have them implemented outside of Darcs.Patch.Named, explicitly
importing the data constructors. At least this makes it clear that we
operate on internals and are responsible for not destroying invariants.

When/if your rebase refactor gets finished, we may want to add
(redundant) RepoPatch constraints there, so that we cannot abuse them in
the way you indicated. This would also require us to implement the
check/repair stuff for RebasePatch, see below, so better postpone that
until we have got the essentials right.

> The comment on getLogInfoCore is now wrong:
> 
>> -- This function is split out from getLogInfo/getLogInfoFL
> 
> and I guess it could be merged back into getLogInfo. But not too
> important to do immediately.

I have already done that in a patch that I haven't sent yet.

In the long run we will probably want to support at least some of the
matching options, such as --patches or --matches. This will require
further refactorings now, but I think this is okay.

> If the introduction of LogInfo had been done in a separate patch, it'd
> have been a bit easier to review, and it'd have been clear that the
> actual code change to switch RebaseChange is pleasingly simple.

I agree. My only excuse is that when I made these changes I was in a
frenzy to get at the point where I could see whather the whole plan had
a chance to succeed. And picking such refactorings apart after the fact
is awfully hard. I spent many hours rebasing the bundle and making sure
intermediate states can at least be built, cleaning up patches,
inserting explicit dependencies, rebasing again etcetc.

It would help a lot if rebase had an explicit command to force-commute
suspended patches. Currently the only way to do that is to obliterate
one of the patches which means the we must drop /all/ explicit
dependencies on that patch, even for any later patches that aren't
involved in the force-commute. This is pretty bad because choosing
exactly which explicit dependencies are required is not easy. I have
hopes that when your refactor of the rebase internals is done we can add
such a (sub)command more easily.

>>   * remove Invert from RepoPatch constraint synonym
> 
>>   This also disables tests for inverses of RepoPatches and fixes the
>>   permutivity test to no longer require it.
> 
> I guess we should just delete those test references rather than having
> them commented out? (I guess the underlying tests are still there and
> used for prim patches)

I did not delete any properties, just commented out the tests in
Darcs.Test.Patch. I think you are right and we should eventually remove
the code I commented out. But when we do that we should be very careful
to check that we actually run all those tests in the appropriate
settings i.e. for all prim types.

>>   * remove Check and Repair instances in D.P.Rebase.Suspended and D.P.Rebase.Item
>>   
>>   These were only needed while we mixed the rebase patch with normal patches.
> 
> Not an issue for this bundle as you were only removing dead code, but I
> guess we should think about whether the rebase state should be subject
> to checking/repairing. We lost that without noticing (or I've forgotten
> any discussion) when rebase was moved to the repository level.

No, I think this wasn't discussed at all, in fact it completely escaped
my notice that this has ever been a feature and not just a side-effect
of having to implement them because of superclass constraints. We should
remind ourselves to implement this when your rebase-based-on-prim
refactor has settled down and lands in screened.

(BTW you seem to be re-setting the title to "WIP: ...", I guess this is
an unintended side-effect of how you use the tracker? I always use my
email client for messages, nowadays, since at some point the
web-interface stopped formatting the text with line breaks...)