Skip to content

uucore/quoting_style: Add support for quoting/escaping empty strings #7693

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Apr 13, 2025
Merged

uucore/quoting_style: Add support for quoting/escaping empty strings #7693

merged 2 commits into from
Apr 13, 2025

Conversation

sargas
Copy link
Contributor

@sargas sargas commented Apr 8, 2025

This PR adds the ability to determine quotations and/or escaping needed for empty strings. This can happen if an empty string is passed to printf.

@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 8, 2025

GNU testsuite comparison:

Skipping an intermittent issue tests/misc/stdbuf (passes in this run but fails in the 'main' branch)

RenjiSann
RenjiSann requested changes Apr 8, 2025
View reviewed changes
Copy link
Collaborator

@RenjiSann RenjiSann left a comment

Choose a reason for hiding this comment

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

Thanks !
Can you also add a test in test_printf.rs as well ?

src/uucore/src/lib/features/quoting_style.rs Outdated
Comment on lines 433 to 432
} else if name.is_empty() {
(Quotes::Single, true)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please account for the clippy warning and merge the if case with the one above.

@sargas sargas force-pushed the escape_empty_string branch from 2017735 to 12fb470 Compare April 8, 2025 11:51
@sargas
Copy link
Contributor Author

sargas commented Apr 8, 2025 •
edited
Loading

Made the clippy change, test_printf is next, but need some more time to figure out why this didn't pass the printf-quote test in CI (it passes locally).

The test failure appears to be from parts of the tests setting LC_ALL=$LOCALE_FR_UTF8 for multi-byte tests. Adding set -x to the test I can see that I don't even run those tests locally (they are skipped if test $LOCALE_FR_UTF8 is none, which seems strange because my understanding is that test is usually used for boolean return values, not its standard output).

From the output in GitHub Action logs, it looks like we have the correct behavior for LC_ALL=$LOCALE_FR_UTF8 but not LC_ALL=C, so it doesn't look like this is fixable with correctly handle locale.

@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 8, 2025

GNU testsuite comparison:

Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)
Congrats! The gnu test tests/misc/tee is no longer failing!

@RenjiSann
Copy link
Collaborator

Our implementation does not handle locale setting at all for the moment. This will change in the future, but in the mean time, the implementation should stick to the LC_ALL=C behavior.

The GNU tests that rely on locale-setting are expected to fail until then.

@sargas sargas force-pushed the escape_empty_string branch from 12fb470 to 4756866 Compare April 9, 2025 03:20
@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 9, 2025

GNU testsuite comparison:

Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)

@sargas
Copy link
Contributor Author

sargas commented Apr 9, 2025

Gotcha, thank you. So this test doesn't pass tests/printf/printf-quote (unless one does not have the French locale installed), but at least it fixes printf %q ''

@sargas sargas force-pushed the escape_empty_string branch from 4756866 to 9d7991e Compare April 9, 2025 05:10
@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 9, 2025

GNU testsuite comparison:

Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/misc/stdbuf (passes in this run but fails in the 'main' branch)

@sargas sargas force-pushed the escape_empty_string branch from 9d7991e to b88afd4 Compare April 9, 2025 11:21
@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 9, 2025

GNU testsuite comparison:

Skip an intermittent issue tests/misc/stdbuf (fails in this run but passes in the 'main' branch)
Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)
Congrats! The gnu test tests/misc/tee is no longer failing!

@sargas sargas force-pushed the escape_empty_string branch from b88afd4 to 3cdc248 Compare April 12, 2025 04:18
@github-actions GitHub Actions
Copy link

GNU testsuite comparison:

Skipping an intermittent issue tests/misc/stdbuf (passes in this run but fails in the 'main' branch)

@sargas sargas force-pushed the escape_empty_string branch from 3cdc248 to 9f1e7a2 Compare April 13, 2025 02:44
@github-actions GitHub Actions
Copy link

GNU testsuite comparison:

Skip an intermittent issue tests/misc/stdbuf (fails in this run but passes in the 'main' branch)

@sargas
Copy link
Contributor Author

sargas commented Apr 13, 2025

I've rebased this PR; it does fix tests/printf/printf-quote except for locale-specific logic, I wasn't intending to add more to this PR unless there's other code review comments

RenjiSann
RenjiSann approved these changes Apr 13, 2025
View reviewed changes
Copy link
Collaborator

@RenjiSann RenjiSann left a comment

Choose a reason for hiding this comment

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

LGTM

@RenjiSann RenjiSann merged commit c35d26d into uutils:main Apr 13, 2025
69 checks passed
@sargas sargas deleted the escape_empty_string branch April 24, 2025 02:06
@BrewTestBot BrewTestBot mentioned this pull request May 24, 2025
Merged
@moonfruit moonfruit mentioned this pull request May 26, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RenjiSann RenjiSann RenjiSann approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sargas @RenjiSann