Skip to content

Remove unnecessary special cases for bash/zsh #2086

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

Merged
merged 2 commits into from
Apr 20, 2021
Merged

Conversation

orent
Copy link
Contributor

@orent orent commented Mar 31, 2021

The internal shell utility hash is standard posix. There is no need to special-case it only for bash and zsh.
In fact, this check can break the script on shells such as the built-in sh on BSD systems, ksh, ash or
busybox sh which are otherwise perfectly capable of running it. The script is a valid posix shell script, as
verified by shellcheck. The one feature that depends on bash (checking that the script is sourced rather
than executed) will work only on bash, but it is only a safety check.

https://pubs.opengroup.org/onlinepubs/9699919799/utilities/hash.html

orent and others added 2 commits March 31, 2021 09:32

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
The internal shell utility hash is standard posix. There is no need to special-case it only for bash and zsh.
In fact, this check can break the script on shells such as the built-in sh on BSD systems, ksh, ash or 
busybox sh which are otherwise perfectly capable of running it. The script is a valid posix shell script, as
verified by shellcheck. The one feature that depends on bash (checking that the script is sourced rather 
than executed) will work only on bash, but it is only a safety check.

 https://pubs.opengroup.org/onlinepubs/9699919799/utilities/hash.html
@codecov
Copy link

codecov bot commented Mar 31, 2021

Codecov Report

Merging #2086 (c097017) into main (0262fa6) will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2086      +/-   ##
==========================================
- Coverage   93.70%   93.68%   -0.03%     
==========================================
  Files          88       88              
  Lines        4383     4383              
==========================================
- Hits         4107     4106       -1     
- Misses        276      277       +1     
Flag Coverage Δ
tests 93.68% <ø> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/virtualenv/seed/embed/base_embed.py 96.22% <0.00%> (-1.89%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0262fa6...c097017. Read the comment docs.

Copy link
Contributor

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

Can you please add a changelog entry?

Copy link
Contributor

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

@gaborbernat gaborbernat merged commit 8500df6 into pypa:main Apr 20, 2021
@gaborbernat
Copy link
Contributor

Released via https://pypi.org/project/virtualenv/20.4.4/

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

2 participants