platformsh / legacy-cli

This is the legacy version of Platform.sh's command-line interface. The new version is at: https://github.com/platformsh/cli
https://docs.platform.sh/administration/cli.html
MIT License
221 stars 120 forks source link

Preserve --include/--exclude order when using mount:download #1301

Closed akalipetis closed 1 year ago

akalipetis commented 1 year ago

Under the hood, mount:download is using rsync which is a bit peculiar when it comes to --include and --exclude arguments. The order of the arguments matters and to make things even worse, it seems like it's different between rsync versions.

I'd suggest the following things:

  1. If --include is used without an --exclude, always append '--exclude=*' as it will not work otherwise
  2. Include --exclude argument before --include or at least preserve the user order, otherwise it does not currently work on MacOS with rsync version 2.6.9
Examples that validate the above ```console ➜ rsync -avzn '6m7pxz7aqf5aa-main-bvxea6i--github-app-poc@ssh.eu-3.platform.sh:.cache/' .cache receiving file list ... done created directory .cache ./ one.txt two.pdf sent 34 bytes received 124 bytes 63.20 bytes/sec total size is 0 speedup is 0.00 ➜ rsync -avzn '--include=*.txt' '6m7pxz7aqf5aa-main-bvxea6i--github-app-poc@ssh.eu-3.platform.sh:.cache/' .cache receiving file list ... done created directory .cache ./ one.txt two.pdf sent 45 bytes received 124 bytes 112.67 bytes/sec total size is 0 speedup is 0.00 ➜ rsync -avzn '--include=*.txt' '--exclude=*' '6m7pxz7aqf5aa-main-bvxea6i--github-app-poc@ssh.eu-3.platform.sh:.cache/' .cache receiving file list ... done created directory .cache ./ one.txt sent 46 bytes received 105 bytes 100.67 bytes/sec total size is 0 speedup is 0.00 ➜ github-app-poc git:(main) ✗ rsync -avzn '--exclude=*' '--include=*.txt' '6m7pxz7aqf5aa-main-bvxea6i--github-app-poc@ssh.eu-3.platform.sh:.cache/' .cache receiving file list ... done ./ sent 40 bytes received 82 bytes 48.80 bytes/sec total size is 0 speedup is 0.00 ```
pjcdawkins commented 1 year ago

If --include is used without an --exclude, always append '--exclude=*' as it will not work otherwise

Does it depend what you expect --include to do? Both your examples ended up syncing the .txt files that were requested.

man rsync says:

  --include=PATTERN        don't exclude files matching PATTERN

Exclude before include makes sense

akalipetis commented 1 year ago

Does it depend what you expect --include to do? Both your examples ended up syncing the .txt files that were requested.

Yes, that's intended for rsync, but our help message mentions:

      --include=INCLUDE              File(s) to include in the download (pattern) (multiple values allowed)

Personally, I would expect --include to only include the given pattern.

pjcdawkins commented 1 year ago
  1. Include --exclude argument before --include

It looks like we already do this part: https://github.com/platformsh/legacy-cli/blob/7fa4b45ed27f080bb0dd9c2e4b1858bf3bf1a13b/src/Service/Rsync.php#L131-L134

akalipetis commented 1 year ago

Yes, you're correct. I messed up the order - it seems like the correct order is the opposite one (include before exclude).

PLATFORMSH_CLI_DEBUG=1 platform mount:download -m .cache --target .cache --yes --include='*.txt' --exclude='*'
...
# Running command: 'rsync' '--archive' '--compress' '--human-readable' '-vv' 'XXX:.cache/' '.cache' '--exclude=*' '--include=*.txt'
...
[sender] hiding file one.txt because of pattern *
[sender] hiding file two.pdf because of pattern *

What we'd like to do is:

'rsync' '--archive' '--compress' '--human-readable' '-vv' 'XXX:.cache/' '.cache' '--include=*.txt' '--exclude=*'
...
[sender] showing file one.txt because of pattern *.txt
[sender] hiding file two.pdf because of pattern *
pjcdawkins commented 1 year ago

OK reading deeper in the man page:

   The order of the rules is important because the first rule that matches is the one that takes effect.  Thus, if an early rule excludes a file, no include rule that comes after
   it can have any effect. This means that you must place any include overrides somewhere prior to the exclude that it is intended to limit.
pjcdawkins commented 1 year ago

I've updated the PR #1304 to address that