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

Word splitting: filter out empty fields #15

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tucak
Copy link
Contributor

@tucak tucak commented Nov 20, 2019

I'm unsure where the POSIX specification mentions this, but all of the implementations I tried (bash, dash, mksh, zsh) seem to agree on it.

Unquoted fields that resulted from expansion and contain nothing but
IFS whitespace are now removed during splitting.
@mgree
Copy link
Owner

mgree commented Nov 21, 2019

Interesting, thank you! What did Smoosh do on this test case before your changes?

@tucak
Copy link
Contributor Author

tucak commented Nov 21, 2019

Before the change, some of it would end up as an empty field, but the last one was ignored. The output of the test was:

Default:
3
3
4
4
Colon:
3
4
4
4
Unset:
3
3
4
4

@mgree
Copy link
Owner

mgree commented Nov 21, 2019

Gotcha. Let me double check that this change doesn't hurt POSIX conformance. (If I don't get back to you within a week, please ping me!)

When an expansion ends with a field separator, the following user
field separator should be merged into it. The easiest way to achieve
this is to turn them into whitespace field separators.
@tucak
Copy link
Contributor Author

tucak commented Dec 2, 2019

I wasn't satisfied with the previous approach as it didn't actually fix the actual issue that caused the empty fields to remain. They should have been removed in combine_fields as they only contained white-space. The commit I added should fix it, by turning the user-field separators following the expanded words into whitespace.

The additional test passes on bash, dash, mksh and zsh (I did not test any other), but fails on smoosh without this commit. In case you are curious, the results without this change (the actual argument counts) are:

  • 1 (correct)
  • 3 (incorrect)
  • 2 (correct)
  • 5 (incorrect)
  • 3 (correct)

@mgree
Copy link
Owner

mgree commented Dec 2, 2019

Thanks for the update! I still haven't had a chance to fix the POSIX suite under Docker, but will hopefully have time after I'm back from Thanksgiving travel.

For the last variable in read, the original separators need to be
preserved.
@tucak
Copy link
Contributor Author

tucak commented Dec 22, 2019

Another change, this time to preserve delimiters for the last variable in read. Before this, it would replace every delimiter with a space.

It's a bit more involved than the previous changes, but I found no easier way to do it.

@mgree
Copy link
Owner

mgree commented Aug 26, 2020

I'm planning on migrating to GPLv3 for Smoosh. (Integrating with the GPLv3 Morbig parser will require it.)

Would you like me to merge your code first, while things are still under an MIT license, or is it okay if I wait and the merged code will be under GPLv3?

As I make the change, I plan on making a CONTRIBUTORS.md file in the root, and I'd like to acknowledge your work there. How would you like to be listed?

@tucak
Copy link
Contributor Author

tucak commented Aug 27, 2020

Take your time, I don't mind the GPL at all. Do you need my legal name for the contributors file?

@mgree
Copy link
Owner

mgree commented Aug 27, 2020

I'm honestly unsure what I need. I think the licensing itself works best if contributors disclaim copyright/assign it to me. I was thinking of CONTRIBUTORS.md as being for credit more than for any licensing issues, but I'm really not that up on the details.

If you have a thought or preference here, I'm happy to honor it.

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.

2 participants