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

use sed -r instead of -E when using GNU sed #171

Merged
merged 4 commits into from
Sep 26, 2023
Merged

use sed -r instead of -E when using GNU sed #171

merged 4 commits into from
Sep 26, 2023

Conversation

Un1q32
Copy link
Contributor

@Un1q32 Un1q32 commented Aug 17, 2023

sed -r and -E do the same thing, but sed -E is the portable version that works with all versions of sed, however some old versions of GNU sed only support -r, a workaround is to check if we're running GNU sed, and use -r if we are

@Un1q32
Copy link
Contributor Author

Un1q32 commented Sep 21, 2023

bump

Copy link
Collaborator

@catumin catumin left a comment

Choose a reason for hiding this comment

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

Built a version of GNU sed from before -E was added (I used 4.1.5, it was added 4.2), and then tested with that sed, my system's recent GNU sed, and macOS sed. All behaved as they should.

@Un1q32
Copy link
Contributor Author

Un1q32 commented Sep 26, 2023

@BKasin can you try the latest commit, I reworked it into a wrapper function that replaces the arguements if sed is GNU sed, should be completely transparent so nobody has to remember to add $sed_ext to every sed command from now on

@catumin
Copy link
Collaborator

catumin commented Sep 26, 2023

Will do.

Copy link
Collaborator

@catumin catumin left a comment

Choose a reason for hiding this comment

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

Re-tested on previously stated platforms and can confirm the command gets rewritten properly based on GNU vs non-GNU sed. Screenshot shows that detection of GNU sed worked, and the ASCII art printing means that the sed on line 5222 works.

sed

@hykilpikonna hykilpikonna merged commit b1df896 into hykilpikonna:master Sep 26, 2023
@hykilpikonna
Copy link
Owner

Nice fix, thanks!

And @BKasin thank you so much for the detailed testing!

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.

3 participants