Many refactorings and cleanups, mostly for Darcs.Repository.Hashed. The
most drastic measure is to decouple the code that handles parsing and
unparsing of inventories into its own module Darcs.Repository.Inventory
and to create appropriate data types. I also added properties and unit
tests.

There remains a lot to be done in this area but I think this is already
an improvement and it passes all tests. As usual I got side-tracked with
an experimental change, in this case a new parsing library for darcs
with the ability to return partially parsed values. The main use case
would be the extraction of the pristine hash of a head inventory without
having to parse all the rest of the content. This is currently done with
ad-hoc functions which I renamed to peekPristineHash, pokePristineHash,
and skipPristineHash.

I'll write more about this in a separate message to the list.

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

patch 76f5a3e21f7d1b94ac392b91bfe3e19ed2689c87
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Mar 11 10:21:22 CET 2018
  * add prefsDir and prefsDirPath, similar to constants defined in
D.R.Hashed

patch 820f125a4a14892f466b5835aa195c97d829d77e
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Mar 11 10:23:26 CET 2018
  * renamed inv2pris -> peekPristineHash

patch 56abe25ec95df0f9ff46b2f255d88bac268374c5
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Mar 11 10:27:09 CET 2018
  * export constants for repo subpaths in D.R.Hashed, use them in D.R.Create

patch 16e75cb7b2060abc3c63545493fe06f381a8b9e1
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Mar 11 12:09:26 CET 2018
  * use makeDarcsdirPath and path constants in D.R.Hashed

patch 2510b28c8f7a28d47f58e1273007bca39dc58446
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Mar 10 12:35:51 CET 2018
  * cleaned up set/unset in D.R.Hashed and optimize command
  
  This mess should never have passed patch review, if you ask me.

patch 16cef7605717d670f63f30b4eaba53f97e5d9355
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Mar 11 10:15:55 CET 2018
  * export the type SM from ReadMonads and reformat export list

patch 5deabd7c9657f61bcd52f406fb838adf130a9ccf
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Mar 11 11:38:15 CET 2018
  * factor D.R.Hashed.Inventory out of D.R.Hashed, add data type for
inventories
  
  This is a first step to making the huge D.R.Hashed more modular.

patch d49fa3b34bd251194138b6ab6fa91644e54819e3
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Mar 11 12:38:55 CET 2018
  * add DirLayout data type to abstrcat over plain vs. bucketed in
D.R.Hashed

patch 2e1caa54e72b7743e809f497a345ccccd5dc8595
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Mar 13 00:12:04 CET 2018
  * add properties and unit tests for D.R.Inventory
  
  For the usual parse/show roundtrip property we need a newtype wrapper for
  Strings that are valid hashes according to D.R.Cache.okayHash; which is a
  good idea anyway, so we expose ValidHash as part of the Inventory type.

patch 424a014d9faf5ffc5d3a98304777623aa4cef517
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Mar 13 10:02:45 CET 2018
  * add -fno-warn-orphans for the whole test suite
  
  ...and remove the individual OPTIONS_GHC pragmas with the same effect. For
  an executable,orphan instances are not a serious problem and we use
them in
  quite a lot of places.

patch c3a78de013d89cd82a03ea36bceeae3ec088683a
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Mar 17 14:42:22 CET 2018
  * cleanups in D.R.Inventory (layout, comments)

patch 0d3a18aa57e7a8d06816709c3874a41852404cb7
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Mar 13 14:02:11 CET 2018
  * pass the full path name of the head inventory to
readInventoryPrivate/Local

patch fc550ff9fdaac398804ade878a0eba7c2e8795f6
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Mar 18 23:18:28 CET 2018
  * fix, adapt, and add haddocks in D.R.Hashed

Attachments

• add-prefsdir-and-prefsdirpath_-similar-to-constants-defined-in-d_r_hashed.dpatch

This is a big bundle so I am not screening it immediately.

Attached a new bundle with two further patches.

Attachments

• add-prefsdir-and-prefsdirpath_-similar-to-constants-defined-in-d_r_hashed.dpatch

Hi Ben,

Great bundle. These tests for inventory parsing and writing are an
important milestone. I agree with enabling no-warn-instances across the
whole source code: the cleanups in the plain vs bucketed caches are also
welcome.

You can screen the bundle as it is, if you want. I have a couple of
non-essential suggestions:

* we probably do not need the opinionated long comment of the patch
about set/unset
* avoid leaving out commented functions in D.R.Inventories
* the SM-exporting patch could be merged with the patch that uses this
import

Guillaume

Thanks. Some comments/explanations:

* we probably do not need the opinionated long comment of the patch
about set/unset

You are right, it doesn't belong there. Will fix.

* avoid leaving out commented functions in D.R.Inventories

I left them in by intention, so they can serve as a hint as to the
simpler API we would want to have here, ideally. This API would
currently be less efficient, as it would mean we must parse the whole
inventory when we only want to access the pristine hash. Which is why I
kept the lower level peek/poke/skip interface to the pristine hash.

In hindsight, adding the pristine hash to _darcs/hashed_inventory was a
bad idea. This reference should be in a separate file so it can be
queried and updated independently.

I will add a comment in the module to explain these things.

* the SM-exporting patch could be merged with the patch that uses this
import

I had second thoughts about this. Apparently, the D.P.ReadMonads module
was intended to be used polymorphically, except when finally calling
parseStrictly on the parsers. So the concrete SM type is not needed. I
made a follow-up patch that rolls back the export of SM and uses
(ParserM m =>) constraints instead.

I think I will rebase the bundle to address these points.

Okay, here is the rebased bundle. I have added one additional patch that
introduces a few more newtypes to allow distinction between hashes for
inventories, patches, and pristine trees at the type level. I am not
100% happy with it because it should be really be in a separate module,
but it works for now and I have added a TODO comment for further
restructuring, which should, at one point, include D.R.Cache.

There remains a lot to do. For instance, these hashes are perfect
candidates to internally represent with ByteString instead of String
(they use only a small subset of ASCII). For hashed storage we'd ideally
 want to have IO functions that take ByteString for file paths, but I
fear this may cause problems on Windows.

Oops, forgot to attach the bundle, here we go.

Attachments

• add-prefsdir-and-prefsdirpath_-similar-to-constants-defined-in-d_r_hashed.dpatch

I am screening these patches now, except the last one, until gh had a
chance to take a look at it.

This last patch looks good to me (also it is indeed an improvement in
code quality).

Maybe you can use the GeneralizedNewtypeDeriving extension in
Darcs.Test.Repository.Inventory to avoid writing the Arbitrary instances
for the newtypes.

I've thought about that. I am a big fan of GeneralizedNewtypeDeriving.
But to make it work in this case we'd have to make String an instance of
ValidHash and I think this would undermine the very idea of the newtypes.

Screening the last (for now) patch in this series.

I had another look at the bundle, it its fine.

(I cannot push it to reviewed since it depends on
http://bugs.darcs.net/patch1662 which depends on
http://bugs.darcs.net/patch1636 )