Standardize on `return drush_user_abort();` as correct way to stop execution when user cancels

Here is a question: should drush roll back after a user cancels? I tend to think that the answer is yes, but so far Drush has for the most part not done this. Watchdog, site install and the core search commands would, but other command either just called drush log (which lets the post hooks run), or drush_die (which I changed to drush_user_abort, which calls drush_die, in #917650: Improve the state of drush error reporting during startup).

I tend to think that I should replace all of the calls to drush_user_abort to instead call this:

/**
 * Call drush_set_error with DRUSH_USER_ABORT.
 */
function drush_set_user_abort() {
  drush_set_context('DRUSH_USER_ABORT', TRUE);
  return drush_set_error('DRUSH_USER_ABORT', dt('Aborting.'));
}

Then rollback functions would be called. Rollback functions are rare (at the moment), so I think this isn't too likely to break existing code, and it could really help someone out in the future if a need to roll back after an abort did come up. I think that should be rare too, though, as you'd -usually- want to confirm before you start making changes. You never know, though...

Thoughts?

I could make everyone call drush_user_abort() after confirm "n". That function just calls 'die', so neither rollbacks nor post hooks will execute. Better than fiddling with the commandfile list, I think.

I'm kind of uncomfortable about exiting via die(). The shutdown function is still called, but it interferes with backend invoke. I don't know that --backend --no is exactly common, but all the same, it doesn't give me a good feeling.

This patch takes a more conservative approach. Any existing function that exited with return drush_log('abort'); will now exit via drush_set_error instead (called indirectly via drush_set_user_abort). This follows established drush conventions for exiting from commands with an error condition. drush_set_error is documented as the preferred method.

Existing code that called drush_user_abort() is unchanged for now (until drush-5, I suppose).

I think that at a minimum we need to fix the pm-* commands, as they are likely to be hooked.

Okay, thanks; I'll re-roll with a completed patch in a little bit.

Here it is; standardized on return drush_set_error();, with no more exit-via-die on user abort.

I did not like the fact that the 'Aborting' message said "[error]", so I altered drush_set_error to permit the caller to change the log type. The default is still 'error', but drush_user_abort says "[ok]" instead, because it is okay to cancel.

The only potential problem here is that if there are any contrib drush commands that call drush_set_error(), before this patch that function exited with die(), and it now returns FALSE. This means that this patch might turn some people's abort function into a no-op.

I agree with your suggestion.

I think that rollbacks should be triggered on FALSE -or- drush_get_error; in the past I was fixing bugs in code that would call drush_set_error but not return FALSE. I prefer making invoke more bulletproof (per #12) to tracking down niggling "errors" in all of the drush commands (and all of contrib...)

I'll re-roll #11 tonight (if you don't commit it first)

Here's a new patch per #12; it is, in fact, much cleaner this way.

There are a couple of spurious tabs-to-spaces fixes in this diff; please ignore.

Wow, those were some bad typos. I don't know how it worked at all. Probably didn't; I think that drush_user_abort was always returning TRUE, but I put my $result on the wrong function, which always returned FALSE, so commands would always cancel. Here's a new patch that fixes the stuff in #15.

When re-testing this patch, I did a 'n' / 'y' sequence one after the other and did not like that both of them ended with an '[ok]' log, so in this patch I introduced a '[cancel]' log type, used only by drush_user_abort. Looks nice; I don't know if there is any reason why we shouldn't add a new log type.

Committed with change from #17 and documentation in all three areas mentioned in #18. http://drupal.org/cvs?commit=471668