Created on 2021-06-09.14:50:14 by bfrk, last changed 2023-07-15.00:35:18 by ganesh.
See mailing list archives
for discussion on individual patches.
msg22879 (view) |
Author: bfrk |
Date: 2021-06-09.14:50:09 |
|
Here is the first batch of "interesting" changes that redefines the rt
(RepoType) parameter and removes the witness for the tentative state. It
also makes all repo state follow the transaction protocol.
I am not screening this bundle immediately to leave room for discussion, as
these are quite far-reaching changes.
13 patches for repository http://darcs.net/screened:
patch 241eb0fdc7c2f731dca0386aee7b6ab2f6e0fa61
Author: Ben Franksen <ben.franksen@online.de>
Date: Tue Jan 19 16:00:23 CET 2021
* remove most of Darcs.Patch.RepoType
This removes the type distinction between repos with and without a rebase in
progress. The 'rt :: RepoType' type parameter (and its kind RepoType) are
kept for now, in case we ever want to re-introduce such a type distinction
for some other purpose.
patch 00777493abb3256e84992afb3d2d47f1c2d57111
Author: Ben Franksen <ben.franksen@online.de>
Date: Wed Feb 24 23:57:16 CET 2021
* remove rt::RepoType type parameter from PatchSet and PatchInfoAnd
patch 4c85d8b43eaae6410724e9d19a2532fc84960952
Author: Ben Franksen <ben.franksen@online.de>
Date: Thu Feb 25 00:04:03 CET 2021
* re-type the rt type parameter to AccessType=RO|RW
This patch does not yet go the full way of changing all Repository access
procedures, only some of the central pieces: revertRepositoryChanges and
finalizeRepositoryChanges, withRepoLock, repo creation and identification.
Commands that call withRepoLock now take the lock even when --dry-run is in
effect. This fixes a potential race condition because in dry-run mode these
commands still modified the tentative state on disk. Instead, the dry-run
option is now evaluated in the finalizing procedures (which includes,
currently, addPendingDiffToPending and addToPending).
patch 5c084c8f97a20c5291fedadfef4c226a875f29c3
Author: Ben Franksen <ben.franksen@online.de>
Date: Thu Feb 25 16:45:59 CET 2021
* convert D.R.Hashed functions to take access typed Repository
All procedures in D.R.Hashed that access the tentative state now take a
Repository 'RW, while 'readPatches' stays polymorphic, but reads the
tentative or the recorded state, depending on the Repository access type
parameter. This required to export SAccessType from D.R.InternalTypes and
return that type from repoAccessType, so that pattern matching on the result
forces unification of the Repository's access type.
patch 64008265cf5bb7b1e0619d286cb70d9b4ec54b08
Author: Ben Franksen <ben.franksen@online.de>
Date: Sat Feb 27 09:49:25 CET 2021
* convert size-prefixed pristine when we start a transaction
We must not modify _darcs/hashed_inventory except when finalizing a
transaction.
patch ecfc634b88fdf307c247e05b3797e8f9cc44d76e
Author: Ben Franksen <ben.franksen@online.de>
Date: Thu Feb 25 22:27:10 CET 2021
* writeTentativeInventory now takes a Repository argument
patch 3fd6d87619d098cdeadbe6316f7887c29f7141e9
Author: Ben Franksen <ben.franksen@online.de>
Date: Sat Feb 27 11:23:18 CET 2021
* convert D.R.Pristine to assign the correct access type parameters
The same schema as in D.R.Hashed applies: functions that modify the
tentative pristine are only available for 'RW typed repos, those that only
read pristine decide at runtime which files to read and stay polymorphic.
A special (and ugly) case is replacePristine which has a case for 'RO typed
repos, which should only be used when creating a fresh repo. The 'RW case
now modifies the tentative pristine, so we need to add missing finalization
in the repair and optimize pristine commands.
patch d2669ee09f8598b4054e2f663b3aa08abc147623
Author: Ben Franksen <ben.franksen@online.de>
Date: Mon Mar 1 16:27:02 CET 2021
* fix rt ~ 'RO in AskAboutDeps
patch ba73b15970f91c8d37e80c83c1ff8a32b475a391
Author: Ben Franksen <ben.franksen@online.de>
Date: Sat Feb 27 11:14:54 CET 2021
* replace readTentativePristine with the generalized readPristine
The latter now now does the right thing for both RO and RW repos.
patch f31a6374dcee92fc0312ed1245cf7ba504f9b871
Author: Ben Franksen <ben.franksen@online.de>
Date: Thu Feb 25 12:54:51 CET 2021
* lock/unlock inside revert/finalizeRepositoryChanges
This has the disadvantage that intermediate commits also unlock the
repository for a short time. However, intermediate commits are mostly
unnecessary, the only remaining instance is in convert darcs-2, which will
soon become obsolete anyway.
patch 524d393e6dbb75218508be13ed74dc4487a03dd5
Author: Ben Franksen <ben.franksen@online.de>
Date: Mon Mar 1 16:16:35 CET 2021
* remove the tentative state witness parameter for Repository
Since we now distinguish at the type level and at runtime whether we are in
a transaction or not, it no longer makes sense to keep the distinction
between the recorded and the tentative state: inside a transaction the
recorded state is irrelevant and should be ignored, while outside a
transaction the tentative state should be ignored and only the recorded
state is relevant. When we start a transaction both states coincide by
definition (we throw away any existing tentative state). When we end a
transaction, we throw away the recorded state and overwrite it with the
tentative one.
Note that not all of Darcs.Repository has been changed yet to follow the
transaction protocol yet, in particular the pending and unrevert states.
This will be done in a later patch because the distinction between recorded
and tentative state gets in the way of making the decision at runtime when
reading the state. Also note that while this patch touches many lines of
code, the changes are very systematic and mostly mechanical.
patch c36151bdd403a6516afe1d022ef0fc55aaacaf1f
Author: Ben Franksen <ben.franksen@online.de>
Date: Wed Mar 3 15:12:32 CET 2021
* make D.R.Traverse procedures RW so they work on the tentative state
This required fixing convert import where cleanRepository was called after
finalizing the transaction.
patch 878794434cee4b9ca72dad57e90c5a6e51748827
Author: Ben Franksen <ben.franksen@online.de>
Date: Tue Mar 2 10:29:35 CET 2021
* make pending and unrevert follow transaction protocol
This means that all modifications are made to the tentative versions and
that we define and systematically use revert/finalize.
Note that the implementations of revertTentativeUnrevert and
finalizeTentativeUnrevert are tricky: the case where we have no unrevert
bundle present must be handled carefully.
Attachments
|
msg22883 (view) |
Author: bfrk |
Date: 2021-06-19.07:14:03 |
|
This is the same bundle as sent previously, but with one more patch:
patch b9117bbe58bad86463ed390c75aba9a2539a2c30
Author: Ben Franksen <ben.franksen@online.de>
Date: Tue Mar 2 11:48:56 CET 2021
* get rid of "nasty hack" in applyPatchesForRebaseCmd
This eliminates the intermediate commits in 'rebase suspend' and 'rebase
apply'. In the long comment for
patch f31a6374dcee92fc0312ed1245cf7ba504f9b871
Author: Ben Franksen <ben.franksen@online.de>
Date: Thu Feb 25 12:54:51 CET 2021
* lock/unlock inside revert/finalizeRepositoryChanges
I claimed that 'convert darcs-2' was the only remaining use of intermediate
commits, but that is true only after applying this patch, too.
Attachments
|
msg22884 (view) |
Author: bfrk |
Date: 2021-06-19.07:19:14 |
|
I am screening this now since most of what I have in my queue depends
on this.
|
msg23130 (view) |
Author: ganesh |
Date: 2023-02-26.23:02:15 |
|
Partial review:
> * remove most of Darcs.Patch.RepoType
OK
> * remove rt::RepoType type parameter from PatchSet and PatchInfoAnd
OK
> * re-type the rt type parameter to AccessType=RO|RW
> +unsafeStartTransaction :: Repository 'RO p wR wU wT -> Repository
'RW p wR wU wT
> +unsafeStartTransaction (Repo l f p c SRO) = Repo l f p c SRW
> +
> +unsafeEndTransaction :: Repository 'RW p wR wU wT -> Repository
'RO p wR wU wT
> +unsafeEndTransaction (Repo l f p c SRW) = Repo l f p c SRO
Given these are "unsafe", a comment explaining why and what the pre-
conditions are would be helpful.
Also why is unsafeEndTransaction unsafe, given that it removes
"permissions" from the repo object?
|
msg23131 (view) |
Author: bfrk |
Date: 2023-02-27.12:36:49 |
|
Both unsafeStartTransaction and unsafeEndTransaction are "unsafe" in the sense
that they merely "coerce" the type but do not actually perform the steps (IO
actions) required to start or end a transaction (this is done by
revertRepositoryChanges and finalizeRepositoryChanges). Technically this is not
an actual coercion like with e.g. unsafeCoerceR, due to the singleton typed
member, but in practical terms it is no less unsafe, because RO vs. RW changes
whether wR refers to the recorded or the tentative state, respectively. In
particular, you may get wrong results if you are inside a transaction and read
the patchset with a "coerced" Repository of access type 'RO. I therefore regard
unsafeEndTransaction as unsafe in the same way that unsafeStartTransaction is.
(BTW I am not wedded to the "unsafe" prefix and open to any reasonable proposal
for renaming these two functions.)
It is not easy to state precise/formal preconditions for their safe use.
Informally speaking it comes down to "use them only when actually starting/ending
a transaction".
I'll try to turn these explanations into proper documentation.
|
msg23132 (view) |
Author: ganesh |
Date: 2023-03-02.20:32:35 |
|
Thanks for the explanation about `unsafeEnd`, I'd forgotten that `RW` isn't just a
statement of permissions but also involves on-disk state.
More incremental review:
> * convert D.R.Hashed functions to take access typed Repository
> -readPatchesHashed = readPatchesUsingSpecificInventory hashedInventoryPath
> +readPatchesHashed repo =
> + case repoAccessType repo of
> + SRO -> readPatchesUsingSpecificInventory hashedInventoryPath repo
> + SRW -> readPatchesUsingSpecificInventory tentativeHashedInventoryPath repo
I guess this is in theory a bug fix? There are a lot of call sites of `readPatches`
which ends up calling this and I don't know if they were all consistent in being
outside a transaction.
Looks fine, anyway.
> * convert size-prefixed pristine when we start a transaction
OK. (I didn't check this in detail. I guess it's just a refactoring to do with the
timing of when we do this.)
> * writeTentativeInventory now takes a Repository argument
OK.
|
msg23133 (view) |
Author: bfrk |
Date: 2023-03-04.08:24:40 |
|
>> * convert D.R.Hashed functions to take access typed Repository
>
>> -readPatchesHashed = readPatchesUsingSpecificInventory hashedInventoryPath
>> +readPatchesHashed repo =
>> + case repoAccessType repo of
>> + SRO -> readPatchesUsingSpecificInventory hashedInventoryPath repo
>> + SRW -> readPatchesUsingSpecificInventory tentativeHashedInventoryPath repo
>
> I guess this is in theory a bug fix?
Actually this introduces a bug. The type signature
readPatchesHashed :: RepoPatch p => Repository rt p wR wU wT
-> IO (PatchSet p Origin wR)
claims to return a PatchSet ending in state wR, but it now returns one
that may end in state wR or wT, depending on rt. This is fixed in
patch 524d393e6dbb75218508be13ed74dc4487a03dd5
Author: Ben Franksen <ben.franksen@online.de>
Date: Mon Mar 1 16:16:35 CET 2021
* remove the tentative state witness parameter for Repository
where we identify wR and wT in favour of solely relying on rt. Perhaps
it was a mistake to separate these two changes.
> There are a lot of call sites of `readPatches`
> which ends up calling this and I don't know if they were all consistent in being
> outside a transaction.
The opposite is the case, we always call readPatches from inside the
transaction (simply because starting the transaction is done from within
withRepoLock, immediately after identifying the repo to work with).
However, back then it was only ever called before making any changes,
i.e. where we still have wR ~ wT.
One of the goals of these refactors was to get rid of that limitation
i.e. allow code to call readPatches (and similar procedures) after
making changes and get back not the original (unchanged) PatchSet, but
the current (tentative) one. For an example of where this limitation
hurts, see rebase apply/pull, where we had to split the command into two
separate transactions.
Attachments
|
msg23134 (view) |
Author: bfrk |
Date: 2023-03-04.09:11:00 |
|
Again, it seems that thunderbird has messed up the formatting of my
last response (extra newlines everywhere). I must remember not to
reply per email. Why do they unnecessarily breaks things in this way?
Replying per email used to work perfectly fine a few months ago IIRC.
|
msg23135 (view) |
Author: bfrk |
Date: 2023-03-04.13:55:03 |
|
Here is msg23133 again, from my browser:
>> * convert D.R.Hashed functions to take access typed Repository
>
>> -readPatchesHashed = readPatchesUsingSpecificInventory hashedInventoryPath
>> +readPatchesHashed repo =
>> + case repoAccessType repo of
>> + SRO -> readPatchesUsingSpecificInventory hashedInventoryPath repo
>> + SRW -> readPatchesUsingSpecificInventory tentativeHashedInventoryPath
repo
>
> I guess this is in theory a bug fix?
Actually this introduces a bug. The type signature
readPatchesHashed :: RepoPatch p => Repository rt p wR wU wT
-> IO (PatchSet p Origin wR)
claims to return a PatchSet ending in state wR, but it now returns one that
may end in state wR or wT, depending on rt. This is fixed in
patch 524d393e6dbb75218508be13ed74dc4487a03dd5
Author: Ben Franksen <ben.franksen@online.de>
Date: Mon Mar 1 16:16:35 CET 2021
* remove the tentative state witness parameter for Repository
where we identify wR and wT in favour of solely relying on rt. Perhaps it was
a mistake to separate these two changes.
> There are a lot of call sites of `readPatches`
> which ends up calling this and I don't know if they were all consistent in
being
> outside a transaction.
The opposite is the case, we always call readPatches from inside the
transaction (simply because starting the transaction is done from within
withRepoLock, immediately after identifying the repo to work with). However,
back then it was only ever called before making any changes, i.e. where we
still have wR ~ wT.
One of the goals of these refactors was to get rid of that limitation i.e.
allow code to call readPatches (and similar procedures) after making changes
and get back not the original (unchanged) PatchSet, but the current
(tentative) one. For an example of where this limitation hurts, see rebase
apply/pull, where we had to split the command into two separate transactions.
|
msg23143 (view) |
Author: ganesh |
Date: 2023-03-18.18:07:26 |
|
> * convert D.R.Pristine to assign the correct access type parameters
OK
> * fix rt ~ 'RO in AskAboutDeps
~ 'RW, right? but fine.
> * replace readTentativePristine with the generalized readPristine
OK
> * lock/unlock inside revert/finalizeRepositoryChanges
> + atexit (releaseLock lock)
I'm not keen on increasing the use of `atexit` compared to `bracket` -
it makes the code less modular and is likely to be particularly
problematic when using darcs as a library. I also didn't fully
understand the motivation for this change.
> * remove the tentative state witness parameter for Repository
Yay!
> +-- [@wR@] the witness for the recorded state of the repository,
> +-- (what darcs get would retrieve).
This isn't quite right when a transaction is in progress, is it?
> * make D.R.Traverse procedures RW so they work on the tentative state
OK
> * make pending and unrevert follow transaction protocol
OK - I'm afraid I didn't manage to check this properly.
|
msg23144 (view) |
Author: ganesh |
Date: 2023-03-18.18:09:53 |
|
I've now reviewed every patch in this bundle (phew!).
There are some outstanding comments, which given the tardiness of my
review I certainly won't insist on :-)
Maybe you can confirm when you've done all the followups you want to
(if any) and we can close this one?
|
msg23153 (view) |
Author: bfrk |
Date: 2023-03-20.21:51:54 |
|
>> * fix rt ~ 'RO in AskAboutDeps
>
> ~ 'RW, right? but fine.
Ahem, yes.
>> * lock/unlock inside revert/finalizeRepositoryChanges
>
>> + atexit (releaseLock lock)
>
> I'm not keen on increasing the use of `atexit` compared to `bracket` -
> it makes the code less modular and is likely to be particularly
> problematic when using darcs as a library. I also didn't fully
> understand the motivation for this change.
The motivation was to make sure that the repo is locked whenever we are
in a transaction. However, I think I went a little too far here and your
arguments against that move are pretty convincing.
I will roll this one back.
>> +-- [@wR@] the witness for the recorded state of the repository,
>> +-- (what darcs get would retrieve).
>
> This isn't quite right when a transaction is in progress, is it?
Correct, good catch. Will fix.
I will send the two follow-ups as a separate bundle but attached to this
ticket.
|
msg23160 (view) |
Author: bfrk |
Date: 2023-03-22.15:03:02 |
|
2 patches for repository http://darcs.net/screened:
patch 39bcf72323b3b37eca59afa32de973d597863ef2
Author: Ben Franksen <ben.franksen@online.de>
Date: Mon Mar 20 21:30:24 CET 2023
* fix docs for the wR type parameter to Repository
patch f9887b257f5bb9d353ba9f034f1abb6c124ccc21
Author: Ben Franksen <ben.franksen@online.de>
Date: Mon Mar 20 21:51:51 CET 2023
* rollback "lock/unlock inside revert/finalizeRepositoryChanges"
The motivation for that patch was to ensure that the repo is always locked
inside a transaction. However, the price for that is considerable: bracket
is much cleaner than atexit and plays better with use of darcs as a library.
Attachments
|
msg23165 (view) |
Author: ganesh |
Date: 2023-03-22.21:53:27 |
|
I think the only (maybe) outstanding thing on this patch is this note
about documenting `unsafe{Start,End}Transaction`:
> I'll try to turn these explanations into proper documentation.
|
msg23170 (view) |
Author: bfrk |
Date: 2023-03-22.23:43:04 |
|
Following up on review.
1 patch for repository http://darcs.net/screened:
patch 7e6bfb6c4b0dc834b218ce7468e8616c65c9bccf
Author: Ben Franksen <ben.franksen@online.de>
Date: Thu Mar 23 00:41:10 CET 2023
* add docs for unsafeStart/EndTransaction
Attachments
|
msg23173 (view) |
Author: ganesh |
Date: 2023-03-23.00:26:40 |
|
🎉
|
|
Date |
User |
Action |
Args |
2021-06-09 14:50:14 | bfrk | create | |
2021-06-19 07:14:04 | bfrk | set | files:
+ patch-preview.txt, remove-most-of-darcs_patch_repotype.dpatch, unnamed messages:
+ msg22883 |
2021-06-19 07:16:07 | bfrk | set | files:
- patch-preview.txt |
2021-06-19 07:16:14 | bfrk | set | files:
- remove-most-of-darcs_patch_repotype.dpatch |
2021-06-19 07:16:16 | bfrk | set | files:
- unnamed |
2021-06-19 07:19:14 | bfrk | set | status: needs-screening -> needs-review messages:
+ msg22884 |
2023-02-26 23:02:15 | ganesh | set | status: needs-review -> review-in-progress messages:
+ msg23130 |
2023-02-27 12:36:50 | bfrk | set | messages:
+ msg23131 |
2023-03-02 20:32:45 | ganesh | set | messages:
+ msg23132 |
2023-03-04 08:24:46 | bfrk | set | files:
+ OpenPGP_0xD36E45316E58CC97.asc messages:
+ msg23133 |
2023-03-04 09:11:01 | bfrk | set | messages:
+ msg23134 |
2023-03-04 13:55:09 | bfrk | set | messages:
+ msg23135 |
2023-03-18 18:07:28 | ganesh | set | messages:
+ msg23143 |
2023-03-18 18:09:54 | ganesh | set | status: review-in-progress -> followup-in-progress messages:
+ msg23144 |
2023-03-20 21:51:56 | bfrk | set | messages:
+ msg23153 |
2023-03-22 15:03:02 | bfrk | set | files:
+ patch-preview.txt, fix-docs-for-the-wr-type-parameter-to-repository.dpatch messages:
+ msg23160 |
2023-03-22 21:53:27 | ganesh | set | messages:
+ msg23165 |
2023-03-22 23:43:04 | bfrk | set | files:
+ patch-preview.txt, add-docs-for-unsafestart_endtransaction.dpatch messages:
+ msg23170 |
2023-03-23 00:26:40 | ganesh | set | status: followup-in-progress -> accepted-pending-tests messages:
+ msg23173 |
2023-07-15 00:35:18 | ganesh | set | status: accepted-pending-tests -> accepted |
|