darcs

Patch 1701 refactorings in the Darcs.Patch subsystem

Title refactorings in the Darcs.Patch subsystem
Superseder Nosy List bf
Related Issues
Status review-in-progress Assigned To
Milestone

Created on 2018-07-17.14:09:37 by bf, last changed 2018-07-29.20:58:50 by ganesh.

Files
File name Status Uploaded Type Edit Remove
apply-hunks-for-the-same-file-in-one-go-for-v2_-too.dpatch bf, 2018-07-17.14:09:36 text/x-darcs-patch
See mailing list archives for discussion on individual patches.
Messages
msg20191 (view) Author: bf Date: 2018-07-17.14:09:36
A bunch of refactorings in the Darcs.Patch subsystem.

The most important (non-trivial) refactor is
  * replace naturalMerge with mergeNoConflicts

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

patch 2f159567f4cac4c1b8395de7b6f2d1f71d96e442
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Feb 16 12:19:52 CET 2018
  * apply hunks for the same file in one go for V2, too
  
  It seems this optimization was never done for RepoPatchV2.

patch 72545e3b3d8c082e195039af0eed119a70bae8a2
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Apr 26 09:00:37 CEST 2018
  * rename 'conflictedEffect' to 'isConflicted'
  
  This function (class method) is used to classify patches and has
nothing to
  do with the effect of a patch.

patch 0ce071080b90f467e23a7cbb4f514cdc3e4bcda7
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Apr 26 09:12:09 CEST 2018
  * add haddocks for D.P.Conflict.IsConflictedPrim

patch 12839791e8952a09579f1432177ce11965d96542
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Apr 26 09:12:55 CEST 2018
  * remove out-commented code line from mangleUnravelledHunks

patch 0210b59079b37ea03a6e837e85a85f9cdb5849d3
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue May 29 14:55:40 CEST 2018
  * improved haddocks for CommuteNoConflicts

patch 5538a1fc92fa3dd0210dd9261fd9b6ed2ad11847
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue May 29 15:01:51 CEST 2018
  * refactor the instance Commute RepoPatchV2
  
  To avoid repeated calls to commuteNoConflicts in cases where we know
it will
  fail, add commuteConflicting as a separate top-level function. Also factor
  out invertCommuter which is now used for both commuteConflicting and
  commuteNoConflicts.

patch 86ccfc74c0c04b2d8ef2acca37c17c7367432102
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed May 30 18:30:32 CEST 2018
  * factor swapMerge from D.P.V2.RepoPatch to D.P.Merge

patch 536b211062f18b42715cd525e2be075897b8df91
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed May 30 18:31:34 CEST 2018
  * simplify V1 merge by calculating both branches in one go

patch 71853b7f5fac8841c62214ccb15ebc81650dace4
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Jun  4 17:48:12 CEST 2018
  * in D.P.V1.Commute, remove commuteNoMerger and replace with
commuteNoConflicts

patch c31ae97d50d3e7b5659ba005d0509c4ea78d1e1b
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Jun  7 20:14:32 CEST 2018
  * replace naturalMerge with mergeNoConflicts
  
  This change is a consequence of my improved understanding of what
  commuteNoConflicts is about and how non-conflicting merge should work, see
  the haddocks for details. The new function is also more efficient because
  (a) commuteNoConflicts has to consider fewer cases and (b) we can now drop
  the extra commute check on the result of the merge.

patch 95404fa9c8100ee49d976f2cd203b6a49534d69a
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Jun  7 21:51:30 CEST 2018
  * add laws to classes Commute and CommuteNoConflicts

patch ad2600e278e89fb4f0fff9413bc94dbff5379e62
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed May 16 10:02:35 CEST 2018
  * add _forceCommute to D.P.Merge
  
  The only purpose of this function is to document how the merge operation,
  together with the square commute law, allows to commute any pair of
adjacent
  patches.
Attachments
msg20192 (view) Author: bf Date: 2018-07-17.14:28:03
Screening this bundle (all patches basically make sense AFAICS and
successfully test on my machines).
msg20229 (view) Author: ganesh Date: 2018-07-29.09:04:53
A partial review - I've pushed all the patches below to
reviewed. I'm still thinking about the remaining three.

>  * apply hunks for the same file in one go for V2, too

Fine

>  * rename 'conflictedEffect' to 'isConflicted'

OK - to be honest I don't find either name particularly clear.
'isConflicted' feels a bit strange as it doesn't return a Bool.
'conflictedEffect' seems to reflect the fact that ultimately
the implementation on V1 patches calls 'effect' but I agree it
isn't very helpful. I don't have a better name, anyway.

>  * add haddocks for D.P.Conflict.IsConflictedPrim

Fine

>  * remove out-commented code line from mangleUnravelledHunks

Fine


>  * refactor the instance Commute RepoPatchV2

Fine

>  * factor swapMerge from D.P.V2.RepoPatch to D.P.Merge

Fine

>  * simplify V1 merge by calculating both branches in one go

Either delete the commented out code or add a comment to explain why it's there

>  * in D.P.V1.Commute, remove commuteNoMerger and replace with commuteNoConflicts

Fine


>  * add _forceCommute to D.P.Merge

Fine - is calling commute first just an optimisation, or is it necessary for correctness?
msg20230 (view) Author: ganesh Date: 2018-07-29.20:45:12
As I mentioned, I'm still thinking about the last few patches in this bundle,
that clarify commuteNoConflicts.

One of the things that's making it harder for me to understand them is the
idea that a conflict is between a *sequential* pair of patches (p:>q).
I've always thought of conflicts as between a *parallel* pair of patches
(p:\/:q).

I know the language you used is somewhat implied by the fact that
commuteNoConflicts is the one that actually has a definition. But do you think
it'd make sense to formulate as something like
"commuteNoConflicts (p:>q) fails when commute (p:>q) succeeds in the case
that (p:>q) results from a conflicted merge"?

Maybe further down the line we could also make mergeNoConflicts be the
primitive operation.

>  * improved haddocks for CommuteNoConflicts

Apart from the definition of 'conflict', this is fine. The new documentation is
a bit ambiguous about what happens with composite patches: the answer is that the
commute fails if any of the individual patch commutes fail.

>  * replace naturalMerge with mergeNoConflicts

I need to keep thinking about the comment associated with this change.

>  * add laws to classes Commute and CommuteNoConflicts

It would be nice if these laws had associated tests.

> +-- * If an instance @'Commute' p@ exists, then

Should we just add Commute as a superclass of CommuteNoConflicts?
Even though it's not necessary for the implementation I don't think it's
useful not to have it.
History
Date User Action Args
2018-07-17 14:09:37bfcreate
2018-07-17 14:28:03bfsetstatus: needs-screening -> needs-review
messages: + msg20192
2018-07-29 09:04:54ganeshsetmessages: + msg20229
2018-07-29 20:45:12ganeshsetmessages: + msg20230
2018-07-29 20:58:50ganeshsetstatus: needs-review -> review-in-progress