darcs

Patch 1863 force PatchSet to always start at Origin

Title force PatchSet to always start at Origin
Superseder Nosy List ganesh
Related Issues
Status accepted Assigned To
Milestone

Created on 2019-08-09.12:04:09 by ganesh, last changed 2019-08-20.15:33:52 by bfrk.

Files
File name Status Uploaded Type Edit Remove
patch-preview.txt ganesh, 2019-08-09.12:04:09 text/x-darcs-patch
patch-preview.txt ganesh, 2019-08-12.01:34:17 text/x-darcs-patch
patch-preview.txt ganesh, 2019-08-12.10:17:52 text/x-darcs-patch
patch-preview.txt ganesh, 2019-08-12.22:47:39 text/x-darcs-patch
refactor-the-_darcs-log_-code-to-accept-a-list-of-patches.dpatch ganesh, 2019-08-12.01:34:17 application/x-darcs-patch
refactor-the-_darcs-log_-code-to-accept-a-list-of-patches.dpatch ganesh, 2019-08-12.10:17:52 application/x-darcs-patch
refactor-the-_darcs-log_-code-to-accept-a-list-of-patches.dpatch ganesh, 2019-08-12.22:47:39 application/x-darcs-patch
unnamed ganesh, 2019-08-09.12:04:09 text/plain
unnamed ganesh, 2019-08-12.01:34:17 text/plain
unnamed ganesh, 2019-08-12.10:17:52 text/plain
unnamed ganesh, 2019-08-12.22:47:39 text/plain
wip_-force-patchset-to-always-start-at-origin.dpatch ganesh, 2019-08-09.12:04:09 application/x-darcs-patch
See mailing list archives for discussion on individual patches.
Messages
msg21063 (view) Author: ganesh Date: 2019-08-09.12:04:09
Followup to discussion in patch1807

Not for screened yet because the unsafeCoercePEnd in
Darcs.UI.Commands.Rebase is pretty gratuitous and
can hopefully be refactored away.

I haven't run the tests, just the type-checker :-)

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

patch 641287e98759a019659e635952a2510162f7f4c3
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Fri Aug  9 12:40:50 BST 2019
  * WIP: force PatchSet to always start at Origin
Attachments
msg21069 (view) Author: bfrk Date: 2019-08-10.10:27:59
> 1 patch for repository darcs-unstable@darcs.net:screened:
> 
> patch 641287e98759a019659e635952a2510162f7f4c3
> Author: Ganesh Sittampalam <ganesh@earth.li>
> Date:   Fri Aug  9 12:40:50 BST 2019
>   * WIP: force PatchSet to always start at Origin

data PatchSet rt p wStart wY where
  PatchSet :: RL (Tagged rt p) Origin wX -> RL (PatchInfoAnd rt p) wX wY
           -> PatchSet rt p Origin wY

Hm. wStart isn't used now. What does it stand for? Nothing. It is just
there so that the kind of PatchSet is compatible with patches.

patchSet2RL :: PatchSet rt p wStart wX -> RL (PatchInfoAnd rt p) wStart wX

This and many other functions defined in Darcs.Patch.Set are now wrong.
They should return 'RL (PatchInfoAnd rt p) Origin wX'. That the type
checker doesn't flag this is an error worries me...

Here is an alternative proposal: we rename PatchSet to UnsafePatchSet
and define a type synonym

type PatchSet rt p = UnsafePatchSet rt p Origin

and then remove the first witness from all ocurrences of PatchSet. This
would lead us to find this second problematic piece of code:

src/Darcs/Patch/Bundle.hs:119:39: error:
    • Could not deduce: wZ ~ Origin

This and the problematic piece in Darcs.UI.Commands.Rebase could then be
fixed, temporarily, by using UnsafePatchSet directly until we find a way
to refactor these to avoid UnsafePatchSet.
msg21071 (view) Author: ganesh Date: 2019-08-10.11:20:43
On 10/08/2019 11:27, Ben Franksen wrote:

> data PatchSet rt p wStart wY where
>   PatchSet :: RL (Tagged rt p) Origin wX -> RL (PatchInfoAnd rt p) wX wY
>            -> PatchSet rt p Origin wY
> 
> Hm. wStart isn't used now. What does it stand for? Nothing. It is just
> there so that the kind of PatchSet is compatible with patches.

Yes. I agree this is confusing.

> patchSet2RL :: PatchSet rt p wStart wX -> RL (PatchInfoAnd rt p) wStart wX
> 
> This and many other functions defined in Darcs.Patch.Set are now wrong.
> They should return 'RL (PatchInfoAnd rt p) Origin wX'. That the type
> checker doesn't flag this is an error worries me...

You can't get a non-bottom/unsafeCoerced PatchSet without wStart~Origin,
so the signature is still correct. We could also systematically follow
around all the uses of PatchSet and the things that originate a PatchSet
and make the signatures tighter.

> Here is an alternative proposal: we rename PatchSet to UnsafePatchSet
> and define a type synonym
> 
> type PatchSet rt p = UnsafePatchSet rt p Origin
> 
> and then remove the first witness from all ocurrences of PatchSet. This
> would lead us to find this second problematic piece of code:
> 
> src/Darcs/Patch/Bundle.hs:119:39: error:
>     • Could not deduce: wZ ~ Origin

How did you get that error? If I change the type of makeBundleN (which
is what is at line 119) to use Origin, then Bundle.hs compiles fine but
Darcs.UI.Commands.Push has an error which I can then fix by using Origin
there too.

> This and the problematic piece in Darcs.UI.Commands.Rebase could then be
> fixed, temporarily, by using UnsafePatchSet directly until we find a way
> to refactor these to avoid UnsafePatchSet.

Won't all the uses of Fork need to use UnsafePatchSet too? Plus things
like :> etc.

I'm not opposed to having a second type to reduce confusion but I don't
think 'Unsafe' in the name is necessary because we can make it not be
unsafe by constraining the constructor type. The constrained constructor
type gives us a global invariant that a [Unsafe]PatchSet can only start
from Origin.

An alternative view would be that a PatchSet-like-thing that doesn't
start from the Origin is independently useful (and not unsafe in
itself). Then we could give it a completely different name but make sure
that any code that does rely on the invariant only uses the PatchSet
synonym.

To be clear, I don't think either piece of code you identified above is
problematic, though perhaps the types are a bit misleading. To the
extent that any code implicitly assumes that the patch sequence starts
from Origin, that assumption is now validated by the constrained
constructor type.
msg21072 (view) Author: bfrk Date: 2019-08-10.12:00:05
I take back my counter proposal. Using Unsafe... all over the place is
not good. Yours is nicely uninvasive, but I would like us to do the
refactor in the rebase log command first.

> You can't get a non-bottom/unsafeCoerced PatchSet without wStart~Origin,
> so the signature is still correct.

Okay, I didn't get that.

>> src/Darcs/Patch/Bundle.hs:119:39: error:
>>     • Could not deduce: wZ ~ Origin
> 
> How did you get that error? 

My mistake. I replaced /every/ wStart with Origin in the module, but the
one in makeBundle2 is not an argument to a PatchSet but rather an RL.

> Won't all the uses of Fork need to use UnsafePatchSet too? Plus things
> like :> etc.

Yes.

> To be clear, I don't think either piece of code you identified above is
> problematic, though perhaps the types are a bit misleading. To the
> extent that any code implicitly assumes that the patch sequence starts
> from Origin, that assumption is now validated by the constrained
> constructor type.

With the exception of the unsafeCoerce you need for rebase log.

I think we can refactor the log and rebase log to share a smaller
function that doesn't take a PatchSet, just an RL. getLogInfo handles
matching options for a patchSet but rebase log doesn't need that, in
fact it doesn't have any matching options.
msg21079 (view) Author: ganesh Date: 2019-08-12.01:34:17
Here's a bit of a brute force refactoring to avoid passing
a PatchSet from rebase log.

I don't particularly like it as there's no principle
behind the new 'getLogInfoCore' but it should do the job.
Any better solutions would be welcome.

(as before I haven't tested this, only typechecked it -
I'll run the tests before pushing anything to screened)

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

patch 69a11e8e32c629e83503673014d487eb83239fd4
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Mon Aug 12 02:32:18 BST 2019
  * refactor the "darcs log" code to accept a list of patches
  
  "rebase log" was faking a PatchSet to call it, which
  violated the invariant observed elsewhere that all
  PatchSets should start from Origin.
Attachments
msg21080 (view) Author: ganesh Date: 2019-08-12.10:17:52
I've updated the PatchSet change to depend on the
"darcs log" refactoring (and obviously remove the unsafeCoerceP),
and added some comments to explain the Origin constraint.

I think this is ready for screened now but I'll give it a couple
of days in case there's a better refactoring.

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

patch 69a11e8e32c629e83503673014d487eb83239fd4
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Mon Aug 12 02:32:18 BST 2019
  * refactor the "darcs log" code to accept a list of patches
  
  "rebase log" was faking a PatchSet to call it, which
  violated the invariant observed elsewhere that all
  PatchSets should start from Origin.

patch 1ba8269fb7687826e432d0ddcd02e854ca7b5e0d
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Mon Aug 12 10:48:51 BST 2019
  * force PatchSet to always start at Origin
Attachments
msg21089 (view) Author: bfrk Date: 2019-08-12.15:49:57
I think I can improve on the refactor. BTW, it also conflicts with my
Matchable refactor.
msg21090 (view) Author: ganesh Date: 2019-08-12.15:57:58
OK, I'll leave it on hold for now.
msg21094 (view) Author: bfrk Date: 2019-08-12.20:58:09
> OK, I'll leave it on hold for now.

You can push it. What I wanted to do was to disentangle the matching
from conversion to the output of getLogInfo. This was more difficult
than I imagined, so for the moment I gave it up. I think this will be
easier to clean up in the course of a somewhat deeper refactor to patch
matching, that I haven't done yet but have ideas for.
msg21096 (view) Author: ganesh Date: 2019-08-12.22:47:39
final version with conflicts fixed, will screen it now

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

patch 09752ab266ec039f5dd4fc012b227d50299c05b9
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Mon Aug 12 23:11:52 BST 2019
  * refactor the "darcs log" code to accept a list of patches
  
  "rebase log" was faking a PatchSet to call it, which
  violated the invariant observed elsewhere that all
  PatchSets should start from Origin.

patch 6ceeac1d52fdfbb706e600cf1e9ed101ef62b592
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Mon Aug 12 23:11:56 BST 2019
  * force PatchSet to always start at Origin
Attachments
msg21147 (view) Author: bfrk Date: 2019-08-20.15:33:52
More type safety, at the very minor cost of a slightly confusing type signature; which is anyway more than compensated by the everything being well-documented.
History
Date User Action Args
2019-08-09 12:04:09ganeshcreate
2019-08-09 20:41:39ganeshsetstatus: needs-screening -> followup-in-progress
2019-08-10 10:27:59bfrksetmessages: + msg21069
2019-08-10 11:20:43ganeshsetmessages: + msg21071
2019-08-10 12:00:05bfrksetmessages: + msg21072
2019-08-12 01:34:17ganeshsetfiles: + patch-preview.txt, refactor-the-_darcs-log_-code-to-accept-a-list-of-patches.dpatch, unnamed
messages: + msg21079
2019-08-12 10:17:52ganeshsetfiles: + patch-preview.txt, refactor-the-_darcs-log_-code-to-accept-a-list-of-patches.dpatch, unnamed
messages: + msg21080
2019-08-12 10:19:16ganeshsetstatus: followup-in-progress -> needs-screening
title: WIP: force PatchSet to always start at Origin -> force PatchSet to always start at Origin
2019-08-12 15:49:57bfrksetmessages: + msg21089
2019-08-12 15:57:58ganeshsetstatus: needs-screening -> followup-in-progress
messages: + msg21090
2019-08-12 20:58:09bfrksetmessages: + msg21094
2019-08-12 22:47:39ganeshsetfiles: + patch-preview.txt, refactor-the-_darcs-log_-code-to-accept-a-list-of-patches.dpatch, unnamed
messages: + msg21096
2019-08-13 00:58:37ganeshsetstatus: followup-in-progress -> needs-review
2019-08-20 15:33:52bfrksetstatus: needs-review -> accepted
messages: + msg21147