chmod -R u+w checkout directory before clearing

This should do it ... need to find a 'breaking' test which can confirm it. :)

Eliminated a redundant chmod on the sites/default directory.

Second missed apostrophe tonight. :(

Tested on scratchtestbot and committed to 6.x-2.x (d95a082).

+++ b/review/client.inc
@@ -345,13 +345,10 @@ abstract class pifr_client_review {
-        if (!chmod($sites_default, 0777)) {
...
+      if (!$this->exec('chmod -R u+w ' . $this->checkout_directory . '/sites/default')) {

Shouldn't it be: a+w ?

I checked on the last patch which broke testbot, and the files were still owned by www-data. The chmod is executed as www-data, so it likely won't be able to modify anything that it didn't already own, anyway.

Can you provide a link to your last patch which broke things this way? I'll run it through a dev testbot before we roll this into a new release candidate.

I can only refer to #1364798: Impossible to generate meaningful diffs of upgrade db dumps, but that seems to be the issue you already tested.

As you're making the extra step already, and the sole purpose is to change permissions in order to be able to delete the checkout files right afterwards (correct?), I'd say it would make more sense to go the full way and ensure that files can be deleted no matter what.

The previous code did exactly that (0777), merely limited to settings.php, instead of all files.

As the chmod is run as the www-data user, it would not succeed for files not already owned by www-data ... thus 'owned by www-data' becomes a pre-assumption for this patch, and a+w does not buy us anything extra.

Of course ... that path won't exist during the reconfirmation process. I'll strip it back to just the '/checkout' directory.

Thanks, rfay!

The code is already wrapped in a 'if exists' condition for the checkout directory itself ... so this patch should allow the reconfirmation to happen.

However, if the directory removal fails, that could result in future tests running with an invalid settings.php file. I think a better option would be to have the testbot disable itself if the 'remove checkout directory' failure is detected (I'll create a new feature request for that scenario.)

Edit: Ah, I get what you're saying. I had left out the return statement to ensure that the 'rm' command would also be executed ... but the error would need to be cleared after the rm anyway, so I'll just leave it out altogether.

Boo to extra whitespace.

Double Boo to whitespace! (Starting to hate this patch ... remove two lines, and somehow add two different whitespace errors?)

I give up. \o/

Fixed in 6.x-2.8-rc4.