darcs

Issue 2529 Darcs doesn't always return an error code when unable to find a patch

Title Darcs doesn't always return an error code when unable to find a patch
Priority Status resolved
Milestone 2.15.0 Resolved in
Superseder Nosy List gpiero
Assigned To
Topics

Created on 2017-03-25.17:00:02 by gpiero, last changed 2020-06-21.21:31:42 by bfrk.

Messages
msg19396 (view) Author: gpiero Date: 2017-03-25.17:00:01
When darcs expect an option to match a number of patches, it doesn't 
return any error if cannot find any matching patches. This is in 
contrast for when the option is expected to match a single patch.
Some examples should clarify better than I could.

$ darcs initialize R
$ cd R
$ darcs log --patches xxx ; echo rc=$?
rc=0

'--patches' is expected to match many patches and does not return an 
error when the list of results is empty. On the other hand, 
'--from-patch' is expected to match a single patch (as the start of the 
range) and it DOES return an error if it does not succeed:

$ darcs log --from-patch xxx ; echo rc=$?
darcs-screened: Couldn't find patch matching "patch-name xxx"
CallStack (from HasCallStack):
  error, called at src/Darcs/Patch/Match.hs:654:43 in 
  darcs-2.13.0-8gUFVFHnEqLKewBqd2XTAf:Darcs.Patch.Match
rc=1

The same applies to all commands. For example:

$ darcs rebase suspend --tags xxx ; echo rc=$?
No patches selected!
Rebase finished!
rc=0

$ darcs clone --tag xxx . ../S ; echo rc=$?
WARNING: creating a nested repository.
Copying patches, to get lazy repository hit ctrl-C...
Going to specified version...
darcs-screened: Couldn't find a tag matching "tag-name xxx"
CallStack (from HasCallStack):
  error, called at src/Darcs/Patch/Match.hs:665:43 in 
  darcs-2.13.0-8gUFVFHnEqLKewBqd2XTAf:Darcs.Patch.Match
rc=1

(please also note that the last case is a bit misleading, as the clone 
succeeds, but it behaves as if the '--tag' option wasn't passed).

Thanks,
Gian Piero.

$ darcs --version
2.13.0 (unknown)

$ darcs --exact-version
darcs compiled on Mar 24 2017, at 22:28:36

Context:

[show repo: fixed formatting (boringfile Pref overflowed alignment)
Ben Franksen <benjamin.franksen@helmholtz-berlin.de>**20170323124112
 Ignore-this: 4f800e377ce5b6aecd3a15b8c546ff74
]
msg19401 (view) Author: bfrk Date: 2017-03-26.08:23:36
Perhaps the intention here was that the pural indicates "zero or more",
so zero is an expected result?
msg19415 (view) Author: gpiero Date: 2017-04-01.16:03:27
* [Sun, Mar 26, 2017 at 08:23:37AM +0000] Ben Franksen:
>Perhaps the intention here was that the pural indicates "zero or more",
>so zero is an expected result?

Probably yes. The fact is that it is not consistent. For example:

$ darcs diff --from-patch xxx --to-patch yyy

raises an error, while

$ darcs diff --index x-y

doesn't, despite the fact they are conceptually the same case.[0]

Also, given that '-h' is an alias to '--match' instead of '--matches' 
(more about this on another mail),

$ darcs log --matches 'hash xxx'

does not croak, while

$ darcs log -h xxx

does.

In general, I think darcs should always return an error code when cannot 
found a patch he was asked for, both for consistency and for easing 
scripting.[1]

Better, darcs should return a specific, different from 1, error code for 
this (let's say 20), so that for example

$ darcs diff --patch x file

returns:

20 if the patch cannot be found
1  if there are differences in the two versions of the file (similarly 
to the *nix diff command)
0  if the two versions of the file are the same

Ciao,
Gian Piero.

[0] Also, I'm surprised --index doesn't croak about not being fed 
integers.

[1] Just for example, as naming convention has often been referred as a 
sort of branching, `darcs log -p '<BRANCH>'` could be a quick check for 
testing we have the right branch.
msg19417 (view) Author: bfrk Date: 2017-04-01.20:52:16
Am 01.04.2017 um 18:03 schrieb Gian Piero Carrubba:
> Gian Piero Carrubba <gpiero@rm-rf.it> added the comment:
> * [Sun, Mar 26, 2017 at 08:23:37AM +0000] Ben Franksen:
>> Perhaps the intention here was that the pural indicates "zero or more",
>> so zero is an expected result?
> 
> Probably yes. The fact is that it is not consistent. [...]

I agree with all your points. Indeed the way the matching options are
handled is far from optimal wrt consistency. If you are interested in
digging into the sources and cleaning things up, you are welcome (as far
as I am concerned).

The idea of using a dedicated return code for 'no matching patch for
this operation' is nice. I have often complained about the ubiquitous
use of 'fail' everywhere and added a function 'die' to
Darcs.Util.Exception to help fight 'fail' (its is used only in a few
places that I cleaned up some time ago). It would be nice to have
another function there e.g. 'nomatch' that exits with appropriate code.

Perhaps even better would be to define a few specific (non-fatal) darcs
exceptions for early termination of a command. These could be handled
when using darcs as a library. A top-level handler (in darcs/darcs.hs)
could then convert these into exit codes for scripting.
msg22105 (view) Author: bfrk Date: 2020-06-21.21:31:39
This has been essentially fixed.

Match failures are now reported using a dedicated exception type and 
not by calling error. Parse errors for index options are now handled 
early and fail with an appropriate message.

The singular options (--hash, --patch, --match) and range limits (--
from-xxx, -to-xxx) should fail if nothing matches, while the plural 
variants (--patches, --matches) should not.

Please re-open if there are any remaining inconsistencies.
History
Date User Action Args
2017-03-25 17:00:02gpierocreate
2017-03-26 08:23:37bfrksetmessages: + msg19401
2017-04-01 16:03:28gpierosetmessages: + msg19415
2017-04-01 20:52:17bfrksetmessages: + msg19417
2020-06-21 21:31:42bfrksetstatus: unknown -> resolved
messages: + msg22105
milestone: 2.15.0