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

patch bc1aa9cfc3057bf9d43aad4e0b77b47e18600037
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Mar 14 23:00:59 CET 2021
  * better error handling when applying patches

  In the TreeMonad we now throw the IO errors that fit the situation instead
  of userError. This allows us to give getter warning messages when using
  runTolerantly. In addition, if we cannot run tolerantly, i.e. when we apply
  patches to pristine, we now notify the user which patch caused the problem
  and suggest to run 'darcs repair' on the repo that contains the broken
  patch. This is important because we no longer silently ignore broken move
  patches.

patch dbff6448bc931d93451dc4762b4dd4438ca88ab1
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Jun 20 09:01:38 CEST 2021
  * TreeMonad: factor out findItem

  This will later allow us to throw more precise exceptions.

patch 7aa45c9cabe5ecf31d8188de385f7dab3a4a801f
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Mar 16 08:22:51 CET 2021
  * TreeMonad: explicit import of D.U.Path, some simplifications

patch 137d9000b21038a273b1b72149d7095c8e27b929
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Mar 23 15:49:29 CET 2021
  * TreeMonad: more fine grained error checks

  In particular, 'readFile' and 'writeFile' now distinguish between
  non-existence and inappropriate type (directory), and 'copy' throws an
  exception when the source item does not exist instead of doing nothing.
  Since System.IO.Error does not export a function to construct an
  InappropriateType IOError, we no longer use the constructor functions, but
  instead import the IOErrorType from GHC.IO.Exception.

patch 84462d54e68f93a2cfff48aa72e97e58c2172ac2
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Jun 20 09:29:23 CEST 2021
  * TreeMonad: add two comments and some code re-formatting

patch 44bd6cb7c60754dea8033d1b3cc3d2c3fb88767f
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Mar 17 08:45:45 CET 2021
  * tests/broken_move.sh: adapt to better apply error messages

Attachments

• better-error-handling-when-applying-patches.dpatch
• patch-preview.txt

Additional bundle with a bugfix patch.

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

patch 1f8db857216ce6656d10f17c51da7d21c659fb9c
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Mar  5 12:02:23 CET 2023
  * bugfix in TreeMonad rename

  The two failure conditions did not match the exceptions thrown, they were
  swapped. This goes back to:

  patch bc1aa9cfc3057bf9d43aad4e0b77b47e18600037
  Author: Ben Franksen <ben.franksen@online.de>
  Date:   Sun Mar 14 23:00:59 CET 2021
    * better error handling when applying patches

Attachments

• bugfix-in-treemonad-rename.dpatch
• patch-preview.txt

>   * better error handling when applying patches

OK. I was wondering whether it's good advice to suggest
"darcs repair" without being sure of the cause of an error,
but if the repo is not broken it should be harmless.

>  * TreeMonad: factor out findItem

OK

>   * TreeMonad: explicit import of D.U.Path, some simplifications

OK

>   * TreeMonad: more fine grained error checks

OK, one minor comment:

> +    Just _ ->
> +      throw $
> +        flip ioeSetErrorString "is a directory" $
> +        mkIOError InappropriateType "readFile" Nothing (Just (displayPath p))

I had to poke around briefly to convince myself that we can't get `Stub` here
(because of the `expandTo` call in `findItem`). Maybe it'd be more defensive
to look for `Just (Subtree ...)` instead and have another fallthrough case?
But maybe that's overkill.

>   * TreeMonad: add two comments and some code re-formatting

> +-- TODO It is unclean to throw exceptions from potentially pure code.

I think this depends on whether we expect to catch them or not. If we expect
them to abort the program, or at the very least to be caught in IO without
a strong expectation of _which_ exception will be thrown (as per "A Semantics
for Imprecise Exceptions"), I don't think it is unclean.

>   * bugfix in TreeMonad rename

So much for my reviewing, as I didn't spot that :-)

>>    * better error handling when applying patches
> 
> OK. I was wondering whether it's good advice to suggest
> "darcs repair" without being sure of the cause of an error,
> but if the repo is not broken it should be harmless.

You have a point here, in that darcs repair potentially modifies patches (without 
changing their identity), and there may be corner cases where this can cause harm 
or at least confusion. There are many ways in which we could improve the repair 
command to guard against this, but this is a separate discussion. Perhaps I'll 
open an issue for that.

>> +    Just _ ->
>> +      throw $
>> +        flip ioeSetErrorString "is a directory" $
>> +        mkIOError InappropriateType "readFile" Nothing (Just (displayPath p))
> 
> I had to poke around briefly to convince myself that we can't get `Stub` here
> (because of the `expandTo` call in `findItem`). Maybe it'd be more defensive
> to look for `Just (Subtree ...)` instead and have another fallthrough case?
> But maybe that's overkill.

There are lots of places (in that module) where we make this assumption. So, not a 
bad idea per se, but if we do that we should revisit all functions in this module 
and that's out of scope here.

>>    * TreeMonad: add two comments and some code re-formatting
> 
>> +-- TODO It is unclean to throw exceptions from potentially pure code.
> 
> I think this depends on whether we expect to catch them or not. If we expect
> them to abort the program, or at the very least to be caught in IO without
> a strong expectation of _which_ exception will be thrown (as per "A Semantics
> for Imprecise Exceptions"), I don't think it is unclean.

I'd like to argue that if you have to make assumptions on how things are used, 
then there is already something "unclean" with your design. (Not that we adhere to 
such a strict standard everywhere else in Darcs, so go ahead and call me a 
hypocrite.) In any case, the point is moot insofar as with

patch c2737fa99e15af26c36602ec445ffeb4a10db25d
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Sep 19 14:00:41 CEST 2021
  * require MonadThrow from the base apply monads

(which is in screened) we have a completely clean solution.