Force removal of test directories in cleanup - #336
Open
ekamran wants to merge 1 commit into
Open
Conversation
`rm -r` without `-f` prompts for confirmation on write-protected files, such as the read-only git object files in a nested checkout, leaving cleanup.php waiting for input that can never arrive. The two sibling commands in the same array already use `rm -rf`, so the missing flag looks like an oversight rather than a choice. The same applies to the default `WPT_RM_TEST_DIR_CMD`, which is executed on the remote host over SSH. Anyone who has overridden that variable should include `-f` in their own command as well, so the README now says so in both places it documents the variable. That default also builds a shell command by concatenating `WPT_TEST_DIR` directly, so a path containing a space was split into several targets and a path containing a semicolon ran as a second command on the remote host. It is now passed through `escapeshellarg()`. The value is escaped again when the whole command is handed to `ssh`, which unwraps correctly. This weakness predates the change, but forcing removal makes it worth closing here rather than leaving it on a line this commit already touches. Genuine failures such as permission errors still return a non-zero exit code and are still reported by `perform_operations()`. One behaviour does change: removing a path that does not exist now succeeds quietly, where before it reported a failure. Cleaning an already clean directory is not an error, so that seems like the better outcome, but it is a change. Fixes WordPress#301
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @marlonanjos. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #301
cleanup.phpremoves the preparation directory withrm -r. Without-f,rmstops to ask about write-protected files such as the read-only objects in a nested git checkout, and cleanup waits for input that never arrives. The two commands beside it already userm -rf, so the missing flag looks like an oversight.Changes
cleanup.php: remove the preparation directory withrm -rf, matching its neighbours.functions.php: the same flag on the defaultWPT_RM_TEST_DIR_CMD, which runs over SSH.functions.php: that default concatenatedWPT_TEST_DIRunescaped, so a path with a space became several targets and a path with a semicolon ran a second command on the remote host. It now goes throughescapeshellarg(). Older than this change, but forcing removal makes it worth closing here.README.md: both places documentingWPT_RM_TEST_DIR_CMDnow showrm -rf, since anyone who set it by hand still has the original problem.Testing
On a directory containing a read-only file, with a terminal attached:
rm -rasksoverride r--r--r--...?and then fails withDirectory not empty. The directory survives.rm -rfremoves it, no prompt.rm -rfstill returns a non-zero exit code on genuine failures, soperform_operations()still reports them. One behaviour changes: removing a path that does not exist now succeeds quietly instead of reporting a failure.php -lpasses on both changed files, and PHPCS shows the same single pre-existing warning asmaster.On macOS the prompt reads
override ...?rather than theremove write-protected regular file ...?wording in the report.