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

Account for mktemp failures #7482

Merged
merged 2 commits into from Nov 14, 2020
Merged

Account for mktemp failures #7482

merged 2 commits into from Nov 14, 2020

Conversation

ericonr
Copy link
Contributor

@ericonr ericonr commented Nov 13, 2020

Description

Discussed in #7477, catches when mktemp errors out and exits the script / function.

I can try to add some error case that exports TMPDIR to some place that can't be written, if you find it necessary.

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

@mqudsi
Copy link
Contributor

mqudsi commented Nov 13, 2020

Why don't we just add a tests function called mktemp? We need to do that anyway to get the tests to pass on older macOS versions that don't support the same syntax that Linux and newer macOS do.

@faho
Copy link
Member

faho commented Nov 13, 2020

Why don't we just add a tests function called mktemp?

We would still have to add or return 1 everywhere, because the function won't return out of the calling function.

We need to do that anyway to get the tests to pass on older macOS versions that don't support the same syntax that Linux and newer macOS do.

As far as I'm aware, all these uses work on macOS and linux - the issue was stuff like too short templates or using templates with files instead of directories and such.

@ericonr
Copy link
Contributor Author

ericonr commented Nov 13, 2020

I don't really have a horse in this race, I'm only extending the patch from my previous PR :)

So I'm happy to proceed with whatever's decided.

@faho faho added this to the fish 3.2.0 milestone Nov 14, 2020
@faho faho merged commit 31870e7 into fish-shell:master Nov 14, 2020
@faho
Copy link
Member

faho commented Nov 14, 2020

Okay, I merged this because it's a good thing by itself. Making a compatibility function can be done later, and isn't immediately necessary as far as I can tell.

@mqudsi
Copy link
Contributor

mqudsi commented Nov 15, 2020

Agreed. Thanks, all!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants