The main patches in this bundle are

  * replace errorDoc, assertDoc, bug, and impossible with direct error calls
  * add a top-level error handler

I will not screen this immediately, pending comments. However, some of my
preparation patches for the V3 integration will depend on these patches, so
please take look.

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

patch 762aa5806a5e6eed15a09baa7d8ed26b3280939e
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Dec 14 14:14:58 CET 2018
  * fix several uses of error where it should be fail
  
  These situations are normal failure conditions and not bugs in Darcs.

patch 80176b9065234cd49d8f2303d8a890059963ff9a
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Dec 14 16:02:17 CET 2018
  * replace debugFail with plain fail
  
  The extra information obtained is usefull to diagnose the error and should
  be available to the user without having to supply --debug.

patch 1bcf44c5b15cdbbfe86727a41fbb0fe40a09ee4c
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Dec 14 16:34:21 CET 2018
  * replace a few calls to 'bug' with 'fail'
  
  These aren't bugs in Darcs at all, instead they are normal failures.

patch c45cdff91edbbc5635e92c436da5c543fa8777f9
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Dec 14 16:36:46 CET 2018
  * replace errorDoc, assertDoc, bug, and impossible with direct error calls
  
  All of these functions indicate the same thing: if they are evaluated then
  this is a bug in Darcs. In case such an error happens we want to know the
  file and line number. If we use any of these functions, then the location we
  get from ghc is only an unhelpful src/Darcs/Prelude.hs or
  src/Darcs/Util/Printer.hs. That is, unless the callstack actually contains
  useful information, which apparently requires profiling to be enabled. So
  this is useless to us in practice because who uses a special
  profiling-enabled Darcs in their daily work?

patch 7448ac2618ae158dbe576e45f487d24e9c71170c
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Dec 14 16:36:46 CET 2018
  * add new Exception types CommuteCommonPatches and PatchNotAvailable
  
  We now throw these special exceptions instead of calling error. This is
  because neither is (necessarily) a bug in Darcs. Instead these exceptions
  can be thrown under normal conditions and indeed there are tests that
  provoke these failures.

patch 8ffa32f7fe19f3901c0674276fa4d6c907815376
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Dec 14 16:36:46 CET 2018
  * add a top-level error handler
  
  This exception handler merely adds information about where to report the
  bug, then prints the exception to stderr and exits with ExitFailure 4. This
  allows externals tools to distinguish whether darcs exited due to a bug.

patch ab76f348c29fce108cdf56c27b1f1566dd1ca6d0
Author: Ben Franksen <benjamin.franksen@helmholtz-berlin.de>
Date:   Tue Dec 18 18:37:46 CET 2018
  * D.P.Match: pattern match and error instead of fromJust

patch a17a3a3b10d112133239580afba4b208a5823ce1
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Jan 22 18:27:31 CET 2019
  * replace bug with error in two new places

Attachments

• fix-several-uses-of-error-where-it-should-be-fail.dpatch
• patch-preview.txt
• unnamed

The upshot of all this is: when we call 'fail' in the IO Monad, this
will be treated as "normal" failure, whereas a direct call to 'error' is
treated as a bug. If we want to signal a "normal" failure from pure code
we define a special Exception type and raise that.

On 23/01/2019 17:26, Ben Franksen wrote:
> 
> Ben Franksen <ben.franksen@online.de> added the comment:
> 
> The upshot of all this is: when we call 'fail' in the IO Monad, this
> will be treated as "normal" failure, whereas a direct call to 'error' is
> treated as a bug. If we want to signal a "normal" failure from pure code
> we define a special Exception type and raise that.

This convention would mean that a pattern-match failure in the IO monad
will be treated as a "normal" failure. Does that matter?

FWIW I found and fixed (i.e. replaced with error) some cases of
potential pattern-match failure in polymorphic monadic code when dealing
with MonadFail, but I don't think we'd ever actually hit any of them in
practice.

Cheers,

Ganesh

Yes, pattern match failures in IO code will not be treated as a bug. I
guess whenever we find a problem caused by this we must manually insert
an error call. I don't think it is a big issue.

As far as I know, the goal of MonadFail is to eventually remove the fail
method from class Monad. I guess what we want to do then is replace all
explicit uses of fail with some function that throws an appropriate
exception that we define ourselves. We can then handle that (and only
that) exception at the top level. This will be much cleaner than what we
do now; I must say I do not fully understand how the distinction between
error calls and IO.fail works currently in darcs but it does.

> Yes, pattern match failures in IO code will not be treated as a bug. I
> guess whenever we find a problem caused by this we must manually insert
> an error call. I don't think it is a big issue.

To elaborate, the issue is that if we have a bug caused by a pattern
match failure in the IO monad the error message that darcs displays will
not say so. When/if this actually happens, we must of course solve the
underlying bug. When MonadFail is fully implemented and can rely on fail
no longer being a Monad method and we refactor our uses of fail to throw
an appropriate exception this issue will go away.

> When MonadFail is fully implemented and can rely on fail
> no longer being a Monad method and we refactor our uses of
> fail to throw an appropriate exception this issue will go away.

I think we're already there for code that's polymorphic in the 
monad, but IO implements MonadFail so fail will always be available
invisibly in IO-specific code.

>> When MonadFail is fully implemented and can rely on fail
>> no longer being a Monad method and we refactor our uses of
>> fail to throw an appropriate exception this issue will go away.
> 
> I think we're already there for code that's polymorphic in the 
> monad, but IO implements MonadFail so fail will always be available
> invisibly in IO-specific code.

Ah, right. So what do you make of this bundle. You think I can screen this?

> Ah, right. So what do you make of this bundle. You think
> I can screen this?

Didn't get time to read the code yet, but from skimming the
patch descriptions it sounds fine to screen it.

Okay then, the fine details can be worked out on review.

>  * fix several uses of error where it should be fail
  
>  These situations are normal failure conditions and not bugs in Darcs.


> hunk ./src/Darcs/Repository/PatchIndex.hs 730
> -      unless (isInOrder ascTs pids) (error $ "In order test failed! 
filename: " ++ show fn)
> -      forM_ spans $ \(FidSpan fid _ _) -> unless (M.member fid fpspans) 
(error $ "Valid file id test failed! fid: " ++ show fid)
> +      unless (isInOrder ascTs pids) (fail $ "In order test failed! 
filename: " ++ show fn)
> +      forM_ spans $ \(FidSpan fid _ _) -> unless (M.member fid fpspans) 
(fail $ "Valid file id test failed! fid: " ++ show fid)
hunk ./src/Darcs/Repository/PatchIndex.hs 742
> -      unless (isInOrder ascTs pids) (error $ "In order test failed! 
fileid: " ++ show fid)
> -      forM_ spans $ \(FpSpan fn _ _) -> unless (M.member fn fidspans) 
(error $ "Valid file name test failed! file name: " ++ show fn)
> +      unless (isInOrder ascTs pids) (fail $ "In order test failed! 
fileid: " ++ show fid)
> +      forM_ spans $ \(FpSpan fn _ _) -> unless (M.member fn fidspans) 
(fail $ "Valid file name test failed! file name: " ++ show fn)
> hunk ./src/Darcs/Repository/PatchIndex.hs 747
> -      unless (and $ zipWith f spans (tail spans)) (error $ "Adjcency > 
test failed! fid: " ++ show fid)
> +      unless (and $ zipWith f spans (tail spans)) (fail $ "Adjcency 
test failed! fid: " ++ show fid)

How is the user supposed to understand/recover from these errors? Do
they indicate that the patch index is corrupt and should be recreated,
for example?

>   * replace debugFail with plain fail
  
>   The extra information obtained is usefull to diagnose the error
>   and should be available to the user without having to supply
>   --debug.

Given that the definition of debugFail was

    debugFail m = debugMessage m >> fail m

I think it already was available to the user directly. But I
can't see the point in (possibly) putting it in the debug output
too, so I'm fine with the change anyway.

>  * replace a few calls to 'bug' with 'fail'

>  These aren't bugs in Darcs at all, instead they are normal failures.

> hunk ./src/Darcs/UI/Commands/MarkConflicts.hs 218
> -           bug ("Can't undo pending changes!" ++ show e)
> +           fail ("Can't undo pending changes!" ++ show e)
> hunk ./src/Darcs/UI/Commands/MarkConflicts.hs 222
> -             bug ("Problem marking conflicts in mark-conflicts!" ++ 
show e)
> +             fail ("Problem marking conflicts in mark-conflicts!" ++ 
show e)
> hunk ./src/Darcs/UI/Commands/Replace.hs 172
> -            bug $ "Can't do replace on working!\n" ++ show e
> +            fail $ "Can't do replace on working!\n" ++ show e

As with the patch index changes, I'm not sure how the user is supposed
to react to these.

>  * replace errorDoc, assertDoc, bug, and impossible with direct error calls

Fine

>  That is, unless the callstack actually contains
>  useful information, which apparently requires profiling to be enabled. So
>  this is useless to us in practice because who uses a special
>  profiling-enabled Darcs in their daily work?

For what it's worth, putting HasCallStack constraints on functions
nowadays seems to work fairly reliably in all kinds of builds. But that
would require us to go manually through our code to find all the places
it would helpfully improve the error reports, so still isn't that useful.

>  * add a top-level error handler

OK

>   * D.P.Match: pattern match and error instead of fromJust

OK

>   * replace bug with error in two new places

OK

>  * add new Exception types CommuteCommonPatches and PatchNotAvailable
  
>  We now throw these special exceptions instead of calling error. This is
>  because neither is (necessarily) a bug in Darcs. Instead these 
exceptions
>  can be thrown under normal conditions and indeed there are tests that
>  provoke these failures.

Same reservation as the other two cases - I don't know
what the user is supposed to do when they hit these.

>> hunk ./src/Darcs/Repository/PatchIndex.hs 730
>> -      unless (isInOrder ascTs pids) (error $ "In order test failed! 
> filename: " ++ show fn)
>> [...]>
> How is the user supposed to understand/recover from these errors? Do
> they indicate that the patch index is corrupt and should be recreated,
> for example?

These are in function piTest which is part of the 'show patch-index'
command, which is why I thought they should be treated as normal
failures. I guess 'show patch-index' was added for developers mostly to
see if the patch index works or to investigate things if it doesn't.

>>  * replace a few calls to 'bug' with 'fail'
> 
>>  These aren't bugs in Darcs at all, instead they are normal failures.
> 
>> hunk ./src/Darcs/UI/Commands/MarkConflicts.hs 218
>> -           bug ("Can't undo pending changes!" ++ show e)
>> +           fail ("Can't undo pending changes!" ++ show e)
>> hunk ./src/Darcs/UI/Commands/MarkConflicts.hs 222
>> -             bug ("Problem marking conflicts in mark-conflicts!" ++ 
> show e)
>> +             fail ("Problem marking conflicts in mark-conflicts!" ++ 
> show e)
>> hunk ./src/Darcs/UI/Commands/Replace.hs 172
>> -            bug $ "Can't do replace on working!\n" ++ show e
>> +            fail $ "Can't do replace on working!\n" ++ show e
> 
> As with the patch index changes, I'm not sure how the user is supposed
> to react to these.

So, if applyToWorking fails this is probably due to e.g. some affected
file not being writable or the disk being full or something like that.
Which would definitely not be our fault. I think the best hint to the
user in such cases is to retain the original (OS level) error message
and this is what we do here. I think there is not much more we can do
for IO exceptions. It may be better to catch just these and not all
exceptions, though, and treat other exceptions as bugs.

>>  * add new Exception types CommuteCommonPatches and PatchNotAvailable
>   
>>  We now throw these special exceptions instead of calling error. This is
>>  because neither is (necessarily) a bug in Darcs. Instead these 
> exceptions
>>  can be thrown under normal conditions and indeed there are tests that
>>  provoke these failures.
> 
> Same reservation as the other two cases - I don't know
> what the user is supposed to do when they hit these.

I think I made the CommuteCommonPatches change when i still thought
merge-by-value could be made to work. This should really be an error/bug
in darcs. We should rollback this change.

The PatchNotAvailable change is sound because this exception is usually
triggered by lazy repos, so the user should try and make their repo
complete.

Following up on patch review.

1 patch for repository /home/ben/src/darcs/sent:

patch d55f77f479dfc3af6ec5092eaeb0885a608d7762
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Jun 12 17:39:25 CEST 2019
  * treat 'failed to commute common patches' as a bug
  
  This partially rolls back
    patch 7448ac2618ae158dbe576e45f487d24e9c71170c
    * add new Exception types CommuteCommonPatches and PatchNotAvailable

Attachments

• patch-preview.txt
• treat-_failed-to-commute-common-patches_-as-a-bug.dpatch
• unnamed

OK, thanks for the comments and followup. Now that we have a
clear strategy for dealing with errors we can always tweak
individual cases later.

> I think there is not much more we can do
> for IO exceptions. It may be better to catch just these and not all
> exceptions, though, and treat other exceptions as bugs.

I'll open a separate issue for this as it affects many more commands.