Extracted these largely unproblematic patches from my bundle patch1887.

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

patch 4e4f497e3f651e6e887d56fb4c6a69015d5400f6
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Aug 17 13:17:05 CEST 2019
  * fix doc comment for filterDirContents
  
  We changed the behavior slightly a while ago.

patch f674822894c2e40548ef1e5d8eb294b1f91f338a
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Aug 20 11:11:02 CEST 2019
  * tests/clone.sh: fix removing of repo dirs

patch 9661ce4c1e5e7ee5bb0d7cf6f94350d0c6769edc
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Aug 20 13:39:03 CEST 2019
  * fix some haddocks in Darcs.Repository.Hashed

patch 23653c3fe926e7af11ddd5d0a0fe559e518fa840
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Aug 20 16:53:38 CEST 2019
  * cleanup messages and filepath/url concatentation in D.R.Cache

patch 281e71088845cb4062b2c96160dc7ade8a36b7ad
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Aug 22 11:57:33 CEST 2019
  * tests/conflict-fight-failure.sh: increase overshoot margin to 50%

patch 4d15f0339d3bd4084df056490ed9fff0a15423ce
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Aug 22 17:34:18 CEST 2019
  * fix build with -fcurl

patch 4db770608457af823a5ac0d2c40495d1d5efa240
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Aug 22 21:54:49 CEST 2019
  * fallback to simplePrinters if fancyPrinters throws exception

patch 9cc6114629b91285f64019d00327ac00dcaeb4a4
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Aug 23 10:36:16 CEST 2019
  * do atexit actions in the opposite order of registration

patch 2ec817fedf5489fabb3cdca19a70e9434485150f
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Aug 23 11:27:19 CEST 2019
  * make working with temporary directories more robust
  
  Cleaning up after withTempDir or withDelayedDir fails if an asynchronous
  thread or an interleaved IO action continues to create files in this
  directory. We rather want those operations to fail, so we first rename the
  directory and then delete that.

patch 9ee34ceb729f6328649e91e2422b35c4fb717d9c
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Aug 23 11:36:01 CEST 2019
  * make sure we cancel pending download actions at exit

patch b3d118c61ce9a3bc8c806dc5bf6856306c417ff7
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Aug 23 20:31:31 CEST 2019
  * un-comment and export parseHeadInventory
  
  Commented-out code quickly bit-rots, which was actually the case here. So we
  rather add an unused export.

patch 8ce298d8e0e65e61f2c8d83c7b60dd497a81266d
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Aug 24 11:04:47 CEST 2019
  * tests: remove access to http://hub.darcs.net/kowey/tabular
  
  This slowed down the networks tests considerably. Instead I added a tar ball
  of the current state of this repo and the tests are now run against a
  locally running http server.

patch 0af5ee37177b7de9eda52e6bc1b35533efc10d8d
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Aug 24 11:45:09 CEST 2019
  * tests/network: cleanup clone.sh and lazy-clone.sh

patch 7dcada891e90c1367736c84b44d77a270ee3c8bb
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Aug 24 11:46:07 CEST 2019
  * extend tests/network/lazy-clone.sh

Attachments

• fix-doc-comment-for-filterdircontents.dpatch
• patch-preview.txt
• unnamed

>   * fix doc comment for filterDirContents
>   * fix some haddocks in Darcs.Repository.Hashed

Fine

>   * cleanup messages and filepath/url concatentation in D.R.Cache

Fine. Changing code that uses /s is always a bit dangerous for
Windows but in this file </> is imported from System.FilePath.Posix so
there shouldn't be any behaviour changes as a result.

That said I'm not sure it *should* be using System.FilePath.Posix rather
than System.FilePath as it's just manipulating on-disk filepaths, but
it'll work anyway so there's not much point in changing it.

>   * tests/conflict-fight-failure.sh: increase overshoot margin to 50%

OK (performance tests are always nasty to maintain :-/)

>   * fix build with -fcurl

Fine. Now we have the new HTTP downloading could we just drop -fcurl? I
can't remember if we discussed that before.

>   * fallback to simplePrinters if fancyPrinters throws exception

I really don't like the catchall. Did you have some specific error
condition in mind?

>    * do atexit actions in the opposite order of registration

I don't think this is correct. atExit puts the new action at the head of
the list. So if you call atexit a; atexit b; atexit c, then the list
will be c;b;a.

>   * make working with temporary directories more robust
>   
>   Cleaning up after withTempDir or withDelayedDir fails if an asynchronous
>   thread or an interleaved IO action continues to create files in this
>   directory. We rather want those operations to fail, so we first rename the
>   directory and then delete that.

Sounds good - as long as the windows tests don't throw up any problems!

>   * make sure we cancel pending download actions at exit
>   * un-comment and export parseHeadInventory
>   * tests: remove access to http://hub.darcs.net/kowey/tabular
>   * tests/network: cleanup clone.sh and lazy-clone.sh
>   * extend tests/network/lazy-clone.sh

All fine.

Following up on review.

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

patch 1d4771365f518765b91d7029918a6d86a375a884
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Sep 26 14:19:48 CEST 2019
  * restore correct order of atexit actions
  
  This is a rollback of
  
  patch 9cc6114629b91285f64019d00327ac00dcaeb4a4
  Author: Ben Franksen <ben.franksen@online.de>
  Date:   Fri Aug 23 10:36:16 CEST 2019
    * do atexit actions in the opposite order of registration
  
  which was complete bogus because it did the opposite of what it says. We use
  the list as a stack here, so it already has LIFO behavior.

Attachments

• patch-preview.txt
• restore-correct-order-of-atexit-actions.dpatch
• unnamed

>>   * fix build with -fcurl
> 
> Fine. Now we have the new HTTP downloading could we just drop -fcurl? I
> can't remember if we discussed that before.

This came up because I had a situation where I wanted to compare with a
darcs built against curl. I cannot remember the details, but I had a
serious performance regression when interacting with remote
repositories, related to the asynchronous download changes. As long as
it doesn't hurt us seriously I would like us to keep it.

>>   * fallback to simplePrinters if fancyPrinters throws exception
> 
> I really don't like the catchall. Did you have some specific error
> condition in mind?

It failed for me in a specific situation, but unfortunately I cannot
remember precisely which exception I got. It was related to the problem
with the race between cleanup of temporary directories and
asynchronously downloading files. I believe the problem was that this
could lead to exceptions thrown from an exception handler.

My justification for using catchall here was that we don't loose
anything significant, just coloring. I don't think the exeption in
question was an IO error because I would have used catchIOError in that
case.

>>    * do atexit actions in the opposite order of registration
> 
> I don't think this is correct. atExit puts the new action at the head of
> the list. So if you call atexit a; atexit b; atexit c, then the list
> will be c;b;a.

Oh my, thanks for catching this one. This change was made in haste when
I tried to fix the race conditions mentioned below. For whatever reasons
it improved the situation, so I thought it must be correct. But I wasn't
satisfied with that and wanted a more thorough fix. Afterwards I just
thought "why not?". Will rollback.

>>   * make working with temporary directories more robust
>>   
>>   Cleaning up after withTempDir or withDelayedDir fails if an asynchronous
>>   thread or an interleaved IO action continues to create files in this
>>   directory. We rather want those operations to fail, so we first rename the
>>   directory and then delete that.
> 
> Sounds good - as long as the windows tests don't throw up any problems!

Ahem, yes, we should check that. I keep forgetting that there are
problems with renaming directories in Windows.

>> Fine. Now we have the new HTTP downloading could we just drop -fcurl? I
>> can't remember if we discussed that before.
> 
> This came up because I had a situation where I wanted to compare with a
> darcs built against curl. I cannot remember the details, but I had a
> serious performance regression when interacting with remote
> repositories, related to the asynchronous download changes. As long as
> it doesn't hurt us seriously I would like us to keep it.

OK. It's just more complexity, including configuration and some C files,
but it's not really costing anything to maintain right now. I don't plan
to pay any attention to keeping it working on Windows though.

>>>   * fallback to simplePrinters if fancyPrinters throws exception
>>
> My justification for using catchall here was that we don't loose
> anything significant, just coloring. I don't think the exeption in
> question was an IO error because I would have used catchIOError in that
> case.

I see these options, in order of preference for me (I could live with
any of them):

 - remove the catchall and wait for it to fail again, and analyse the
   actual causes
 - add a debugPrint when the catchall gets hit, so that someone trying
   to understand why they're not getting colour can find out
 - leave the catchall as it is, perhaps with a comment to explain it


>> Sounds good - as long as the windows tests don't throw up any problems!
> 
> Ahem, yes, we should check that. I keep forgetting that there are
> problems with renaming directories in Windows.

It was fine when I ran the tests, which I always do before actually
pushing to reviewed.

A gap we do have with Windows testing is that you could perfectly
legitimately run the tests on Linux and then push to reviewed and I'd
only find it later. But the same is true for any tests that are being
skipped in my configuration, I think we just need to accept those risks.

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

patch 6fe28e795b391a39174d229be96c0f4fdb478be2
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Sep 27 00:14:55 CEST 2019
  * remove catchall handler for fancyPrinters
  
  This rolls back
  
  patch 4db770608457af823a5ac0d2c40495d1d5efa240
  Author: Ben Franksen <ben.franksen@online.de>
  Date:   Thu Aug 22 21:54:49 CEST 2019
    * fallback to simplePrinters if fancyPrinters throws exception
  
  Using catchall is bad because we will never know what exactly went wrong and
  why. If the error/exception happens again we can take a closer look and
  catch just this exception.

Attachments

• patch-preview.txt
• remove-catchall-handler-for-fancyprinters.dpatch
• unnamed

All good now