Skip to content

Conversation

ElvishJerricco
Copy link
Contributor

Fixes #26955

This might be considered a breaking change, because the example from the bug report (f "/tmp/x/\x20a\nb" 0644 0 0 - \x20foo\nbar) will now produce a file whose name has the escaped characters instead of a file named x20anb. But this behavior reflects the current documentation that says C style escapes are supported.

@github-actions github-actions bot added the please-review PR is ready for (re-)review by a maintainer label May 6, 2023
@yuwata yuwata added the tmpfiles label May 8, 2023
@keszybz
Copy link
Member

keszybz commented May 9, 2023

In cases like this, it is paramount to include a test case. Please extend either test/test-systemd-tmpfiles.py or test/units/testsuite-22.* to test this. Ideally, first add a commit that adds the test (with the old behaviour), and then do you change and adjust the test in the second commit. The diff for the second commit will show what changed.

@keszybz keszybz added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed please-review PR is ready for (re-)review by a maintainer labels May 9, 2023
@keszybz
Copy link
Member

keszybz commented May 9, 2023

This might be considered a breaking change, because the example from the bug report (f "/tmp/x/\x20a\nb" 0644 0 0 - \x20foo\nbar) will now produce a file whose name has the escaped characters instead of a file named x20anb. But this behavior reflects the current documentation that says C style escapes are supported.

Yeah, I agree that treating this is a bugfix sounds fine. People were unlikely to put in escape sequences that didn't actually work.

@github-actions github-actions bot added tests please-review PR is ready for (re-)review by a maintainer and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels May 26, 2023
@ElvishJerricco
Copy link
Contributor Author

@keszybz How does that look for the test? First is a commit where the test passes the old behavior, and then the commit that changes the behavior also changes the test to reflect the new behavior.

@yuwata yuwata added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed please-review PR is ready for (re-)review by a maintainer labels May 26, 2023
Copy link
Member

@keszybz keszybz left a comment

Choose a reason for hiding this comment

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

LGTM.

@keszybz keszybz added ci-failure-appears-unrelated and removed good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed labels May 26, 2023
@keszybz keszybz merged commit c4f521a into systemd:main May 26, 2023
@ElvishJerricco
Copy link
Contributor Author

Thanks! It'd be nice to have this backported to stable.

@ElvishJerricco
Copy link
Contributor Author

@keszybz Any chance of this getting backported?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

tmpfiles.d rules do not allow C escapes, contrary to documentation
3 participants