Watching Deleted/Moved files. #136

Merged
merged 16 commits into from Sep 27, 2011

Conversation

Projects
None yet
4 participants
@limeyd
Contributor

limeyd commented Sep 12, 2011

Hey thibaudgg here's my work on watching deleted/moved files.
D.

@thibaudgg

This comment has been minimized.

Show comment Hide comment
@thibaudgg

thibaudgg Aug 24, 2011

populate the sha1 hash could take a very long time on big folder :|

populate the sha1 hash could take a very long time on big folder :|

This comment has been minimized.

Show comment Hide comment
@limeyd

limeyd Aug 24, 2011

Owner
Owner

limeyd replied Aug 24, 2011

This comment has been minimized.

Show comment Hide comment
@thibaudgg

thibaudgg Aug 24, 2011

yeah, keeping track of path for existence for moved/deleted files seems a better and lighter approach than sha1 hash.

yeah, keeping track of path for existence for moved/deleted files seems a better and lighter approach than sha1 hash.

@thibaudgg

This comment has been minimized.

Show comment Hide comment
@thibaudgg

thibaudgg Sep 12, 2011

Owner

This pull request cannot be automatically merged. Please can you merge your fork with the master before I merged it. Thanks

Owner

thibaudgg commented Sep 12, 2011

This pull request cannot be automatically merged. Please can you merge your fork with the master before I merged it. Thanks

@rymai

This comment has been minimized.

Show comment Hide comment
@rymai

rymai Sep 12, 2011

Owner

And what about the specs? :)

Owner

rymai commented Sep 12, 2011

And what about the specs? :)

lib/guard.rb
+ deletions = paths.collect { |f| f.slice(1..-1) if f.start_with?('!') }.compact
+ end
+
+ unless deletions.empty?

This comment has been minimized.

Show comment Hide comment
@rymai

rymai Sep 12, 2011

Owner

deletions might be undefined here since it's created inside the unless block above. I suggest moving this unless inside the unless above.

@rymai

rymai Sep 12, 2011

Owner

deletions might be undefined here since it's created inside the unless block above. I suggest moving this unless inside the unless above.

@thibaudgg

This comment has been minimized.

Show comment Hide comment
@thibaudgg

thibaudgg Sep 12, 2011

Owner

Oh yeah the specs and the README note :)

Owner

thibaudgg commented Sep 12, 2011

Oh yeah the specs and the README note :)

limeyd added some commits Sep 13, 2011

moved timestamp hash creation into it's own method add initial specs …
…for watching deleted files and fixed some minor formatting
Merge remote branch 'upstream/master'
Conflicts:
	lib/guard.rb
	lib/guard/listener.rb
	spec/guard/listener_spec.rb
@limeyd

This comment has been minimized.

Show comment Hide comment
@limeyd

limeyd Sep 13, 2011

Contributor

So I've done the merge but I'm getting to FAILS on specs see the following gist https://gist.github.com/e887abe38bcd35081a35

Contributor

limeyd commented Sep 13, 2011

So I've done the merge but I'm getting to FAILS on specs see the following gist https://gist.github.com/e887abe38bcd35081a35

@limeyd

This comment has been minimized.

Show comment Hide comment
@limeyd

limeyd Sep 14, 2011

Contributor

all specs are passing now.

Contributor

limeyd commented Sep 14, 2011

all specs are passing now.

@thibaudgg

This comment has been minimized.

Show comment Hide comment
@thibaudgg

thibaudgg Sep 14, 2011

It would be great to found an option name that also include the moved files, don't you think? Maybe something like -- timestamps-tracking

It would be great to found an option name that also include the moved files, don't you think? Maybe something like -- timestamps-tracking

This comment has been minimized.

Show comment Hide comment
@limeyd

limeyd Sep 14, 2011

Owner

see latest commit --watch-moves-deletions

Owner

limeyd replied Sep 14, 2011

see latest commit --watch-moves-deletions

@thibaudgg

This comment has been minimized.

Show comment Hide comment
@thibaudgg

thibaudgg Sep 14, 2011

Owner

It seems there's still two issues with the jruby specs on Travis :( (Have you an idea why?) http://travis-ci.org/#!/limeyd/guard

Owner

thibaudgg commented Sep 14, 2011

It seems there's still two issues with the jruby specs on Travis :( (Have you an idea why?) http://travis-ci.org/#!/limeyd/guard

README.md
@@ -213,6 +213,15 @@ $ guard --guardfile ~/.your_global_guardfile
$ guard -G ~/.your_global_guardfile # shortcut
```
+### `-D`/`--deletions` option

This comment has been minimized.

Show comment Hide comment
@rymai

rymai Sep 14, 2011

Owner

Maybe --watch-deletions is more appropriate/explicit?

@rymai

rymai Sep 14, 2011

Owner

Maybe --watch-deletions is more appropriate/explicit?

This comment has been minimized.

Show comment Hide comment
@limeyd

limeyd Sep 14, 2011

Contributor

That's a fair comment i'l make the change.
D.

On Wed, Sep 14, 2011 at 2:32 AM, Rmy Coutable <
reply@reply.github.com>wrote:

@@ -213,6 +213,15 @@ $ guard --guardfile ~/.your_global_guardfile
$ guard -G ~/.your_global_guardfile # shortcut


+### `-D`/`--deletions` option

Maybe --watch-deletions is more appropriate/explicit?

Reply to this email directly or view it on GitHub:
https://github.com/guard/guard/pull/136/files#r124392

darren pearce | interactive designer | o:403.475.4545 | m:403.630.6258

@limeyd

limeyd Sep 14, 2011

Contributor

That's a fair comment i'l make the change.
D.

On Wed, Sep 14, 2011 at 2:32 AM, Rmy Coutable <
reply@reply.github.com>wrote:

@@ -213,6 +213,15 @@ $ guard --guardfile ~/.your_global_guardfile
$ guard -G ~/.your_global_guardfile # shortcut


+### `-D`/`--deletions` option

Maybe --watch-deletions is more appropriate/explicit?

Reply to this email directly or view it on GitHub:
https://github.com/guard/guard/pull/136/files#r124392

darren pearce | interactive designer | o:403.475.4545 | m:403.630.6258

@limeyd

This comment has been minimized.

Show comment Hide comment
@limeyd

limeyd Sep 14, 2011

Contributor

I'll take a look.
d.

On Wed, Sep 14, 2011 at 12:44 AM, Thibaud Guillaume-Gentil <
reply@reply.github.com>wrote:

It seems there's still two issues with the jruby specs on Travis :( (Have
you an idea why?) http://travis-ci.org/#!/limeyd/guard

Reply to this email directly or view it on GitHub:
#136 (comment)

darren pearce | interactive designer | o:403.475.4545 | m:403.630.6258

Contributor

limeyd commented Sep 14, 2011

I'll take a look.
d.

On Wed, Sep 14, 2011 at 12:44 AM, Thibaud Guillaume-Gentil <
reply@reply.github.com>wrote:

It seems there's still two issues with the jruby specs on Travis :( (Have
you an idea why?) http://travis-ci.org/#!/limeyd/guard

Reply to this email directly or view it on GitHub:
#136 (comment)

darren pearce | interactive designer | o:403.475.4545 | m:403.630.6258

@limeyd

This comment has been minimized.

Show comment Hide comment
@limeyd

limeyd Sep 14, 2011

Contributor

as for the option name how about this for explict --watch-moves-deletions

Contributor

limeyd commented Sep 14, 2011

as for the option name how about this for explict --watch-moves-deletions

@rymai

This comment has been minimized.

Show comment Hide comment
@rymai

rymai Sep 14, 2011

Owner

Perfect for me !

Sent from my iPhone

On 14 sept. 2011, at 19:10, Darren Pearce reply@reply.github.com wrote:

as for the option name how about this for explict --watch-moves-deletions

Reply to this email directly or view it on GitHub:
#136 (comment)

Owner

rymai commented Sep 14, 2011

Perfect for me !

Sent from my iPhone

On 14 sept. 2011, at 19:10, Darren Pearce reply@reply.github.com wrote:

as for the option name how about this for explict --watch-moves-deletions

Reply to this email directly or view it on GitHub:
#136 (comment)

@thibaudgg

This comment has been minimized.

Show comment Hide comment
@thibaudgg

thibaudgg Sep 14, 2011

Owner

Maybe --watch-moves-and-deletions would be even more precise?

Owner

thibaudgg commented Sep 14, 2011

Maybe --watch-moves-and-deletions would be even more precise?

@limeyd

This comment has been minimized.

Show comment Hide comment
@limeyd

limeyd Sep 14, 2011

Contributor

That would be for sure :) and I'd be happy to change it as it does read
better.
d.

On Wed, Sep 14, 2011 at 1:20 PM, Thibaud Guillaume-Gentil <
reply@reply.github.com>wrote:

Maybe --watch-moves-and-deletions would be even more precise?

Reply to this email directly or view it on GitHub:
#136 (comment)

darren pearce | interactive designer | o:403.475.4545 | m:403.630.6258

Contributor

limeyd commented Sep 14, 2011

That would be for sure :) and I'd be happy to change it as it does read
better.
d.

On Wed, Sep 14, 2011 at 1:20 PM, Thibaud Guillaume-Gentil <
reply@reply.github.com>wrote:

Maybe --watch-moves-and-deletions would be even more precise?

Reply to this email directly or view it on GitHub:
#136 (comment)

darren pearce | interactive designer | o:403.475.4545 | m:403.630.6258

@thibaudgg

This comment has been minimized.

Show comment Hide comment
@thibaudgg

thibaudgg Sep 15, 2011

Owner

Yep, I think we can go for --watch-moves-and-deletions and use the -W alias rather than -D to not be focus only on the deleted files.

Owner

thibaudgg commented Sep 15, 2011

Yep, I think we can go for --watch-moves-and-deletions and use the -W alias rather than -D to not be focus only on the deleted files.

@limeyd

This comment has been minimized.

Show comment Hide comment
@limeyd

limeyd Sep 15, 2011

Contributor

How about --watch-all-modifications and then use -A or -M as -w is
used for watchdir
thoughts.

On Thu, Sep 15, 2011 at 1:15 AM, Thibaud Guillaume-Gentil <
reply@reply.github.com>wrote:

Yep, I think we can go for --watch-moves-and-deletions and use the -W
alias rather than -D to not be focus only on the deleted files.

Reply to this email directly or view it on GitHub:
#136 (comment)

darren pearce | interactive designer | o:403.475.4545 | m:403.630.6258

Contributor

limeyd commented Sep 15, 2011

How about --watch-all-modifications and then use -A or -M as -w is
used for watchdir
thoughts.

On Thu, Sep 15, 2011 at 1:15 AM, Thibaud Guillaume-Gentil <
reply@reply.github.com>wrote:

Yep, I think we can go for --watch-moves-and-deletions and use the -W
alias rather than -D to not be focus only on the deleted files.

Reply to this email directly or view it on GitHub:
#136 (comment)

darren pearce | interactive designer | o:403.475.4545 | m:403.630.6258

@rymai

This comment has been minimized.

Show comment Hide comment
@rymai

rymai Sep 15, 2011

Owner

Yeah you're right, it's way better IMO! -A seems good for it too. Let's wait for @thibaudgg's opinion!

Owner

rymai commented Sep 15, 2011

Yeah you're right, it's way better IMO! -A seems good for it too. Let's wait for @thibaudgg's opinion!

@thibaudgg

This comment has been minimized.

Show comment Hide comment
@thibaudgg

thibaudgg Sep 16, 2011

Owner

Yeah, --watch-all-modifications with -A is even better! Go for it!

Owner

thibaudgg commented Sep 16, 2011

Yeah, --watch-all-modifications with -A is even better! Go for it!

limeyd added some commits Sep 21, 2011

changed watch deletions option to watch_all_modifiactions, Merge bran…
…ch 'master' of git://github.com/guard/guard

Conflicts:
	lib/guard.rb
	lib/guard/cli.rb
	lib/guard/listener.rb
@limeyd

This comment has been minimized.

Show comment Hide comment
@limeyd

limeyd Sep 21, 2011

Contributor

done and done and all code is merged.

Contributor

limeyd commented Sep 21, 2011

done and done and all code is merged.

@thibaudgg

This comment has been minimized.

Show comment Hide comment
@thibaudgg

thibaudgg Sep 22, 2011

Owner

Ok great thanks, it's almost ready to be merged! There's just this 2 issues on jruby, any idea on how to fix it?

Owner

thibaudgg commented Sep 22, 2011

Ok great thanks, it's almost ready to be merged! There's just this 2 issues on jruby, any idea on how to fix it?

@limeyd

This comment has been minimized.

Show comment Hide comment
@limeyd

limeyd Sep 22, 2011

Contributor

I'm looking as it's passing locally using jruby1.6.4 on osx lion

Contributor

limeyd commented Sep 22, 2011

I'm looking as it's passing locally using jruby1.6.4 on osx lion

@thibaudgg

This comment has been minimized.

Show comment Hide comment
@thibaudgg

thibaudgg Sep 22, 2011

Owner

@netzpirat have you an idea about jruby falling (again!) on linux but not on osx

Owner

thibaudgg commented Sep 22, 2011

@netzpirat have you an idea about jruby falling (again!) on linux but not on osx

@netzpirat

This comment has been minimized.

Show comment Hide comment
@netzpirat

netzpirat Sep 22, 2011

Contributor

When I analysed the Guard spec suite on Travis for JRuby, I spent most of the time to understand the ctime bug on JRuby. All other specs were fixed by giving more time before evaluate the result (Just add another sleep or increase the wait time for an exisiting sleep). I believe that Travis has very bad file IO and also CPU is not that fast. This leads to exactly the problem that the specs are passing locally and fails on Travis.

Contributor

netzpirat commented Sep 22, 2011

When I analysed the Guard spec suite on Travis for JRuby, I spent most of the time to understand the ctime bug on JRuby. All other specs were fixed by giving more time before evaluate the result (Just add another sleep or increase the wait time for an exisiting sleep). I believe that Travis has very bad file IO and also CPU is not that fast. This leads to exactly the problem that the specs are passing locally and fails on Travis.

@netzpirat

This comment has been minimized.

Show comment Hide comment
@netzpirat

netzpirat Sep 22, 2011

Contributor

I'm feeling a bit sick right now, so this is the reason why I try to avoid coding, have a good rest and become healthy again. When I feel ok on Monday, I could give it a shot.

Contributor

netzpirat commented Sep 22, 2011

I'm feeling a bit sick right now, so this is the reason why I try to avoid coding, have a good rest and become healthy again. When I feel ok on Monday, I could give it a shot.

@thibaudgg

This comment has been minimized.

Show comment Hide comment
@thibaudgg

thibaudgg Sep 23, 2011

Owner

Ok thanks @netzpirat, take care of yourself!

Owner

thibaudgg commented Sep 23, 2011

Ok thanks @netzpirat, take care of yourself!

@limeyd

This comment has been minimized.

Show comment Hide comment
@limeyd

limeyd Sep 23, 2011

Contributor

I tried reducing the sleep locally and it did raise the same errors as per jruby on travis so the question is we increase the sleep for those failing specs?

Contributor

limeyd commented Sep 23, 2011

I tried reducing the sleep locally and it did raise the same errors as per jruby on travis so the question is we increase the sleep for those failing specs?

@netzpirat

This comment has been minimized.

Show comment Hide comment
@netzpirat

netzpirat Sep 24, 2011

Contributor

I just refactored the listener spec sleep time to be configurable through an environment variable and set it to 2 seconds on Travis. Can you please merge these changes, so we see if this generally solves some timing problems we have with the specs on Travis.

Contributor

netzpirat commented Sep 24, 2011

I just refactored the listener spec sleep time to be configurable through an environment variable and set it to 2 seconds on Travis. Can you please merge these changes, so we see if this generally solves some timing problems we have with the specs on Travis.

@netzpirat

This comment has been minimized.

Show comment Hide comment
@netzpirat

netzpirat Sep 24, 2011

Contributor

Forget about the previous comment, there is definitely something wrong with our specs: After I increased the sleep time on Travis, more specs have failed on other Rubies as well. This can be reproduced locally by setting a longer sleep time:

export GUARD_SLEEP=3
rake spec:portability

From my understanding, increasing the sleep time should not have an influence on the outcome of the specs, but it does. I have to spend some more time to dig into the code to better understand why this happens. But this is not your problem, it's a problem of the Guard specs, so we should take care of it.

I suggest that you just update your branch with the latest changes from master (especially the YARD-docs, so please think about it if something special should be documented), and then we'll merge you branch into master.

I will spend some time next week on the spec suite and see what I can do.

Contributor

netzpirat commented Sep 24, 2011

Forget about the previous comment, there is definitely something wrong with our specs: After I increased the sleep time on Travis, more specs have failed on other Rubies as well. This can be reproduced locally by setting a longer sleep time:

export GUARD_SLEEP=3
rake spec:portability

From my understanding, increasing the sleep time should not have an influence on the outcome of the specs, but it does. I have to spend some more time to dig into the code to better understand why this happens. But this is not your problem, it's a problem of the Guard specs, so we should take care of it.

I suggest that you just update your branch with the latest changes from master (especially the YARD-docs, so please think about it if something special should be documented), and then we'll merge you branch into master.

I will spend some time next week on the spec suite and see what I can do.

lib/guard.rb
@@ -24,14 +24,15 @@ module Guard
# @option options [Array<String>] group the list of groups to start
# @option options [String] watchdir the director to watch
# @option options [String] guardfile the path to the Guardfile
- #
+ # @optuin options [Boolean] watch_all_modifications watches all file modifications if true

This comment has been minimized.

Show comment Hide comment
@rymai

rymai Sep 26, 2011

Owner

Typo: @optuin => @option :)

@rymai

rymai Sep 26, 2011

Owner

Typo: @optuin => @option :)

lib/guard.rb
def setup(options = {})
@options = options
@guards = []
@groups = [:default]
@interactor = Interactor.new
- @listener = Listener.select_and_init(@options[:watchdir] ? File.expand_path(@options[:watchdir]) : Dir.pwd)
+ @listener = Listener.select_and_init(@options[:watchdir] ? File.expand_path(@options[:watchdir]) : Dir.pwd,options)

This comment has been minimized.

Show comment Hide comment
@rymai

rymai Sep 26, 2011

Owner

There should be a space before options here.

@rymai

rymai Sep 26, 2011

Owner

There should be a space before options here.

lib/guard/listener.rb
@@ -158,7 +172,8 @@ module Guard
def relativize_paths(paths)
return paths unless relativize_paths?
paths.map do |path|
- path.gsub(%r{^#{ @directory }/}, '')
+ path.gsub(%r{#{@directory}/}, '')
+ #path.gsub(%r{^#{ @directory }/}, '')

This comment has been minimized.

Show comment Hide comment
@rymai

rymai Sep 26, 2011

Owner

You should probably remove this commented line?

@rymai

rymai Sep 26, 2011

Owner

You should probably remove this commented line?

This comment has been minimized.

Show comment Hide comment
@limeyd

limeyd Sep 26, 2011

Contributor

I was thinking of changing it to gsub(%r{^(!)?/home/limeyd/},'\1') so we can capture the option ! and still keep the path rooted

@limeyd

limeyd Sep 26, 2011

Contributor

I was thinking of changing it to gsub(%r{^(!)?/home/limeyd/},'\1') so we can capture the option ! and still keep the path rooted

@limeyd

This comment has been minimized.

Show comment Hide comment
@limeyd

limeyd Sep 26, 2011

Contributor

Ok everything is merge, I had a conflict with run_on_change so code was moved to execute_supervised_task_for_all_guards, please review.

Contributor

limeyd commented Sep 26, 2011

Ok everything is merge, I had a conflict with run_on_change so code was moved to execute_supervised_task_for_all_guards, please review.

@rymai

This comment has been minimized.

Show comment Hide comment
@rymai

rymai Sep 26, 2011

Owner

Looks good to me now. We will wait for the specs to pass though, maybe wait for @netzpirat see if he has any idea on how to solve this...

Owner

rymai commented Sep 26, 2011

Looks good to me now. We will wait for the specs to pass though, maybe wait for @netzpirat see if he has any idea on how to solve this...

- paths = Watcher.match_files(guard, files)
- UI.debug "#{guard.class.name}##{task} with #{paths.inspect}"
- supervised_task(guard, task, paths)
+ paths = Watcher.match_files(guard, files)

This comment has been minimized.

Show comment Hide comment
@rymai

rymai Sep 26, 2011

Owner

Could you fix the indentation here (until line 209)? Thanks!

@rymai

rymai Sep 26, 2011

Owner

Could you fix the indentation here (until line 209)? Thanks!

@netzpirat netzpirat merged commit 87656c2 into guard:master Sep 27, 2011

@netzpirat

This comment has been minimized.

Show comment Hide comment
@netzpirat

netzpirat Sep 27, 2011

Contributor

I've merge it into the guard/run_on_deletion branch and not into the master branch, as GitHub wrongly reports here. Next is fixing the specs on Travis before merging to master...

Contributor

netzpirat commented Sep 27, 2011

I've merge it into the guard/run_on_deletion branch and not into the master branch, as GitHub wrongly reports here. Next is fixing the specs on Travis before merging to master...

@rymai

This comment has been minimized.

Show comment Hide comment
@rymai

rymai Sep 27, 2011

Owner

Interesting bug! :P Good luck Michael, and THANK YOU!

Owner

rymai commented Sep 27, 2011

Interesting bug! :P Good luck Michael, and THANK YOU!

@thibaudgg

This comment has been minimized.

Show comment Hide comment
@thibaudgg

thibaudgg Sep 28, 2011

Owner

Not sure if it's a bug, when I pull master I got the run_on_deletion branch code (in my local master)

Owner

thibaudgg commented Sep 28, 2011

Not sure if it's a bug, when I pull master I got the run_on_deletion branch code (in my local master)

@netzpirat

This comment has been minimized.

Show comment Hide comment
@netzpirat

netzpirat Sep 28, 2011

Contributor

Wow, really strange. I mean I see my commits here on GitHub on the run_on_deletion branch and not in master. What have I done?

Contributor

netzpirat commented Sep 28, 2011

Wow, really strange. I mean I see my commits here on GitHub on the run_on_deletion branch and not in master. What have I done?

@netzpirat

This comment has been minimized.

Show comment Hide comment
@netzpirat

netzpirat Sep 28, 2011

Contributor

Oh, I see! I branched after I made my commits and I have count the commits wrongly when reseting HEAD~2. How stupid is that :P

So I work today directly on master to fix JRuby on Travis.

Contributor

netzpirat commented Sep 28, 2011

Oh, I see! I branched after I made my commits and I have count the commits wrongly when reseting HEAD~2. How stupid is that :P

So I work today directly on master to fix JRuby on Travis.

@thibaudgg

This comment has been minimized.

Show comment Hide comment
@thibaudgg

thibaudgg Sep 28, 2011

Owner

Ok, no problem :)

Owner

thibaudgg commented Sep 28, 2011

Ok, no problem :)

@limeyd

This comment has been minimized.

Show comment Hide comment
@limeyd

limeyd Sep 28, 2011

Contributor

glad to see that it's been merged :) ping if you need help testing.

D.


darren pearce < interactive
t:403.475.4545, c:403.630.6258

On Wednesday, 28 September, 2011 at 4:02 AM, Thibaud Guillaume-Gentil wrote:

Ok, no problem :)

Reply to this email directly or view it on GitHub:
#136 (comment)

Contributor

limeyd commented Sep 28, 2011

glad to see that it's been merged :) ping if you need help testing.

D.


darren pearce < interactive
t:403.475.4545, c:403.630.6258

On Wednesday, 28 September, 2011 at 4:02 AM, Thibaud Guillaume-Gentil wrote:

Ok, no problem :)

Reply to this email directly or view it on GitHub:
#136 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment