I am not screening this immediately, but would like to do so
soon if there are no objections.

It's a series of incremental refactors to turn the rather
monolithic definition of simplifyPush into more modular pieces
that we'll be able to reuse when refactoring the rebase types.

It was initially part of my WIP bundle for migrating rebase
to use prim types everywhere, but I've factored it out to be
applyable first even though it adds a bit more complexity
(the code is simpler with prims). I think it's worth it to break
the merging work into smaller pieces.

Some of the changes are quite small, reflecting the way I
actually did the refactoring.

mapMB_MB can obviously be replaced with fmap2 if we apply
the Functor2 patch.

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

patch 393574cfc5cbccd959c0f9a14c2e9d9eed4f0ab9
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Fri Sep 27 20:15:09 BST 2019
  * refactor simplifyPush
  
  This patch introduces a general concept of pushing 'fixups'
  past 'items', and uses it to implement simplifyPush.
  
  This provides a framework for modularizing the logic in
  subsequent patches.

patch fcc84f7dfb92f769a649bfbdd5ba6aba639f4280
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Fri Sep 27 20:19:06 BST 2019
  * drop redundant case in pushFixupItem

patch 17f854155bbacb52bf2ab974e04fcb82a2e56d00
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Fri Sep 27 20:19:28 BST 2019
  * drop error in commuteNameNamed if a deleted name is readded
  
  As in D.P.R.Item.pushFixupItem, this scenario isn't impossible
  so should just be a failed commute rather than an error.
  

patch 287c8c901e9ba38e018dfeef7c79cf2473edd563
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Fri Sep 27 20:21:22 BST 2019
  * reuse commuteNameNamed to push RebaseName through ToEdit
  
  A commute is the natural way of implementing this operation
  and following the previous refactors the logic was identical

patch f7235ff1566651fe2f04dac2a71d103b3edd9435
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Fri Sep 27 20:32:01 BST 2019
  * add Maybe2 type

patch 2283cc4f2f6a131728fbcd0a622a9bc49f7458cd
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Fri Sep 27 20:32:08 BST 2019
  * move logic for pushing name fixups to D.P.R.Name

patch 08e571529264913630083abe368ef355a5d4195b
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Fri Sep 27 20:40:03 BST 2019
  * move logic for pushing prim fixups to D.P.R.Fixup

patch 9f3996fbc438da7df101b85535aa760077fbcab3
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Fri Sep 27 20:41:40 BST 2019
  * reorder cases in pushFixupItem
  
  Just to make it easier to see how connected logic can be
  extracted to a separate function.

patch 3e3ebc087fb95171de2fc1ed7befc0b111135289
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Fri Sep 27 20:50:20 BST 2019
  * move logic for pushing past fixups into D.P.R.Fixup

patch 7fa75673d2f15d74320b510f557b6d19fdbc004e
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Fri Sep 27 20:53:55 BST 2019
  * use commuteFixupNamed in pushFixupItem

Attachments

• patch-preview.txt
• refactor-simplifypush.dpatch
• unnamed

resending to amend out trailing whitespace in the last patch

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

patch 393574cfc5cbccd959c0f9a14c2e9d9eed4f0ab9
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Fri Sep 27 20:15:09 BST 2019
  * refactor simplifyPush
  
  This patch introduces a general concept of pushing 'fixups'
  past 'items', and uses it to implement simplifyPush.
  
  This provides a framework for modularizing the logic in
  subsequent patches.

patch fcc84f7dfb92f769a649bfbdd5ba6aba639f4280
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Fri Sep 27 20:19:06 BST 2019
  * drop redundant case in pushFixupItem

patch 17f854155bbacb52bf2ab974e04fcb82a2e56d00
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Fri Sep 27 20:19:28 BST 2019
  * drop error in commuteNameNamed if a deleted name is readded
  
  As in D.P.R.Item.pushFixupItem, this scenario isn't impossible
  so should just be a failed commute rather than an error.
  

patch 287c8c901e9ba38e018dfeef7c79cf2473edd563
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Fri Sep 27 20:21:22 BST 2019
  * reuse commuteNameNamed to push RebaseName through ToEdit
  
  A commute is the natural way of implementing this operation
  and following the previous refactors the logic was identical

patch f7235ff1566651fe2f04dac2a71d103b3edd9435
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Fri Sep 27 20:32:01 BST 2019
  * add Maybe2 type

patch 2283cc4f2f6a131728fbcd0a622a9bc49f7458cd
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Fri Sep 27 20:32:08 BST 2019
  * move logic for pushing name fixups to D.P.R.Name

patch 08e571529264913630083abe368ef355a5d4195b
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Fri Sep 27 20:40:03 BST 2019
  * move logic for pushing prim fixups to D.P.R.Fixup

patch 9f3996fbc438da7df101b85535aa760077fbcab3
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Fri Sep 27 20:41:40 BST 2019
  * reorder cases in pushFixupItem
  
  Just to make it easier to see how connected logic can be
  extracted to a separate function.

patch 3e3ebc087fb95171de2fc1ed7befc0b111135289
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Fri Sep 27 20:50:20 BST 2019
  * move logic for pushing past fixups into D.P.R.Fixup

patch efe5448f9d159779df7ebf47099bcd5cd249d2b5
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Fri Sep 27 21:38:04 BST 2019
  * use commuteFixupNamed in pushFixupItem

Attachments

• patch-preview.txt
• refactor-simplifypush.dpatch
• unnamed

> I am not screening this immediately, but would like to do so
> soon if there are no objections.
> 
> It's a series of incremental refactors to turn the rather
> monolithic definition of simplifyPush into more modular pieces
> that we'll be able to reuse when refactoring the rebase types.

You are obviously approaching this in a way that is quite different from
how I would do it. This is not surprising of course and by all means do
it your way. Just to get it out of my system: I think your step-by-step
refactors is wasting effort on code that is unnecessarily complicated to
begin with, due to the mixing of different types of things (RebaseFixup
mixed fixups with name fixups, RebaseItem mixes suspended patches with
both name and normals fixups). I would rather spend effort on separating
them which I am convinced will make everything a lot simpler. This is
not easy to do in small steps, I understand that, but at some point you
will have to bite the bullet and just plunge ahead.

Now, with that out of the way, your changes look okay to me, so go ahead
and screen them. I find it difficult to check them for correctness in
detail, though. One reason is that I am not familiar enough with the
existing code. Another is lack of documentation. I would like to suggest
that if you find it worth to refactor the existing functions you might
as well add haddocks to them while you're at it. It is always easier to
understand a change when you know the precise purpose of the code. (Some
of it can be guessed from the names but not everything.)

I would personally prefer to review a final version because that doesn't
require me to understand all the steps of how you got there from the
current situation. But we cannot always have it all as we like it, can we?

(BTW, I think this is at least partly the fault of darcs because darcs
diff has no interactive mode. I am missing that a lot when reviewing
bundles that consist of many small patches. I want to traverse my
patches and say: "show me this patch in meld".)

I should definitely have written a haddock for PushFixupFn, I'll
do that asap. simplifyPush itself, which is the thing I'm ultimately
refactoring, already has a haddock. Do you want to see haddocks on
the individual functions I'm introducing? They'd basically amount to
"push x past y", for varying values of x and y.

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

patch be525464af7dd4a365a7bb84cbefdf040addea28
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Sat Sep 28 13:10:16 BST 2019
  * add haddock to PushFixupFn

Attachments

• add-haddock-to-pushfixupfn.dpatch
• patch-preview.txt
• unnamed

Sorry for letting this lie around for so long.

> patch 393574cfc5cbccd959c0f9a14c2e9d9eed4f0ab9
> Author: Ganesh Sittampalam <ganesh@earth.li>
> Date:   Fri Sep 27 20:15:09 BST 2019
>   * refactor simplifyPush
>   
>   This patch introduces a general concept of pushing 'fixups'
>   past 'items', and uses it to implement simplifyPush.
>   
>   This provides a framework for modularizing the logic in
>   subsequent patches.

Okay. It took me a bit to realize that the changes to
Darcs.Patch.Rebase.Item are actually quite trivial. Basically, we move
from returning Sealed to returning a pair i.e. we don't immediately
throw away the fixups that come out at the end.

> patch fcc84f7dfb92f769a649bfbdd5ba6aba639f4280
> Author: Ganesh Sittampalam <ganesh@earth.li>
> Date:   Fri Sep 27 20:19:06 BST 2019
>   * drop redundant case in pushFixupItem

I have stared at this for a while and don't get why this case is
redundant. Could you explain?

> patch 17f854155bbacb52bf2ab974e04fcb82a2e56d00
> Author: Ganesh Sittampalam <ganesh@earth.li>
> Date:   Fri Sep 27 20:19:28 BST 2019
>   * drop error in commuteNameNamed if a deleted name is readded
>   
>   As in D.P.R.Item.pushFixupItem, this scenario isn't impossible
>   so should just be a failed commute rather than an error.

Okay, agreed.

> patch 287c8c901e9ba38e018dfeef7c79cf2473edd563
> Author: Ganesh Sittampalam <ganesh@earth.li>
> Date:   Fri Sep 27 20:21:22 BST 2019
>   * reuse commuteNameNamed to push RebaseName through ToEdit
>   
>   A commute is the natural way of implementing this operation
>   and following the previous refactors the logic was identical

Okay.

> patch f7235ff1566651fe2f04dac2a71d103b3edd9435
> Author: Ganesh Sittampalam <ganesh@earth.li>
> Date:   Fri Sep 27 20:32:01 BST 2019
>   * add Maybe2 type

Fine.

> patch 2283cc4f2f6a131728fbcd0a622a9bc49f7458cd
> Author: Ganesh Sittampalam <ganesh@earth.li>
> Date:   Fri Sep 27 20:32:08 BST 2019
>   * move logic for pushing name fixups to D.P.R.Name

Yup that makes sense.

> patch 08e571529264913630083abe368ef355a5d4195b
> Author: Ganesh Sittampalam <ganesh@earth.li>
> Date:   Fri Sep 27 20:40:03 BST 2019
>   * move logic for pushing prim fixups to D.P.R.Fixup

Same here.

> patch 9f3996fbc438da7df101b85535aa760077fbcab3
> Author: Ganesh Sittampalam <ganesh@earth.li>
> Date:   Fri Sep 27 20:41:40 BST 2019
>   * reorder cases in pushFixupItem
>   
>   Just to make it easier to see how connected logic can be
>   extracted to a separate function.

Agreed...

> patch 3e3ebc087fb95171de2fc1ed7befc0b111135289
> Author: Ganesh Sittampalam <ganesh@earth.li>
> Date:   Fri Sep 27 20:50:20 BST 2019
>   * move logic for pushing past fixups into D.P.R.Fixup

...because again this looks good and it is now indeed easier to see that
it is correct.

> patch 7fa75673d2f15d74320b510f557b6d19fdbc004e
> Author: Ganesh Sittampalam <ganesh@earth.li>
> Date:   Fri Sep 27 20:53:55 BST 2019
>   * use commuteFixupNamed in pushFixupItem

Fine.

If you could respond to the one question above about patch
fcc84f7dfb92f769a649bfbdd5ba6aba639f4280 I'll accept this bundle.

Attachments

• pEpkey.asc

On 22/02/2020 07:58, Ben Franksen wrote:

>> patch fcc84f7dfb92f769a649bfbdd5ba6aba639f4280
>> Author: Ganesh Sittampalam <ganesh@earth.li>
>> Date:   Fri Sep 27 20:19:06 BST 2019
>>   * drop redundant case in pushFixupItem
> 
> I have stared at this for a while and don't get why this case is
> redundant. Could you explain?

'p' is bound by this pattern:

> p@(ToEdit (NamedP pn deps body))

The change was from this:

>   | new `elem` deps =
>       let newdeps = map (\dep -> if new == dep then old else dep) deps
>       in (ToEdit (NamedP pn newdeps (unsafeCoerceP body)) :>: NilFL) :> (NameFixup (Rename old new) :>: NilFL)
>   | otherwise = (unsafeCoerceP p :>: NilFL) :> (NameFixup (Rename old new) :>: NilFL)

to this:

>   | otherwise =
>       let newdeps = map (\dep -> if new == dep then old else dep) deps
>       in (ToEdit (NamedP pn newdeps (unsafeCoerceP body)) :>: NilFL) :> (NameFixup (Rename old new) :>: NilFL)

If 'new' isn't in 'deps', then 'newdeps' will be the same as 'deps', so

 p = ToEdit (NamedP pn deps body)

will be the same as

 ToEdit (NamedP pn newdeps (unsafeCoerce p body))

so we can use the logic that was previously just for when 'new' is in
'deps' even when it's not.

The 'otherwise' case was either an optimisation to avoid rebuilding the
patch, or a mistake - it would have been me and I can't remember which.

I also can't remember exactly why I decided to drop it now but I expect
it makes things simpler when moving code around either in this bundle or
in one of my still-unsubmitted rebase changes.

Got it, thanks for explaining.