These are all related to the patch index. Most are pure layout changes and
minor cleanups, but the first is a real bug fix that acually caused a
failure in a situation I can't remember the details of.

These are mostly pure layout changes.

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

patch 31b2f4d9a34d9becc78fdcb93ddda75064c163e2
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Oct 14 10:29:07 CEST 2018
  * fix in patch index: use removeFile, not removeDirectoryRecursive to remove noPatchIndex

patch 27a39b14b8f28e9689087e7e83f5b91b8121c305
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Oct 14 10:24:08 CEST 2018
  * clean up code layout in Darcs.Patch.Index.*

patch 6b0af3027412cd37fb823657d7e5bdfa676fdbf4
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Oct 14 10:38:56 CEST 2018
  * add progress reporting to patch index

patch 18179d143a4af72ba7b920197058a214b6112ccf
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Oct 14 10:56:00 CEST 2018
  * patch index: clean up import and export lists

patch 1284dae7e358fc37ac2a3b70b975c57af2ba8914
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Oct 14 11:01:11 CEST 2018
  * patch index: replace fn2fp with displayPath

patch bcf0e88d4fb3a695fab85178120ecfe71ff4a82c
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Oct 14 11:06:46 CEST 2018
  * patch index: beautify layout of data definitions

patch e27167d087fd0260ab0f3d4b7e01179854318ace
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Oct 15 09:00:58 CEST 2018
  * patch index: re-format getRelevantSubsequence

Attachments

• fix-in-patch-index_-use-removefile_-not-removedirectoryrecursive-to-remove-nopatchindex.dpatch
• patch-preview.txt
• unnamed

(This bundle is blocked by patch1745)

>  * fix in patch index: use removeFile, not 
removeDirectoryRecursive to remove noPatchIndex

> -   removeDirectoryRecursive (repodir </> darcsdir </> 
noPatchIndex) `catch` \(_ :: IOError) -> return ()
> +   removeFile (repodir </> darcsdir </> noPatchIndex) `catch` \(_ 
:: IOError) -> return ()

Fine. Might be nice to make the catch more limited than any IOError, 
or to check for problems up-front.

>   * clean up code layout in Darcs.Patch.Index.*

Fine.

>  * add progress reporting to patch index

Fine

>   * patch index: clean up import and export lists

Fine

>   * patch index: replace fn2fp with displayPath

Fine

>       * patch index: beautify layout of data definitions

Fine

>   * patch index: re-format getRelevantSubsequence

Fine

>>  * fix in patch index: use removeFile, not 
> removeDirectoryRecursive to remove noPatchIndex
> 
>> -   removeDirectoryRecursive (repodir </> darcsdir </> 
> noPatchIndex) `catch` \(_ :: IOError) -> return ()
>> +   removeFile (repodir </> darcsdir </> noPatchIndex) `catch` \(_ 
> :: IOError) -> return ()
> 
> Fine. Might be nice to make the catch more limited than any IOError, 
> or to check for problems up-front.

Checking for problematic conditions up front is prone to race
conditions. This is why it is generally recommended to just run the
action and catch errors. We could print a warning to stderr, though,
instead of completely ignoring the error.

Do you have a suggestion to what kind of IO errors to limit the catch?

On 20/11/2018 03:48, Ben Franksen wrote:
> 
> Ben Franksen <ben.franksen@online.de> added the comment:
> 
>>>  * fix in patch index: use removeFile, not 
>> removeDirectoryRecursive to remove noPatchIndex
>>
>>> -   removeDirectoryRecursive (repodir </> darcsdir </> 
>> noPatchIndex) `catch` \(_ :: IOError) -> return ()
>>> +   removeFile (repodir </> darcsdir </> noPatchIndex) `catch` \(_ 
>> :: IOError) -> return ()
>>
>> Fine. Might be nice to make the catch more limited than any IOError, 
>> or to check for problems up-front.
> 
> Checking for problematic conditions up front is prone to race
> conditions. This is why it is generally recommended to just run the
> action and catch errors. We could print a warning to stderr, though,
> instead of completely ignoring the error.
> 
> Do you have a suggestion to what kind of IO errors to limit the catch?

Yes, good point re races. Given my subsequent discovery about
isPermissionError on Windows I can't suggest anything with confidence.
Printing to stderr sounds good.

A lot of the error handling in Darcs.Repository.PatchIndex is wrong. For
instance, canUsePatchIndex will crash with an error call if the patch
index exists and is disabled. OTOH, if _darcs/no_patch_index is not
writable then exactly that is the situation created by ignoring IO
errors. deletePatchIndex also calls "error" when there is an IO
exception instead of "fail" which is our de-facto convention.

It is usually not a good idea to catch IO exceptions without re-raising
them. Even if a failure may not be fatal immediately, as in the case of
_darcs/no_patch_index, this tends to cause problems later when the state
on disk doesn't meet our expectations. There are a few special
situations where this is okay, mostly composite actions like "remove
file unless it isn't there". We should strive to factor them to
Darcs.Util.File or a related module.

However, the PatchIndex has more fundamental problems. For instance, it
doesn't store all the touch information: if a tree item is touched, then
we regard all its parent directories as touched, too, but this
information is not stored in the PatchIndex, so we must re-construct it
afterwards. This is inefficient, it takes time quadratic in number of
children of a directory. Also, the representation it uses is too
complicated IMO. It would be simpler and more efficient to track tree
item identities directly i.e. assign them Int keys and track their
"current path" together with the informationa about which patch touches
which tree item.