Created on 2019-08-09.12:04:09 by ganesh, last changed 2019-08-20.15:33:52 by bfrk.
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.
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.
|
|
Date |
User |
Action |
Args |
2019-08-09 12:04:09 | ganesh | create | |
2019-08-09 20:41:39 | ganesh | set | status: needs-screening -> followup-in-progress |
2019-08-10 10:27:59 | bfrk | set | messages:
+ msg21069 |
2019-08-10 11:20:43 | ganesh | set | messages:
+ msg21071 |
2019-08-10 12:00:05 | bfrk | set | messages:
+ msg21072 |
2019-08-12 01:34:17 | ganesh | set | files:
+ patch-preview.txt, refactor-the-_darcs-log_-code-to-accept-a-list-of-patches.dpatch, unnamed messages:
+ msg21079 |
2019-08-12 10:17:52 | ganesh | set | files:
+ patch-preview.txt, refactor-the-_darcs-log_-code-to-accept-a-list-of-patches.dpatch, unnamed messages:
+ msg21080 |
2019-08-12 10:19:16 | ganesh | set | status: 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:57 | bfrk | set | messages:
+ msg21089 |
2019-08-12 15:57:58 | ganesh | set | status: needs-screening -> followup-in-progress messages:
+ msg21090 |
2019-08-12 20:58:09 | bfrk | set | messages:
+ msg21094 |
2019-08-12 22:47:39 | ganesh | set | files:
+ patch-preview.txt, refactor-the-_darcs-log_-code-to-accept-a-list-of-patches.dpatch, unnamed messages:
+ msg21096 |
2019-08-13 00:58:37 | ganesh | set | status: followup-in-progress -> needs-review |
2019-08-20 15:33:52 | bfrk | set | status: needs-review -> accepted messages:
+ msg21147 |
|