A series of refactors to clean up specific parts of the index code. I
followed your (Ganesh's) example and did this as a series of small patches
to convince me that all changes are correct and don't change behavior. This
worked out pretty well: all tests succeeded the first time I ran them.

The part I was concerned about is how path names are extracted from the
index, which involved some "magic" index calculations (as was observed by
others before). This is hopefully much clearer now.

A follow-up patch will clean up construction of Names to make it impossible
to create invalid Names.

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

patch 646c519cab8996f3396e634d705f169cf1cf9d97
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Feb 20 21:26:33 CET 2020
  * index: fix import layout

patch 685de35cb2d0c7a68eca69c9d8a38430fa979a2e
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Feb 20 21:31:52 CET 2020
  * index: fix layout of getFileID'

patch 63e2e45030b48bb09e930a73bb87d2c72f89fb81
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Feb 20 21:42:21 CET 2020
  * index: factor nameof and namelength to top level

patch 24188eb914788511b905bf0d2247d42f017640c9
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Feb 20 21:46:48 CET 2020
  * index: refactor second argument of nameof
  
  It was always called with the dirlength of a state.

patch c4446b814585b63430501391db733cd6ba004a27
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Feb 20 21:54:45 CET 2020
  * index: factor out substateof

patch 3f92c16db820c458c51e5d95d5ee86a4fe587e3b
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Feb 20 22:10:49 CET 2020
  * index: use compare in local 'subs' functions

patch 59a12dd09d7f7802eb86fe323d0336a7e6891bb2
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Feb 21 10:14:38 CET 2020
  * inline namelength and simplify dirlength calculation

patch 2e2202648cb10bc98579117b85c11920a2405df7
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Feb 21 19:18:04 CET 2020
  * index: fix and extend documentation
  
  Due to the previous refactors it is now clear that the terminating null byte
  is not included in the iDescriptor, contrary to what I assumed before. I
  have highlighted the places where we treat the root directory specially and
  the conditions that allow the code to work. This is highly unintuitive and,
  as observed previously, violates the invariants of Name, requiring special
  support from Darcs.Util.Path.appendPath.

patch 8e3f0fa9b11560d01426db550053103ea061401f
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Feb 21 19:20:45 CET 2020
  * index: no longer rely on the special behavior of appendPath

Attachments

• index_-fix-import-layout.dpatch
• patch-preview.txt
• unnamed

> A series of refactors to clean up specific parts of the index code. I
> followed your (Ganesh's) example and did this as a series of small patches
> to convince me that all changes are correct and don't change behavior. This
> worked out pretty well: all tests succeeded the first time I ran them.

FWIW it also makes a big difference to my reviewing speed, as I can
think about each bit separately and I also often only have small chunks
of time to make incremental progress.

>   * index: fix import layout

Fine

>   * index: fix layout of getFileID'

Fine

>   * index: factor nameof and namelength to top level

Fine

>   * index: refactor second argument of nameof

Fine

>   * index: factor out substateof

Fine

>   * index: use compare in local 'subs' functions

Fine

>   * inline namelength and simplify dirlength calculation

Fine

>   * index: fix and extend documentation

Fine (I didn't actually check what you said is true, but I'm sure it is)

>   * index: no longer rely on the special behavior of appendPath

> -          then dirlength state -- must be 0 in this case
> +          Nothing -> dirlength state

Isn't that "must be 0" comment still valid?

> -           tree' = makeTree [ (n, fromJust $ treeitem s) | (n, s) <- inferiors, isJust $ treeitem s ]
> +           tree' = makeTree [ (n, fromJust $ treeitem s) | (Just n, s) <- inferiors, isJust $ treeitem s ]

What's going on here, i.e. how do we know it isn't a behaviour change?

>>   * index: no longer rely on the special behavior of appendPath
> 
>> -          then dirlength state -- must be 0 in this case
>> +          Nothing -> dirlength state
> 
> Isn't that "must be 0" comment still valid?

Hm, yes. Can't remember why I removed it. Should probably re-add.

>> -           tree' = makeTree [ (n, fromJust $ treeitem s) | (n, s) <- inferiors, isJust $ treeitem s ]
>> +           tree' = makeTree [ (n, fromJust $ treeitem s) | (Just n, s) <- inferiors, isJust $ treeitem s ]
> 
> What's going on here, i.e. how do we know it isn't a behaviour change?

We change the type of 'nameof' to return Maybe, where Nothing is
returned only for the root item. The 'inferiors' in the above snippet is
a bad name for 'subtrees' and is the result of calling 'subs $ start
substate' a few lines above that code. 'subs' in turn recursively
enumerates the "states" (think of a state as a cursor) pointing to the
subitems under the current one. It in turn calls 'nameof' which must
return Just because by definiton sub items are not the root item.

Clear now?

Attachments

• pEpkey.asc

Following up on review.

1 patch for repository http://darcs.net/screened:

patch 82093c6452e541f19ec80b0214fb19246bf12104
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Mar  2 11:39:57 CET 2020
  * index: re-add comment for the Nothing case in substateof

Attachments

• index_-re_add-comment-for-the-nothing-case-in-substateof.dpatch
• patch-preview.txt
• unnamed