Skip to content

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
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)

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 ?

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

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
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
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
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
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
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
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

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
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