I regard these as internal refactors. Dropping support for --[un-]compress
(where it concerns hashed files) strictly speaking affects the UI but since
these options are "technical" I regard them as internal. Similarly, whether
or not to display certain messages and in what way is a user-visible change,
but it does not touch what darcs does to you repo or the working copy.

10 patches for repository http://darcs.net/screened:

patch 12b07218135b311f4fa0c5cc4319973dfbaa1020
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Mar 27 08:09:13 CET 2021
  * improve the interface of D.R.Inventory

  This combines the internal functions readInventoryPrivate and
  readPatchesFromInventoryEntries into readOneInventory, for use by
  upgradeOldStyleRebase.

patch 84765e19bee46b2ac1c6393d3125810f1e2f816a
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Nov 27 13:11:14 CET 2022
  * add Darcs.Repository.Transaction

  This is to avoid import cycles when adding functionality to new modules
  under Darcs.Repository, since this usually requires that we add things to
  revertRepositoryChanges and finalizeRepositoryChanges.

patch 6cd64873eaf0bb962ee6622c3a5f903850001584
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Nov 26 01:35:40 CET 2022
  * remove no longer needed export of diffHashLists

patch a1e63cc27cd56ba6c8f7a2043a5483b7f5de8c72
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Nov 29 01:25:28 CET 2022
  * cleanup cleanPristineDir and move from D.R.Pristine to D.R.Traverse

patch 5ed3cca90043c212cc56f44fd89b9e07a218cf01
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Jun 27 15:02:09 CEST 2022
  * repair: do not clean pristine dir

  I found that it can be quite useful that darcs never does any cleaning
  automatically (except for convert export where it actually makes sense),
  since this allows to restore things that would otherwise be completely lost
  e.g. after an erroneous obliterate.

patch 0f79880de0e328eaa79db700d62fcd1b798a9d8c
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Nov 27 19:56:06 CET 2022
  * always store hashed files in compressed form

  This removes the --[un-]compress option from most commands. The only
  remaining command that supports it is `push` since there it is used to
  control whether the bundle is sent in compressed or uncompressed from.
  Sending uncompressed is needed for compatibility with older (pre 2.5) remote
  darcs versions. The `optimize uncompress` subcommand is still available, as
  an uncompressed repository may be nice to have for debugging. Note that we
  are still able to detect and read uncompressed hashed files.

  Rationale: This removes an unnecessary complication (passing around an
  argument of type Compression wherever we may want to write a hashed file).
  It is also better to have only one binary format for hashed files.

patch c36367a6dedcdf905928fb3d36bfb4cb6515f08e
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Dec  3 12:54:53 CET 2022
  * clean up D.R.Clone.copyRepoOldFashioned

patch ac55c760fa0668aa440f7cf22dffbe63f463a6e5
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Dec  3 13:23:24 CET 2022
  * remove the ugly ApplyDir type in favour of Invertible wrapper

  This is much cleaner and allows us to give a more precise type signature for
  applyToTentativePristine. This also makes tentativelyRemovePatches_ the
  primitive and defines tentativelyAddPatch_ as a simple wrapper; this is more
  symmetric to tentativelyRemovePatches_ and also more efficient.

patch 5e41b359d4ca90106041018368beff557e109ab1
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Dec  3 15:21:38 CET 2022
  * eliminate the ugly and low-level applyToTentativePristineCwd

patch 0409f6095f45b8096b2e8615e0d5a37ca9199600
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Dec 15 16:53:16 CET 2022
  * applyToTentativePristine: replace verbose message with progress reporting

  This removes the verbosity parameter to that function and everywhere it gets
  passed around just because we call that function somewhere down the line.
  Instead we add progress reporting also when adding patches, symmetrical to
  what we already did when removing patches.

Attachments

• improve-the-interface-of-d_r_inventory.dpatch
• patch-preview.txt

Reviewing incrementally

>  * improve the interface of D.R.Inventory
>  * add Darcs.Repository.Transaction
>  * remove no longer needed export of diffHashLists

Fine

>   * cleanup cleanPristineDir and move from D.R.Pristine to 
D.R.Traverse

I think you've removed a call to okayHash, was that intentional?

>  * repair: do not clean pristine dir
>  * always store hashed files in compressed form

OK

>   * clean up D.R.Clone.copyRepoOldFashioned

OK (I haven't really checked/understood the changes)

>  * remove the ugly ApplyDir type in favour of Invertible wrapper

> This also makes tentativelyRemovePatches_ the
> primitive and defines tentativelyAddPatch_ as a simple wrapper

I got very confused by this note in the patch description until
I realised it meant to say make tentatively*Add*Patches_ the
primitive :-)

> (mkInvertible ps)
> invert $ mkInvertible

For what it's worth I find this a bit obfuscated and would probably
find just Fwd and Rev easier to read, if we chose to export them
from D.P.Invertible.

Anyway, the patch is basically fine.

>   * eliminate the ugly and low-level applyToTentativePristineCwd

Did you intend to drop the bunchFL calls in this change?

>   * applyToTentativePristine: replace verbose message with progress 
reporting

Looks fine.

>>   * cleanup cleanPristineDir and move from D.R.Pristine to D.R.Traverse
>
> I think you've removed a call to okayHash, was that intentional?

I think so: we want to remove all files in the pristine subdirectory that 
are not reachable from the root. Even more so (in german i'd say "erst 
recht") if they don't even have a valid name for a hashed file.

>> This also makes tentativelyRemovePatches_ the
>> primitive and defines tentativelyAddPatch_ as a simple wrapper
>
> I got very confused by this note in the patch description until
> I realised it meant to say make tentatively*Add*Patches_ the
> primitive :-)

OMG, I am sorry about that!

>> (mkInvertible ps)
>> invert $ mkInvertible
> For what it's worth I find this a bit obfuscated and would probably
> find just Fwd and Rev easier to read, if we chose to export them
> from D.P.Invertible.

Hmm, I don't get what you mean. We start with a patch that's not invertible. 
We apply mkIvertible, to makeit invertible. Then we actually invert it. 
What's obfuscated about this?

>>   * eliminate the ugly and low-level applyToTentativePristineCwd
>
> Did you intend to drop the bunchFL calls in this change?

Yes, because I don't see what they're good for. If you happen to know and can 
convince me that it makes a difference, I'll gladly put them back.

I didn't check to confirm it, but I suspect they come from a time before we 
had the TreeMonad, with its automatic limiting of memory use by regularly 
dumping TreeItems to disk (depending on the sum of the sizes of file changes). 
Indeed, applying patches to pristine is pretty fast and I have never seen it 
leak memory, even for very long histories like that of darcs itself. In fact I 
have often been thinking that we should use a similar techique when applying 
patches to working.

> We apply mkIvertible, to makeit invertible. Then we actually invert it. 
> What's obfuscated about this?

It was clear once I dug into the details, but I think my mind immediately
assumed that mkInvertible was actually inverting something and got
particularly confused in the case where we just call `mkInvertible` and
not `invert`.

Not a big deal, anyway.

> > I think you've removed a call to okayHash, was that intentional?

> I think so: we want to remove all files in the pristine subdirectory 
> that  are not reachable from the root. Even more so (in german i'd say 
> "erst recht") if they don't even have a valid name for a hashed file.

I see, so essentially any junk in there at all gets dropped. That seems
fair enough. It's a minor behaviour change but I doubt people are
using that folder to stash other content!

> It's a minor behaviour change but I doubt people are
> using that folder to stash other content!

https://xkcd.com/1172