Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changes to exit codes #7602

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Changes to exit codes #7602

wants to merge 2 commits into from

Conversation

albertony
Copy link
Contributor

@albertony albertony commented Jan 27, 2024

What is the purpose of this change?

According to rclone documentation, exit code 1 is for "syntax or usage" and 2 is for "not otherwise categorised" errors. However, in current implementation, exit code 2 will never be returned (*with one exception in bisync, see below), and in reality exit code 1 will be used for anything other than the "otherwise categorised" errors - exit code 1 is the default for unsuccessful returns.

There seem to be a more or less convention for exit codes in Linux/shell world, maybe some specifically for bash:

  • Values between 0 and 255
  • 0 is for success
  • 1 is for general errors
  • 2 is usage errors, invalid options or missing arguments.
  • Values above 125 are specifically used by the shell (bash), e.g. when program is terminated on response to a signal.

(https://www.gnu.org/software/bash/manual/html_node/Exit-Status.html)
(https://www.redhat.com/sysadmin/exit-codes-demystified)

This PR does the following changes to exit codes from rclone:

  • Explicitely checks for "syntax or usage" type errors from rclone and return exit code 2 in those cases. E.g. rclone copy a, rclone copy a b c or rclone copy --unknownflag a b.
  • Explicitely return exit code 2 when cobra command processor returns before rclone's command handling, where the resolving of exit codes occur. E.g. when running rclone unknowncommand, or simply rclone, this now returns exit code 2 instead of 1.
  • Keep using exit code 1 as default for unsuccessful returns, which needs no change in existing code, but an update to the documentation so that it correctly reflects this - now after the previous point by switching the description of exit code 1 and 2.

* bisync exception: There was actually one case of exit code 2, in bisync, where it explicitely called os.Exit without going through resolveExitCode. I added a commit to change that to exit code 7, more in-line with rest of rclone, and updated the description of exit codes in bisync documentation according to these changes affecting exit codes 1, 2 and 7.

Was the change discussed in an issue or in the forum before?

https://forum.rclone.org/t/minor-bug-return-code-from-rclone-check/44236

  • That thread started out by questioning why the check command returned exit code 1, documented as "syntax or usage error", when differences were found. With the changes in this PR it will still return exit code 1, but this is now documented as "error not otherwise categorised", and in case of "syntax or usage error" one will get a separate exit code 2. So this somewhat improves the situation, although the suggestion in that thread was to introduce a (or maybe several?) new exit codes for reporting result from a check, which could still be considered..

Checklist

  • I have read the contribution guidelines.
  • I have added tests for all changes in this PR if appropriate.
  • I have added documentation for the changes if appropriate.
  • All commit messages are in house style.
  • I'm done, this Pull Request is ready for review :-)

@albertony albertony self-assigned this Jan 27, 2024
@albertony albertony added the UX label Jan 27, 2024
@albertony albertony marked this pull request as draft January 28, 2024 11:10
@albertony albertony changed the title Change exit code from 1 to 2 for errors not otherwise categorised Changes to exit codes Jan 28, 2024
@albertony albertony force-pushed the exit-code-2 branch 3 times, most recently from 30bc97b to b79e6da Compare January 28, 2024 12:06
Copy link
Member

@ncw ncw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks very nice and unifies the bisync error codes nicely.

I think having 1 as uncategorized and 2 as syntax error is much more sensible as the "default" exit code tends to be 1 and hence uncategorized.

Comment on lines 566 to +570
if strings.HasPrefix(err.Error(), "unknown command") && selfupdateEnabled {
Root.PrintErrf("You could use '%s selfupdate' to get latest features.\n\n", Root.CommandPath())
}
log.Fatalf("Fatal error: %v", err)
log.Printf("Fatal error: %v", err)
os.Exit(exitcode.UsageError)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ncw What about this part, is it safe (enough) to assume that whatever ends up here should be exit code 2? I.e. whatever not ending up in resolveExitCode or in any other os.Exit (log.Fatalf) elsewhere. I think typically the rclone nonexisting case must be handled here, but not sure what else. The alternative would be to move the change into the "unknown command" if above, and perhaps extend it if needed, but what do you think?

@albertony albertony marked this pull request as ready for review January 31, 2024 12:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants