Shell aliases starting with a ! don't pass along options and arguments

I will quote myself from #1319108: Conflice between rsync --include and drush --include here:

Finally, it would be possible to provide a function that got the "extra" command options (options not specified in the command record; only usable for drush commands like drush rsync that declare that they accept "extra" options). This special function could also be made aware of global vs. local options.

I think that implementing this function would help here.

Regarding backend, this is the more difficult question. I do not think I could give a definitive answer without writing some code, but here are some impressions.

First point is that if drush finds drush @remote command, then it will immediately see that @remote is a remote alias, and it will use backend invoke to call drush on the remote machine before it ever tries to find or evaluate command. I think that it would benefit shell aliases, and probably other portions of drush, to change the order of this evaluation so that drush will search for the command prior to processing the alias. If we did this, then we could add a flag in the command record that indicated that the command should always be dispatched locally, even if the site alias is remote. Using this new flag in the site alias implementation would allow us to remotely execute the shell alias directly. To do this, see the implementation of the dssh alias in examples/example.bashrc, and the implementation of drush ssh. I don't think we have an appropriate wrapper to use, but one could be created. We cannot use backend invoke directly, because backend invoke always calls drush, and would not work correctly if it did not (c.f. drush_backend_output). I would recommend against modifying backend invoke to support non-drush uses, as this sort of change could get in the way of future planned changes to support better concurrency and interactivity when invoking drush. Making a new remote shell wrapper based as recommended above would be better, and also fully interactive if the pattern from the dssh alias is followed.

I'm willing to work on this, but it would be some time before I could get to it. I'll certainly help review and test if you want to dive in.

One step at a time, for the backend issue: this patch adds a flag that can be attached to command records, 'handle-remote-commands'. If this attribute exists in the command record, then the command will be executed locally, even if --remote-host is passed on the command line or a remote site alias is specified (i.e. drush @remote !alias. The --remote-host option can be examined directly, or the name of the site alias specified (e.g. "@remote") can be retrieved from the 'DRUSH_TARGET_SITE_ALIAS' context.

This patch does nothing by itself, and has no affect on the test cases (phpunit --filter=sqlSync . is currently failing in master with and without this patch; have not looked into this yet). Next step is to add 'handle-remote-commands' to the core-execute command and have it generate an ssh wrapper when the command is remote.

Nothing is yet done vis-a-vis passing command line options through per #3.

Here is a patch that includes #2 and #4, but not #5. I did it offline, but by coincidence it is mostly independent of #5. This patch introduces the following:

'DRUSH_COMMAND_ARGS' context: This contains all commandline args after the command. For example, for drush core-execute du -s, the DRUSH_COMMAND_ARGS context would contain 'du' and '-s'.

'DRUSH_GLOBAL_CLI_OPTIONS' context: This contains all options that come before the command. For example, for drush core-execute -s ls -l this would contain 's' => TRUE.

'mask-local-cli-options' command attribute: If this is set, then it overwrites the 'cli' option with the contents of 'DRUSH_GLOBAL_CLI_OPTIONS'. Short-form options are converted to long-form options at the same time. Setting this option allows 'du -s' to pass the -s option to the du command without setting the drush --simulate mode.

By way of these contexts and this flag, the core-execute command now passes local options on to the shell command.

No attempt was made to put in an ssh wrapper around remote commands. Once this is merged with #5, it should be easy to do that.

One last thing to consider is whether and how 'command-specific' options should interact with shell aliases. Might be useful to add a few lines of code so that they do. This would allow site aliases to add options to shell aliases. Don't have a use case at the moment, so we could put this off and see if a need develops.

Out of time for today, don't know when I'll next have a chance to move this forward.

The problem with DRUSH_COMMAND_ARGS mentioned in #9 is in fact a bug in my code; I've made a naive fix the works fine, and am in the process of refactoring some code to make it work better. Patch coming shortly.

Here is a fix for the bug mentioned in #10. No time at the moment to continue with todos from #9; will pick up #3 (drush @remote ssh) and #4 (tty) next.

Here's a reroll against HEAD of master. Didn't have time to add anything new.

The tests need to worry about the order they insert global options into the command line to test now, at least for shell aliases and core-execute.

This patch is closer, but not quite there yet. Not sure if I'll have any more time this week to work on this.

I advanced #15 a touch here, and can probably post another patch tonight or tomorrow.

Regarding position of global parameters, it is often useful for drush to care about this. In addition to the commands mentioned in #16, drush rsync and drush ssh could also take advantage of the feature added for shell aliases, and as time goes on other commands might want to use single-letter command options, etc. As the number of commands that need strictly-ordered options grows, it might become confusing that sometimes a -s or -d can be thrown on the end of the cli, and sometimes it cannot.

I also really like the capability of being able to add a -s on the end of the line, and at the same time I think that the new ability to distinguish --simulate from du -s is also very important. What would you think about having a separator, such as "::", to resume global options at the end of the cli?

e.g.

drush -s core-execute du -s

is the same as

drush core-execute du -s :: -s

Other magic separator characters could also be used, but it shouldn't be anything like --, which is likely to be used by some gnu/linux commands.

Okay, I agree that we should proceed without the global-on-the-end separator, and keep option ordering flexible for all commands except those that really need and request it. If we do have a need later, we can always document it at the top of help:

Usage: drush --global-options @target-site command --local-options :: --more-global-options

I think I have the test cases all cleaned up; running the full suite to verify.

Sql-sync test still failing. Suggest handling items mentioned in #9 in separate issues.

Fixed items mentioned in #22 and pushed to master; sorry, there were some extra commits in there.

Commit 89fac3d broke shebang scripts when --php-options are defined. The problem is that in this patch, the drush script was modified so that --php and --php-options were passed at the beginning of the argument list, whereas previously they were passed at the end. Putting these options at the beginning of the argument list, before the drush command, is very important when using commands with strict option handling, because otherwise the --php and --php-options are interpreted as being part of the command options, instead of Drush options as they should be.

However, putting --php and --php-options at the beginning of the argument list puts them in the way of the shebang script argument handling code, which was previously being given:

drush [--optional-shebang-flag] /path/to/script command options

and is now being given:

drush --php=/path/to/php --php-options='-d ...' [--optional-shebang-flag] /path/to/script command options

If there is no optional shebang flag (as there usually isn't -- don't know if anyone has ever used this feature), then shebang scripts can survive with just the --php option, but they would die if --php-options were also specified.

Here is a patch that fixes the issue. I would just commit this as a bug fix, but there is something that bears discussion here. This solution skips past the --php and --php-options added by the drush script by skipping every leading option that begins with "--php". It would also be possible to just assume that the drush script always adds exactly two options rather than test for the common prefix. In either event, the issue that will arise is that any future changes to the options inserted by the drush script will once again break shebang script processing. One solution would be to just commit this with a comment in the drush script. Another possibility might be to add some sort of "marker" option (like the "--" used by some shell commands to separate options from filename arguments) at the end, after --php-options, to signify that the options added by the drush script are over. This feels a little hackish, though. Thoughts?

This is a pretty minor issue; I will just commit with a comment in a couple of days if no one has any specific feedback.

I decided to just commit this with the comment so folks don't waste their time tracking down the unit test failure. (The shebang test seems to be skipped by Travis.) Re-open with comments if any modifications are desired.