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

fix: pipestatus quoting on Zsh/Bash #3088

Merged
merged 4 commits into from
Oct 5, 2021

Conversation

chipbuster
Copy link
Contributor

@chipbuster chipbuster commented Sep 22, 2021

Description

Modifies pipestatus parsing for the starship binary and changes the initscripts for bash/zsh to quote pipestatus. This allows starship to start without errors in the case where pipestatus is empty (usually due to interactions with other shell frameworks).

Motivation and Context

The previous implementation of pipestatus assumed that the arguments to pipestatus would be word-split (bash, zsh) or repeated by the shell (fish). Unfortunately, this causes a shell parse error if pipestatus is not set on first invocation (which can apparently happen if other shell frameworks are present).

There are two ways to fix this:

  1. Modify the init scripts to not use --pipestatus at all when pipestatus is detected to be empty.
  2. Quote the arguments to --pipestatus, which allows clap to detect an empty argument, and then change the parsing code in starship to split the arguments ourselves.

I have implemented the second here, since the first requires significantly complicating the init scripts for bash/zsh.

Screenshots (if appropriate):

How Has This Been Tested?

I'll mark this as ready once I can confirm that this fixes the linked bug in the provided docker.

Confirmed that this does solve the issue mentioned above. There is a separate issue which appears, where on shell startup, the exit status of the previous command is not defined, so starship prints a warning about not being able to parse the exit status. However, it's not as clear to me how to fix that issue, since we do want the warning to appear when a status cannot be parsed.

  • I have tested using MacOS
  • I have tested using Linux
  • I have tested using Windows

Checklist:

  • I have updated the documentation accordingly.
  • I have updated the tests accordingly.

Changes the parsing for pipestatus to allow for multiple arguments, a
single argument of space-separated values, or any mix of the two. All
inputs are flattened into a single array where no elements have spaces
in them.
Modify init scripts for bash/zsh to quote pipestatus in case it is empty
@chipbuster chipbuster marked this pull request as ready for review September 24, 2021 05:57
@pbar1
Copy link

pbar1 commented Sep 30, 2021

I built this commit on macOS and while it seems to resolve the original issue, I started instead seeing:

(starship::modules::status): Error parsing exit_code string to int

Edit: I can't read, you called this out in the original post 😅

@chipbuster
Copy link
Contributor Author

Dammit.

I was afraid of something like that, though I'm surprised it's an error (I noted a similar issue in the PR, but for me it would only come out as a warning).

Is it possible for you to share your configs so I can try to repro?

@pbar1
Copy link

pbar1 commented Oct 1, 2021

I must not have copied the whole string - it was definitely just a warning, not an error! 😄

Apologies for the confusion, shoulda read the original post more closely!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants