updatecode: --version-control=backup stop updatecode --package-handler=cvs to use cvs update and force always a checkout.

This would also fix the problem where php's rename fails for cross-volume moves. Would copying instead of moving cause problems when files are removed from a module? There are existing issues about this, I think. Copy instead of move would at least make it consistent -- drush would need to explicitly remove files that no longer belong in the directory. So, in conclusion, I think the proposed fix is a good idea.

I was planning on using rsync, as drush has a nice wrapper function for it. However, moshe thought that rsync wasn't 'core' enough to use in a critical function such as pm-updatecode. Windows support only seems to be getting sketchier in drush over time, though, so maybe this decision could be revisited...? Are we ready to punt Windows-native support for drush, and require at least cygwin on Windows?

Looks good, but let's be kind to Windows users and do it with a recursive php function. This also improves on #968948: Drush returns an error on dl command on subdomain named subsites.domain.com.

Good!
Some comments:

+++ commands/pm/version_control/backup.inc	2 Dec 2010 18:34:09 -0000
@@ -19,13 +19,21 @@ class drush_pm_version_control_backup im
+      // Move or copy to backup target based in package-handler.
+      $ph = drush_get_option('package-handler', 'wget');
+      if ($ph == 'wget') {
+        if (drush_move_dir($project['full_project_path'], $backup_target)) {
+          return TRUE;
+        }
       }
-      return TRUE;  
-    }
-    else {
-      return FALSE;
+      // cvs or git.
+      else {
+        if (drush_copy_dir($project['full_project_path'], $backup_target)) {
+          return TRUE;
+        }
+      }
+      return drush_set_error('DRUSH_PM_BACKUP_FAILED', dt('Failed to backup project directory !project to !backup_target', array('!project' => $project['full_project_path'], '!backup_target' => $backup_target)));

- less indentation is possible.
- the error never occurs.

+++ includes/drush.inc	2 Dec 2010 18:34:10 -0000
@@ -578,6 +578,72 @@ function drush_delete_dir($dir) {
+  else {
+    if (copy($src, $dest) !== TRUE) {
+      return FALSE;
+    }
+  }

- less indentation is possible.

Update: The observation about the error is wrong, clearer patch in a while.

Here is a new patch with less indentation.

The error DRUSH_PM_BACKUP_FAILED actually will occur if drush_move_dir or drush_copy_dir returns FALSE (e.g. permissions error).

Yes, I was wrong about the error. Alternative proposal:

+      // Move or copy to backup target based in package-handler.
+      $ph = drush_get_option('package-handler', 'wget');
+      if ($ph == 'wget') {
+        return drush_move_dir($project['full_project_path'], $backup_target);
       }
-      return TRUE;  
-    }
-    else {
-      return FALSE;
+      // cvs or git.
+      elseif ($ph == 'cvs' || $ph == 'git_drupalorg') {
+        return drush_copy_dir($project['full_project_path'], $backup_target);
+      }
+      return drush_set_error('DRUSH_PM_BACKUP_FAILED', dt('Failed to backup project directory !project to !backup_target', array('!project' => $project['full_project_path'], '!backup_
     }

Rudimentary tests OK for #9 and #10. You choose.

#10 removes too much indentation and fails to return the correct error code should drush_move_dir or drush_copy_dir fail.

I recommend #9, but will let jonhattan confirm and commit.