Do you really want to continue with the update process? (y/n): n
Aborting. [14.57 sec, 23.35 MB]                                                                                                                   [cancel]
Changes made in drush_pm_updatecode have been rolled back. [14.62 sec, 20.39 MB]                                                                   [rollback]
Running: /usr/src/drush-HEAD/drush.php  --root='/var/www/drupal-6.x-cvs' --uri='http://d6' updatedb --backend [14.62 sec, 20.38 MB]                 [command]
CommentFileSizeAuthor
#7 drush-do-not-rollback-on-cancel.patch989 bytesgreg.1.anderson
#4 drush-pm-update-resultcode.patch736 bytesgreg.1.anderson
#1 drush-1013208.patch845 bytesjonhattan

Comments

jonhattan’s picture

jonhattan
Primary language Spanish
Location Plasencia
commented
StatusFileSize
new drush-1013208.patch845 bytes

Original issue: #1008118: Standardize on `return drush_user_abort();` as correct way to stop execution when user cancels

I don't see the reason for drush_user_abort() to return FALSE if user aborting should not produce a rollback. Anyway attached patch respect this and prevent from running rollback for drush_user_abort(). I don't know the proper solution.

Also, as a side note: any call to drush_user_abort() is done after drush_confirm() --an exception is a call to drush_choice(). If drush_user_abort() use die() or exit() it could be built-in in drush_confirm().

jonhattan’s picture

jonhattan
Primary language Spanish
Location Plasencia
commented
Status: Active » Needs review
moshe weitzman’s picture

moshe weitzman commented

I think the fix would be change updatecode so that it does not return false in this situation. the FALSE does not come from drush_user_abort().

In your except, I see updatedb running after updatecode but that should not be happenning when a rollback has initated. All 'post' hooks should disable. drush_pm_post_pm_update() should not get called. Hmmm.

Agree with critical priority here. I will look into this.

greg.1.anderson’s picture

greg.1.anderson commented
StatusFileSize
new drush-pm-update-resultcode.patch736 bytes

The proposed solution is not right, because it allows post hooks to execute. At a minimum, we need this:

        if (drush_get_error() || ($result === FALSE)) {
          if ((!drush_get_context('DRUSH_USER_ABORT', FALSE))) {
            $rollback = TRUE;
          }
          // break out of the foreach variations and foreach list
          break 2;
        }

However, as I mentioned in the above-quoted issue, I think that rollbacks should happen if the user aborts. The rollback is not causing any problem with pm-updatecode; the only issue I see here is that pm_update is not exiting with the correct result code, so its post hook is still running, even after the user cancels.

The later problem is fixed by the enclosed patch. "Fixing" the rollback issue on user cancel would be a mistake, in my opinion.

jonhattan’s picture

jonhattan
Primary language Spanish
Location Plasencia
commented
Status: Needs review » Reviewed & tested by the community

I did understand the opposite in the quoted issue. Anyway I'm fine with #4

moshe weitzman’s picture

moshe weitzman commented
Status: Reviewed & tested by the community » Fixed

I was also thinking that rollback is a reasonable outcome so I've just committed Greg's patch in #4 and marked this fixed.

greg.1.anderson’s picture

greg.1.anderson commented
Status: Fixed » Needs review
StatusFileSize
new drush-do-not-rollback-on-cancel.patch989 bytes

A reasonable compromise would be to skip the rollback on user cancellation only for the function that caused the user abort. Patch enclosed.

moshe weitzman’s picture

moshe weitzman commented
Status: Needs review » Fixed

IMO we need not blemish drush_invoke() with abort specific code. The patch I committed in #4 is sufficient IMO.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.