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

interp: some binary tests should use space trim before atoi #928

Closed
ihar-orca opened this issue Oct 10, 2022 · 3 comments · Fixed by #935
Closed

interp: some binary tests should use space trim before atoi #928

ihar-orca opened this issue Oct 10, 2022 · 3 comments · Fixed by #935

Comments

@ihar-orca
Copy link
Contributor

It seems that the Bash trims spaces before integer comparison:

> if [ " 14 " -lt "22" ]; then  echo "true"; fi
< true

> if [ " 14 " -eq "14" ]; then  echo "true"; fi
< true

The sh/interp uses strict comparision rules and does not use any space trim functions before the binary tests that are defined in interp/test.go (binTest func, atoi).

@mvdan
Copy link
Owner

mvdan commented Oct 10, 2022

Thanks for reporting. Want to send a PR? We should check other forms of arithmetic expressions too, like let x=" 3 " or your example with [[ as well.

@ihar-orca
Copy link
Contributor Author

no, currently I don't have any relevant and correct PR. for the local issue I just made a call for strings.TrimSpace inside each atoi but I think it's not the best idea.

@mvdan
Copy link
Owner

mvdan commented Oct 23, 2022

For the record, this is a simple form of #754. When it comes to arithmetic expressions, we don't expand quoted strings into fields. If we did that, then the spaces would go away in your example.

That said, this scenario is indeed easy to solve with TrimSpace, so it seems fine to me. cc @riacataquian

mvdan added a commit that referenced this issue Oct 23, 2022
mvdan added a commit that referenced this issue Oct 26, 2022
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 a pull request may close this issue.

2 participants