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

Saturate exit codes to 255 for all builtins #7702

Closed
wants to merge 1 commit into from

Conversation

ethulhu
Copy link

@ethulhu ethulhu commented Feb 9, 2021

Description

After commit 6dd6a57, 3 remaining builtins were still affected by uint8_t overflow: exit, return, and functions --query.

This PR:

  • Moves the overflow check from builtin_set_query to builtin_run.
  • Removes a conflicting intuint8_t conversion in builtin_return.
  • Adds tests for the 3 remaining affected builtins.
  • Changes documentation for exit and return.
  • Does not change documentation for functions --query, because it does not state the exit code in its API.
  • Duplicates CHANGELOG lines for each affected builtin; this could equally be condensed into a single changelog entry?

This PR is a generic solution to overflowing exit codes, following discussion on PR #7698.

TODOs:

  • Changes to fish usage are reflected in user documentation/manpages.
  • Tests have been added for regressions fixed
  • User-visible changes noted in CHANGELOG.rst

CHANGELOG.rst Outdated Show resolved Hide resolved
doc_src/cmds/exit.rst Outdated Show resolved Hide resolved
CHANGELOG.rst Outdated Show resolved Hide resolved
After commit 6dd6a57, 3 remaining
builtins were affected by uint8_t overflow: `exit`, `return`, and
`functions --query`.

This commit:
- Moves the overflow check from `builtin_set_query` to `builtin_run`.
- Removes a conflicting int -> uint8_t conversion in `builtin_return`.
- Adds tests for the 3 remaining affected builtins.
- Simplifies the wording for the documentation for `set --query`.
- Does not change documentation for `functions --query`, because it does
  not state the exit code in its API.
- Updates the CHANGELOG to reflect the change to all builtins.
@krobelus krobelus added this to the fish 3.2.0 milestone Feb 13, 2021
@krobelus
Copy link
Member

Great job, thanks! - merged as 5a0aa78

@krobelus krobelus closed this Feb 13, 2021
xdelaruelle added a commit to xdelaruelle/modules that referenced this pull request Apr 5, 2021
Update fish install tests following change introduced on fish 3.2, where
the exit code now saturates to 255 for all builtins commands rather
overflowing. (see fish-shell/fish-shell#7702)
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants