Download & Extend

rsync should allow multiple --exclude options to be specified

Project:Drush
Version:All-versions-5.x-dev
Component:Core Commands
Category:bug report
Priority:normal
Assigned:greg.1.anderson
Status:closed (fixed)
Issue tags:Release blocker

Issue Summary

--exclude-files option does not do the job - will send unexpanded "%files" to rsync

drush rsync @src @dst --exclude-files -v
...
Calling system(rsync -e 'ssh ' -azv --exclude="%files," ...);

Comments

#1

furthermore, if --exclude-files is set (and only then) --exclude-paths seems not to work

#2

I confirm the bug exists on 4.5 as well.

#3

Still affected with today 5.0-dev from master branch.
Sticking with --exclude-paths="sites/default/files/" option.

#4

Assigned to:Anonymous» greg.1.anderson

Yes, I confirmed that this code path is in fact broken, and has been for some time. Haven't had time to repair it yet.

#5

sub

#6

Tagging as release blocker for drush-5-RC1

#7

Status:active» needs review

Here is a patch that fixes most uses of --exclude-files and --exclude-paths. There is one edge case, where a source-command-specific option in an alias containing a --exclude-paths option is dropped if used with --exclude-files. Fixing that would require rewriting the function, something I don't have time for now, so I hope we can just commit with this limitation. Included is a unit test that identifies what does and does not work.

AttachmentSize
rsync-exclude-files.patch 8 KB

#8

Status:needs review» reviewed & tested by the community

Looks good. Thanks for improving this. Feel free to remove Release blocker tag if you think this is good enough.

#9

Status:reviewed & tested by the community» active

Committed #7; however, I thought of one additional enhancement. If we made drush core-rsync process arguments the same way that drush ssh does, then it would be possible to specify --exclude multiple times. Then you would not need to use --exclude-paths. I think I'd leave --exclude-paths in, for those who were used to using it.

#10

Title:"rsync --exclude-files" broken» rsync should allow multiple --exclude options to be specified

#11

Status:active» needs review

Here is a patch that fixes the bug in #7 and allows multiple --exclude and --include options to be used -- just like the rsync command itself. In this patch, 'mask-local-options' is renamed to 'strict-option-handling', which is more descriptive. core-rsync is modified to use strict option handling, so you must now use drush --simulate core-rsync @a @b; drush core-rsync @a @b -s will no longer do the trick, as the -s option will now be passed on to the rsync command. A small price to pay for better rsync option handling.

Oh, this patch also enhances strict option handling to support command-specific options, which will also be added on to the command's local options. As an added bonus, with this patch it is no longer necessary for Drush to keep an exhaustive list of all available rsync options.

AttachmentSize
drush-rsync-strict-option-handling.patch 18.13 KB

#12

Status:needs review» needs work

#11 is buggy -- it inserts extra stuff into rsync's options. This is closer -- almost, but not quite there yet.

AttachmentSize
drush-rsync-strict-option-handling-12.patch 24.55 KB

#13

Status:needs work» needs review

All fixed up again. The failing test case (source-command-specific --exclude-paths with exclude-files) is still failing (failing test case included w/ assert commented out w/ explanation). All other tests passing. Whew.

AttachmentSize
drush-rsync-strict-option-handling-13.patch 25.56 KB

#14

This patch has make and post-sql-sync stuff in it. Needs a reroll.

#15

Whoops, forgot to rebase my rsync branch. Gotta love how easy git makes fixing stuff like this, though.

AttachmentSize
drush-rsync-strict-option-handling-15.patch 21.3 KB

#16

Status:needs review» reviewed & tested by the community

If the tests are passing, this looks good. Some typos:

1. "-v is an rsync command. ". i think we man 'option' here.
2. "twice times: "

#17

Status:reviewed & tested by the community» fixed

Fixed typos above, ran tests one more time for good measure and committed.

#18

Status:fixed» closed (fixed)

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

nobody click here