Many changes for improved flexibility

[aspiers]

This patch series is a collection of functionality enhancements, bug fixes, and refactorings. The highlights are:

• --delete is now optional.
• Arbitrary arguments can be passed to rsync.
• File removal is handled separately via run_on_removals
• The :excludes option is now an Array of Strings, not a Hash of Regexps. This is a backwards-incompatible change; however it should be possible to reintroduce support for Hashes prior to the next release.

This allows new use cases, in particular where multiple source trees are rsync'd to a single merged destination tree, e.g.:

crowbar/crowbar#1795

Previously the presence of --delete would have meant that any change in one of the source trees would cause files from all the other trees to get wiped from the destination.

[lukemelia]

Adam, thanks for the PR. There is a lot here, and @kselden and I would like to work through them in pieces. Do you mind breaking up the 4 bullet-pointed features in your description into 4 separate PRs and we can provide focused feedback and merge each when ready?

[krisselden]

Thanks for the PR.

Here is a couple quick points of feedback.

• The reasoning behind ensure_no_trailing_slash(options[:input]) is because the difference between trailing slash and no trailing slash (start at directory then recurse vs start at the files in the directory then recurse) affects --delete and --exclude-from. The --delete flag only has an effect recursing into a directory so trailing slash means the root directory is unaffected. The root of absolute exclusions is also affected and if you don't start an exclusion with a slash it is then matched anywhere in the file.
• By moving --delete to just run_on_removals then it can't catch up on deletes that happened while guard was not running (this is the point of run on start).
• The Hash option of --excludes was so that it could ignore files covered by other guard tasks in the group, so you could for example exclude js files that came from coffee but include other js files.

Feel free to contact me via email or google chat to discuss further.

[aspiers]

[Deleted and reposted comment; apparently replying via e-mail disables Markdown - bah 👎]

Hi Kristofor,

• The reasoning behind ensure_no_trailing_slash(options[:input]) is because the difference between trailing slash and no trailing slash (start at directory then recurse vs start at the files in the directory then recurse) affects --delete and --exclude-from. The --delete flag only has an effect recursing into a directory so trailing slash means the root directory is unaffected. The root of absolute exclusions is also affected and if you don't start an exclusion with a slash it is then matched anywhere in the file.
• By moving --delete to just run_on_removals then it can't catch up on deletes that happened while guard was not running (this is the point of run on start).

Yes, I'm aware of those points. However, for some use cases (like the example I gave) the trailing slash does make sense, which is why I strongly believe the choice should be left up to the user. And it is very likely that someone who chooses to use guard-rsync already has good experience of using rsync, and is therefore aware of these subtleties. Therefore applying the Principle of Least Surprise, guard-rsync will provide the most intuitive user experience when it is an un-opinionated transparent wrapper around rsync and doesn't do any unexpected munging of the input parameters.

This is also a good justification for allowing extra flexibility in which options are passed to rsync. For example omitting --delete by default still allows the user to specify it, but does not force it down the user's throat. This is particularly important for --delete and other dangerous options which carry a risk of data loss.

In my use case, the omission of --delete is very specifically required for reasons already explained in my pull request's description, and the resulting inability to catch up on deletes which happened prior to guard start is a necessary and worthwhile trade-off.

• The Hash option of --excludes was so that it could ignore files covered by other guard tasks in the group, so you could for example exclude js files that came from coffee but include other js files.

Ahhh, now I understand; that's nice. So my comment in the comment message for f8fead4 "[that] didn't actually buy anything useful" was wrong. However I still think that all my other points apply. I actually wanted to add back support for Hashes in --excludes in order to avoid breaking backwards compatibility, but didn't get around to that yet. But now I am thinking that this format would be best:

    guard('rsync', {
      :input => 'apps_src/my_app',
      :output => 'apps',
      :excludes => [
        '/.git/',
        { /(.+)\.coffee$/ => lambda { |m| "#{m[1]}.js" } },
        '*.bak',
      ],
    })

since it allows a mix'n'match approach, and means that the performance hit from Dir.glob('*/') is optional and only required if you want to pull the transform trick. How does that sound?

Thanks! :)

[krisselden]

I agree --delete should have been optional.

I don't think your changes to delete is in line with "un-opinionated transparent wrapper around rsync" it is as scenario solving as what I had before, and multiple varying srcs synced to one dest is not as common of a use case as just syncing a single src to a dest exactly.

That being said, one can use https://github.com/guard/guard-shell for a completely un-opinionated wrapper.

Being able to catch up to changes on start was one of my primary use cases which is the idea behind it running a group on start, because my src directory is under source control and I need to catch up to removals from other people as it seems fragile to have to remember I need guard running before I pull or removals will be lost.

Maybe there is a way to take it back to a thin wrapper, and provide enough hooks for the other use cases but at a minimum being able to do a perfect sync of a src to dest that can catch up on start seems in keeping with the principal least surprise.

[krisselden]

I like your mix'n'match proposal to exclusions.

[aspiers]

Thanks for the quick reply ...

I don't think your changes to delete is in line with "un-opinionated transparent wrapper around rsync"

Well, making --delete optional is certainly less opinionated than enforcing it. And silently adding an option the user didn't ask for doesn't strike me as particularly transparent either.

it is as scenario solving as what I had before, and multiple varying srcs synced to one dest is not as common of a use case as just syncing a single src to a dest exactly.

Agreed.

That being said, one can use https://github.com/guard/guard-shell for a completely un-opinionated wrapper.

guard-shell was the first thing I tried even before discovering guard-rsync. But it is no use because it triggers the shell action block once per changed file; this can result in rsync process storms if you do something like a recursive search and replace.

Being able to catch up to changes on start was one of my primary use cases which is the idea behind it running a group on start, because my src directory is under source control and I need to catch up to removals from other people as it seems fragile to have to remember I need guard running before I pull or removals will be lost.

Let's distinguish carefully between "catch up on changes to new/existing files at start" and "catch up on all changes including removals at start". I absolutely agree that the former will be one of the most common use cases, and this pull request provides that as the default behaviour. In contrast, whilst the latter is the current default behaviour, the potential for accidentally causing severe data loss is huge, and can happen merely by misconfiguring either the input or output parameter. IMHO this is not a sensible default. It's trivial for the user to add :extra => '--delete' to their options, but also hard enough that they could not do it without being aware of the risks - and to me that seems like just the right trade-off between convenience and giving the user enough rope.

Maybe there is a way to take it back to a thin wrapper, and provide enough hooks for the other use cases but at a minimum being able to do a perfect sync of a src to dest that can catch up on start seems in keeping with the principal least surprise.

My pull request already provides this, unless by "catch up on start" you actually mean "catch up on start with removals", in which case I think that's a bad idea for the reasons stated above (i.e. it's too opinionated and dangerous).

I like your mix'n'match proposal to exclusions.

Cool. I'll try to find time to amend the pull request to implement this. Thanks!

[aspiers]

Looks like https://github.com/pmcjury/guard-remote-sync has solved many of these issues and also appears to be maintained.