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

• patch-preview.txt
• remove-most-of-darcs_patch_repotype.dpatch
• unnamed

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

• patch-preview.txt
• remove-most-of-darcs_patch_repotype.dpatch
• unnamed

I am screening this now since most of what I have in my queue depends 
on this.

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?

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.

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.

>>    * 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

• OpenPGP_0xD36E45316E58CC97.asc

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.

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.

>  * 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.

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?

>>    * 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.

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

• fix-docs-for-the-wr-type-parameter-to-repository.dpatch
• patch-preview.txt

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.

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

• add-docs-for-unsafestart_endtransaction.dpatch
• patch-preview.txt

🎉