Created on 2018-09-21.06:35:54 by bfrk, last changed 2018-11-25.12:34:13 by ganesh.
File name |
Status |
Uploaded |
Type |
Edit |
Remove |
cleanups-in-d_p_named_wrapped-and-d_p_rebase_container.dpatch
|
|
bfrk,
2018-09-25.20:20:24
|
application/x-darcs-patch |
|
|
correct-comment-about-witnesses.dpatch
|
|
ganesh,
2018-11-25.12:27:33
|
application/x-darcs-patch |
|
|
move-more-repo-paths-to-d_r_paths.dpatch
|
|
bfrk,
2018-09-21.06:35:53
|
application/x-darcs-patch |
|
|
move-more-repo-paths-to-d_r_paths.dpatch
|
|
bfrk,
2018-09-25.18:52:03
|
application/x-darcs-patch |
|
|
patch-preview.txt
|
|
bfrk,
2018-09-21.06:35:53
|
text/x-darcs-patch |
|
|
patch-preview.txt
|
|
bfrk,
2018-09-25.18:52:03
|
text/x-darcs-patch |
|
|
patch-preview.txt
|
|
bfrk,
2018-09-25.20:20:24
|
text/x-darcs-patch |
|
|
patch-preview.txt
|
|
ganesh,
2018-11-17.10:51:21
|
text/x-darcs-patch |
|
|
patch-preview.txt
|
|
ganesh,
2018-11-25.12:27:33
|
text/x-darcs-patch |
|
|
unnamed
|
|
bfrk,
2018-09-21.06:35:53
|
text/plain |
|
|
unnamed
|
|
bfrk,
2018-09-25.18:52:03
|
text/plain |
|
|
unnamed
|
|
bfrk,
2018-09-25.20:20:24
|
text/plain |
|
|
unnamed
|
|
gh,
2018-11-09.22:07:52
|
text/html |
|
|
unnamed
|
|
ganesh,
2018-11-17.10:51:21
|
text/plain |
|
|
unnamed
|
|
ganesh,
2018-11-25.12:27:33
|
text/plain |
|
|
windows-fix-for-rebase_new_style-test.dpatch
|
|
ganesh,
2018-11-17.10:51:21
|
application/x-darcs-patch |
|
|
See mailing list archives
for discussion on individual patches.
msg20316 (view) |
Author: bfrk |
Date: 2018-09-21.06:35:53 |
|
As announced on @darcs-devel here is my bundle for the rebase-storage
refactor. Note this doesn't touch the internal workings of rebase at all. It
is purely about how the rebase patch is stored in the repo. The on-disk
format is exactly the same as before, minus the meta data.
TODO: give the remains of Darcs.Patch.Named.Wrapped a better name and place
in the module hierarchy.
8 patches for repository http://darcs.net/screened:
patch 288d5f53f83e2fd1b72e66a0c37f4c729b531ef0
Author: Ben Franksen <ben.franksen@online.de>
Date: Mon Jul 2 18:58:26 CEST 2018
* move more repo paths to D.R.Paths
patch 5aa080f8f1bf87912d49ef4803a1f8ef07f14ef0
Author: Ben Franksen <ben.franksen@online.de>
Date: Tue Sep 18 19:00:40 CEST 2018
* store rebase patch at the repo layer instead of mixing it with normal patches
This does not yet do away with the WrappedNamed layer and the
RepoType/PatchType cruft, which will be done in a second and third step.
Some tests now fail which is due to bugs which are only weakly related to
the change made here, so will be fixed in a follow-up patch.
Note that this changes is incompatible in that previous versions of darcs
can't handle a repo with a new-style rebase in progress and vice versa. This
is something we cannot avoid unless we keep all the old code around, which
would reap us us of most of the benefits we get from this change.
patch f0e13101eb205765ade027749c5af445e79e5580
Author: Ben Franksen <ben.franksen@online.de>
Date: Tue Sep 18 04:10:46 CEST 2018
* two fixes in clone and convert import commands
The bug was in both cases that finalizeRepositoryChanges was not correctly
paired with revertRepositoryChanges. This was exposed by the new way of
storing the rebase patch, which crashes when it tries to rename the
tentative rebase patch back to its final version.
patch cb9d99d39ac946fbbb931464c24154be80e05290
Author: Ben Franksen <ben.franksen@online.de>
Date: Tue Sep 18 19:13:23 CEST 2018
* remove the WrappedNamed layer
patch 404143fadc2551ac409bdf850d9a10f7075cd939
Author: Ben Franksen <ben.franksen@online.de>
Date: Wed Sep 19 11:56:55 CEST 2018
* add PatchInfoAndG which is polymorphic in the named patch type
The standard PatchInfoAnd is now a type synonym that fixes the named patch
type as 'Named'. Unfortunately this required the addition of Eq2 constraints
in lots of places.
The goal of this generalization is to be able to convert old-style rebasing
repos, for which we need to read PatchInfoAndG with a simplified version of
the old WrappedNamed as the named patch type.
patch efa36333ad5e9073488930c0dd9912b47c68bb23
Author: Ben Franksen <ben.franksen@online.de>
Date: Thu Sep 20 01:10:29 CEST 2018
* use englishNum for correct grammar in rebase status line
patch 49d7d124a3bd725dff62d22f989679fdef4dc30c
Author: Ben Franksen <ben.franksen@online.de>
Date: Wed Sep 19 22:11:51 CEST 2018
* add command 'rebase upgrade'
This required a few additional refactorings mostly in D.R.Hashed. We lift a
local function to the top level that (lazily) reads the patches from a
single inventory. Since this does not return a PatchSet but only an RL of
PatchInfoAnd, we can generalize it to return PatchInfoAndG and so can be
used with WrappedNamed instead of Named. The WrappedNamed has been
resurrected and largely cut down to what is needed for this single purpose.
patch 4a12994d2b9d97fd0b81b87014a4ead7c0f95223
Author: Ben Franksen <ben.franksen@online.de>
Date: Thu Sep 20 15:56:15 CEST 2018
* reliably fail if we detect that an old-style rebase is in progress
The trick is to check if the repo type is tagged with SIsRebase, which means
that the repo format has rebase-in-progress, and then count the suspended
patches in the new-style rebase patch. If it is zero we can assume that we
have an old-style rebase in progress.
While the check itself is simple, making sure it is called with the right
parameters at the right time is not. One problem is that we must make an
exception for the 'rebase upgrade' command. This is achieved by adding a new
kind of RepoJob (OldRebaseJob) just for this command. A further complication
arises because startRebaseJob is called with an SIsRebase-typed repo
regardless of whether a rebase was in progress initially or not. In this
case we cannot decide whether to run the check based on the repo type alone,
but instead have to (re-)test the format stored in repository token.
Attachments
|
msg20318 (view) |
Author: bfrk |
Date: 2018-09-21.07:39:22 |
|
I am not screening this bundle yet. While it passes all tests,
including a new one I added (see the last patch in the bundle), this
is a breaking change that can cause previous darcs versions to
"crash" i.e. throw an unhandled exception (though this happens only
if you invoke it when a new-style rebase is in progress).
I want everyone to think about whether this is a price you would be
willing to pay for this cleanup.
|
msg20323 (view) |
Author: bfrk |
Date: 2018-09-22.21:11:28 |
|
One solution to avoid unhandled exceptions is to introduce a new
format, say new-syle-rebase-in-progress. This gives a slightly more
decent error message:
"""
ben@juliana[1]:.../darcs/store-rebase-at-repo-layer>darcs what
Unable to "darcs whatsnew" here.
Can't understand repository format: Unknown format: new-style-
rebase-in-progress
"""
However, this is now an error for /every/ command, whereas the crash
happens only for commands that modify the repo.
|
msg20324 (view) |
Author: ganesh |
Date: 2018-09-23.15:58:52 |
|
Sorry, still failing to keep up and reply intelligently to all the
great work you're doing :-) My immediate assumption was that a new
repository format was the right way to express a new rebase format. And
I think we *want* old darcs to refuse to touch such a repo, no matter
what command it's running.
|
msg20325 (view) |
Author: bfrk |
Date: 2018-09-23.17:59:37 |
|
Thanks Ganesh. Yes, I came to the same conclusion. One further
advantage of a new format flag is that we can now move patches
between repos regardless of whether there is a (new-style) rebase in
progress. This limitation has bitten me a few times in the past and
I am glad we can get rid of it now and without any additional
complications.
|
msg20326 (view) |
Author: bfrk |
Date: 2018-09-23.18:04:43 |
|
Ganesh, do you think I should screen this bundle? I have a number of
follow-ups waiting...
|
msg20329 (view) |
Author: ganesh |
Date: 2018-09-25.05:46:28 |
|
I have no objections to screening it. The approach with just
updating the rebase context in tentativelyAdd/tentativelyRemove does
look a lot simpler and I'm trying to remember why I didn't do it - I
do now vaguely remember trying it and giving up. But if the tests
pass I can't think what the problem could be, perhaps I was just too
nervous or the repository code was more complicated then.
|
msg20331 (view) |
Author: bfrk |
Date: 2018-09-25.18:52:03 |
|
Here is an extended bundle (same patches as before plus new ones). It is
mostly about compatibility stuff like adding a new format property, tests,
error messages etc, and one or two minor cleanups they depend upon.
14 patches for repository http://darcs.net/screened:
patch 288d5f53f83e2fd1b72e66a0c37f4c729b531ef0
Author: Ben Franksen <ben.franksen@online.de>
Date: Mon Jul 2 18:58:26 CEST 2018
* move more repo paths to D.R.Paths
patch 5aa080f8f1bf87912d49ef4803a1f8ef07f14ef0
Author: Ben Franksen <ben.franksen@online.de>
Date: Tue Sep 18 19:00:40 CEST 2018
* store rebase patch at the repo layer instead of mixing it with normal patches
This does not yet do away with the WrappedNamed layer and the
RepoType/PatchType cruft, which will be done in a second and third step.
Some tests now fail which is due to bugs which are only weakly related to
the change made here, so will be fixed in a follow-up patch.
Note that this changes is incompatible in that previous versions of darcs
can't handle a repo with a new-style rebase in progress and vice versa. This
is something we cannot avoid unless we keep all the old code around, which
would reap us us of most of the benefits we get from this change.
patch f0e13101eb205765ade027749c5af445e79e5580
Author: Ben Franksen <ben.franksen@online.de>
Date: Tue Sep 18 04:10:46 CEST 2018
* two fixes in clone and convert import commands
The bug was in both cases that finalizeRepositoryChanges was not correctly
paired with revertRepositoryChanges. This was exposed by the new way of
storing the rebase patch, which crashes when it tries to rename the
tentative rebase patch back to its final version.
patch cb9d99d39ac946fbbb931464c24154be80e05290
Author: Ben Franksen <ben.franksen@online.de>
Date: Tue Sep 18 19:13:23 CEST 2018
* remove the WrappedNamed layer
patch 404143fadc2551ac409bdf850d9a10f7075cd939
Author: Ben Franksen <ben.franksen@online.de>
Date: Wed Sep 19 11:56:55 CEST 2018
* add PatchInfoAndG which is polymorphic in the named patch type
The standard PatchInfoAnd is now a type synonym that fixes the named patch
type as 'Named'. Unfortunately this required the addition of Eq2 constraints
in lots of places.
The goal of this generalization is to be able to convert old-style rebasing
repos, for which we need to read PatchInfoAndG with a simplified version of
the old WrappedNamed as the named patch type.
patch efa36333ad5e9073488930c0dd9912b47c68bb23
Author: Ben Franksen <ben.franksen@online.de>
Date: Thu Sep 20 01:10:29 CEST 2018
* use englishNum for correct grammar in rebase status line
patch 49d7d124a3bd725dff62d22f989679fdef4dc30c
Author: Ben Franksen <ben.franksen@online.de>
Date: Wed Sep 19 22:11:51 CEST 2018
* add command 'rebase upgrade'
This required a few additional refactorings mostly in D.R.Hashed. We lift a
local function to the top level that (lazily) reads the patches from a
single inventory. Since this does not return a PatchSet but only an RL of
PatchInfoAnd, we can generalize it to return PatchInfoAndG and so can be
used with WrappedNamed instead of Named. The WrappedNamed has been
resurrected and largely cut down to what is needed for this single purpose.
patch 4a12994d2b9d97fd0b81b87014a4ead7c0f95223
Author: Ben Franksen <ben.franksen@online.de>
Date: Thu Sep 20 15:56:15 CEST 2018
* reliably fail if we detect that an old-style rebase is in progress
The trick is to check if the repo type is tagged with SIsRebase, which means
that the repo format has rebase-in-progress, and then count the suspended
patches in the new-style rebase patch. If it is zero we can assume that we
have an old-style rebase in progress.
While the check itself is simple, making sure it is called with the right
parameters at the right time is not. One problem is that we must make an
exception for the 'rebase upgrade' command. This is achieved by adding a new
kind of RepoJob (OldRebaseJob) just for this command. A further complication
arises because startRebaseJob is called with an SIsRebase-typed repo
regardless of whether a rebase was in progress initially or not. In this
case we cannot decide whether to run the check based on the repo type alone,
but instead have to (re-)test the format stored in repository token.
patch 70e7eb2a2f540344931740f1fa70d84133f6d64a
Author: Ben Franksen <ben.franksen@online.de>
Date: Sun Sep 23 15:07:10 CEST 2018
* adapt rebase tests to new style of storing rebase patch
It is now okay to transfer patches between repos with new-style rebase in
progress; these operations ignore the rebase patch.
patch 18a8aee9ee409ad91985dbdc39095c69f373f9ec
Author: Ben Franksen <ben.franksen@online.de>
Date: Sat Sep 22 14:28:39 CEST 2018
* avoid needless String/ByteString conversions when reading format file
patch d51fae030fb6090bce28e9c1fb682334b04be0aa
Author: Ben Franksen <ben.franksen@online.de>
Date: Sat Sep 22 17:36:12 CEST 2018
* factor yet another bunch of repo paths to D.R.Paths
patch 1ca018fa626ee2d62d1a3a79fa7e5c686f4037c9
Author: Ben Franksen <ben.franksen@online.de>
Date: Sat Sep 22 23:29:54 CEST 2018
* add new repo format new-style-rebase-in-progress
patch 25b1da56c6c7a3fa7ad8acb0dc630b67642de88d
Author: Ben Franksen <ben.franksen@online.de>
Date: Sun Sep 23 19:19:27 CEST 2018
* fix error messages for clone/transfer from repo with old-style rebase in progress
patch 33515bbbf9d2c7da70adf5a04441528c87c92805
Author: Ben Franksen <ben.franksen@online.de>
Date: Sun Sep 23 19:14:42 CEST 2018
* include clone command in test for new-style rebase
Attachments
|
msg20332 (view) |
Author: bfrk |
Date: 2018-09-25.18:53:57 |
|
Thanks Ganesh, I am screening them now.
|
msg20337 (view) |
Author: bfrk |
Date: 2018-09-25.20:20:24 |
|
Yet another follow-up patch for the rebase-storage refactor.
1 patch for repository http://darcs.net/screened:
patch b59a2d9bd57ad82c29c5bcbf1108688706a23c3a
Author: Ben Franksen <ben.franksen@online.de>
Date: Sun Sep 23 18:52:57 CEST 2018
* cleanups in D.P.Named.Wrapped and D.P.Rebase.Container
This mostly removes or fixes outdated docs.
Attachments
|
msg20451 (view) |
Author: bfrk |
Date: 2018-11-08.19:52:49 |
|
Status update: I have been using this version of rebase constantly
during the last month. It works reliably for me and I like that it
doesn't suffer from the restrictions we had for the old version,
which was mainly not being allowed to pull from a repo where a
rebase is in progress. It means I can now use rebase as a longer
term storage for experimental patches that do not work or even
compile yet.
|
msg20453 (view) |
Author: gh |
Date: 2018-11-09.22:07:52 |
|
Great! Clearly this is a big change that will go into darcs 2.16.
Guillaume
El jue., 8 de nov. de 2018 16:55, Ben Franksen <bugs@darcs.net> escribió:
>
> Ben Franksen <ben.franksen@online.de> added the comment:
>
> Status update: I have been using this version of rebase constantly
> during the last month. It works reliably for me and I like that it
> doesn't suffer from the restrictions we had for the old version,
> which was mainly not being allowed to pull from a repo where a
> rebase is in progress. It means I can now use rebase as a longer
> term storage for experimental patches that do not work or even
> compile yet.
>
> __________________________________
> Darcs bug tracker <bugs@darcs.net>
> <http://bugs.darcs.net/patch1731>
> __________________________________
> _______________________________________________
> darcs-devel mailing list
> darcs-devel@osuosl.org
> https://lists.osuosl.org/mailman/listinfo/darcs-devel
>
Attachments
|
msg20476 (view) |
Author: ganesh |
Date: 2018-11-17.09:44:04 |
|
Review below. I haven't really tried the new rebase yet myself - unfortunately
for bad reasons my own working build of darcs is based on a quite old fork of
the codebase.
> * move more repo paths to D.R.Paths
Fine
> * store rebase patch at the repo layer instead of mixing it with normal patches
Fine
> * two fixes in clone and convert import commands
Fine
> * add PatchInfoAndG which is polymorphic in the named patch type
Fine.
It wasn't immediately obvious to me why this led to needing new
constraints (Eq2, PatchListFormat) but it's not really a big deal.
> * use englishNum for correct grammar in rebase status line
Fine.
There's an unrelated change that happened to touch nearby lines, dropping the "qualified"
from an import, but that's fine too.
> * add command 'rebase upgrade'
> -- FIXME inlining this action below where it is used
> -- results in lots of ambiguous type variable errors
> -- which is rather strange behavior of ghc IMHO
Indeed, that's really weird. I played a bit more and it's the
*presence* of the update_repo definition that matters; you can inline it but
leave the definition itself, and also cut it down to
> PatchSet ts (mapRL_RL (fmapPIAP W.fromRebasing) wps')
and there's still no type error. But I didn't manage to simply further from
that expression or work out what's going on. I thought maybe I'd be able to
provoke the error by leaving the definition and messing around with the
monomorphism restriction ([No]{MonomorphismRestriction, MonoLocalBinds}) but
didn't get anywhere.
Anyway, patch is fine. I haven't thought through the logic line by line but
your description sounds plausible and the implementation appears to
match it.
> * reliably fail if we detect that an old-style rebase is in progress
Fine (again I haven't checked every line).
> * adapt rebase tests to new style of storing rebase patch
Fine
> * avoid needless String/ByteString conversions when reading format file
Fine - replacing String with ByteString in our types seems like a good
idea when there aren't any possible encoding issues.
> * factor yet another bunch of repo paths to D.R.Paths
Fine
> * add new repo format new-style-rebase-in-progress
Fine
> * fix error messages for clone/transfer from repo with old-style rebase in progress
Fine
> * include clone command in test for new-style rebase
Fine
> * cleanups in D.P.Named.Wrapped and D.P.Rebase.Container
> -- The witnesses are such that a @Suspended@ appears to have no effect.
> -- This is obsolete now and is also a semantically wrong. It is kept only
> -- because I (bf) am too lazy to fix it.
Can you explain this a bit more? I guess this is only wanted for
upgrading old-style rebases, but I didn't see why the witness model
for the old-style rebases would have changed.
Otherwise fine.
|
msg20479 (view) |
Author: ganesh |
Date: 2018-11-17.10:51:21 |
|
1 patch for repository darcs-unstable@darcs.net:screened:
patch a9c408138b0ab5b300ab99987bcf96f4826bd2d2
Author: Ganesh Sittampalam <ganesh@earth.li>
Date: Sat Nov 17 10:52:43 GMT 2018
* Windows fix for rebase-new-style test
C:\blah is interpreted by tar as a network path, so we have
to pipe it input instead.
Attachments
|
msg20526 (view) |
Author: bfrk |
Date: 2018-11-19.01:20:04 |
|
Am 17.11.18 um 10:44 schrieb Ganesh Sittampalam:
>> * add PatchInfoAndG which is polymorphic in the named patch type
>
> Fine.
>
> It wasn't immediately obvious to me why this led to needing new
> constraints (Eq2, PatchListFormat) but it's not really a big deal.
We abstract over the type of patch that is contained in a PatchInfoAnd.
Where previously this was fixed at (Named p) we now have only p. Named p
has instances for Eq and PatchListFormat but p in general has not. What
we usually have is RepoPatch p but that doesn't have these two as
superclasses.
>> * add command 'rebase upgrade'
>
>> -- FIXME inlining this action below where it is used
>> -- results in lots of ambiguous type variable errors
>> -- which is rather strange behavior of ghc IMHO
>
> Indeed, that's really weird. I played a bit more and it's the
> *presence* of the update_repo definition that matters; you can inline it but
> leave the definition itself, and also cut it down to
>
>> PatchSet ts (mapRL_RL (fmapPIAP W.fromRebasing) wps')
>
> and there's still no type error. But I didn't manage to simply further from
> that expression or work out what's going on. I thought maybe I'd be able to
> provoke the error by leaving the definition and messing around with the
> monomorphism restriction ([No]{MonomorphismRestriction, MonoLocalBinds}) but
> didn't get anywhere.
Yeah, I remember I did a few experiments along that line myself before
giving up. If we manage to cut this down to a stand-alone example we can
ask on @ghc-users what (TF) is going on here...
>> * reliably fail if we detect that an old-style rebase is in progress
>
> Fine (again I haven't checked every line).
That one took me a lot longer to get right than I expected.
>> * cleanups in D.P.Named.Wrapped and D.P.Rebase.Container
>
>> -- The witnesses are such that a @Suspended@ appears to have no effect.
>> -- This is obsolete now and is also a semantically wrong. It is kept only
>> -- because I (bf) am too lazy to fix it.
>
> Can you explain this a bit more? I guess this is only wanted for
> upgrading old-style rebases, but I didn't see why the witness model
> for the old-style rebases would have changed.
Ah, no it wouldn't. I wrote that comment before I realized that we need
to be able to read old-style rebase repos so we can upgrade them. That
means the comment is no longer correct, it should say "if we didn't need
the ability to read old-style rebase, then ...". BTW, I am glad I was
too lazy to "fix" the witnesses because I would have had to change them
back to implement rebase upgrade...
|
msg20556 (view) |
Author: ganesh |
Date: 2018-11-25.12:27:33 |
|
1 patch for repository darcs-unstable@darcs.net:screened:
patch 51687615f252db0e1ddf9422b8a679bb8e3ce370
Author: Ganesh Sittampalam <ganesh@earth.li>
Date: Sun Nov 25 12:29:13 GMT 2018
* correct comment about witnesses
Attachments
|
|
Date |
User |
Action |
Args |
2018-09-21 06:35:55 | bfrk | create | |
2018-09-21 07:39:22 | bfrk | set | messages:
+ msg20318 |
2018-09-22 21:11:28 | bfrk | set | messages:
+ msg20323 |
2018-09-23 15:58:53 | ganesh | set | messages:
+ msg20324 |
2018-09-23 17:59:37 | bfrk | set | messages:
+ msg20325 |
2018-09-23 18:04:43 | bfrk | set | messages:
+ msg20326 |
2018-09-25 05:46:29 | ganesh | set | messages:
+ msg20329 |
2018-09-25 18:52:05 | bfrk | set | files:
+ patch-preview.txt, move-more-repo-paths-to-d_r_paths.dpatch, unnamed messages:
+ msg20331 |
2018-09-25 18:53:57 | bfrk | set | status: needs-screening -> needs-review messages:
+ msg20332 |
2018-09-25 20:20:24 | bfrk | set | files:
+ patch-preview.txt, cleanups-in-d_p_named_wrapped-and-d_p_rebase_container.dpatch, unnamed messages:
+ msg20337 |
2018-11-08 19:52:50 | bfrk | set | messages:
+ msg20451 |
2018-11-09 22:07:53 | gh | set | files:
+ unnamed messages:
+ msg20453 |
2018-11-17 09:44:05 | ganesh | set | status: needs-review -> review-in-progress assignedto: ganesh messages:
+ msg20476 nosy:
+ ganesh |
2018-11-17 10:51:21 | ganesh | set | files:
+ patch-preview.txt, windows-fix-for-rebase_new_style-test.dpatch, unnamed messages:
+ msg20479 |
2018-11-19 01:20:06 | bfrk | set | messages:
+ msg20526 |
2018-11-25 12:27:33 | ganesh | set | files:
+ patch-preview.txt, correct-comment-about-witnesses.dpatch, unnamed messages:
+ msg20556 |
2018-11-25 12:34:13 | ganesh | set | status: review-in-progress -> accepted |
|