Morbo does not ignore .swp files anymore

[arthoq]

I have just updated Mojolicious and now Morbo does not ignore the .swp files created by Vim anymore. Is this a bug?

[arthoq]

Is this the wrong place to ask?

[evoyy]

I just upgraded 6.41 -> 6.55 and found the same issue. Morbo is restarting whenever I type anything. I have reverted back to 6.41 for now.

[jhthorsen]

We know about the issue. It can be fixed with this patch:

diff --git a/lib/Mojo/Server/Morbo.pm b/lib/Mojo/Server/Morbo.pm
index 9217ea9..873e86e 100644
--- a/lib/Mojo/Server/Morbo.pm
+++ b/lib/Mojo/Server/Morbo.pm
@@ -4,6 +4,7 @@ use Mojo::Base -base;
 # "Linda: With Haley's Comet out of ice, Earth is experiencing the devastating
 #         effects of sudden, intense global warming.
 #  Morbo: Morbo is pleased but sticky."
+use File::Basename 'basename';
 use Mojo::Server::Daemon;
 use Mojo::Util qw(deprecated files);
 use POSIX 'WNOHANG';
@@ -24,6 +25,7 @@ sub modified_files {
   my $cache = $self->{cache} ||= {};
   my @files;
   for my $file (map { -f $_ && -r _ ? $_ : files $_ } @{$self->watch}) {
+    next if basename($file) =~ /^\./;
     my ($size, $mtime) = (stat $file)[7, 9];
     my $stats = $cache->{$file} ||= [$^T, $size];
     next if $mtime <= $stats->[0] && $size == $stats->[1];
diff --git a/t/mojo/morbo.t b/t/mojo/morbo.t
index ed53db8..c213afa 100644
--- a/t/mojo/morbo.t
+++ b/t/mojo/morbo.t
@@ -116,9 +116,10 @@ is $tx->res->body, 'Hello!', 'right content';

 # New file(s)
 is_deeply $morbo->modified_files, [], 'directory has not changed';
-my @new = map { catfile $subdir, "$_.txt" } qw/test testing/;
+my @new = map { catfile $subdir, "$_.txt" } qw(.skip.swp test testing);
 spurt 'whatever', $_ for @new;
-is_deeply $morbo->modified_files, \@new, 'two files have changed';
+is_deeply $morbo->modified_files, [grep { $_ !~ /skip\.swp/ } @new],
+  'two files have changed';
 is_deeply $morbo->modified_files, [], 'directory has not changed again';

Need to decide if this is the right thing to do.

[evoyy]

The patch is working, thanks.

It looks like a nice solution to me. I don't think anybody puts application code in dotfiles. But if you want to be conservative you could enable it via a --ignore-dotfiles switch.

[arthoq]

Yes, the patch works.

[jberger]

👍 for @jhthorsen 's patch as long as it includes some documentation somewhere

[pink-mist]

👍 from me too

[kraih]

You don't think it might be problematic that this can't handle directories like .git? Is it better for performance to check the prefix before or after mtime/size?

[jberger]

hmmmm, yeah, handling .git would probably be wise though it is less important than swp since you have to save and commit before the .git directory is changed

[jhthorsen]

I supposed stat() is a lot slower than calling a perl function.

What do you mean with "can't handle directories like .git" @kraih ? Do you mean --watch .git or --watch . ? If so, I imagine that would be slow either way. At least this patch avoids watching temp files while editing.

[jhthorsen]

As @jberger pointed out in #mojo earlier, here is what plack does: https://metacpan.org/source/MIYAGAWA/Plack-1.0039/lib/Plack/Loader/Restarter.pm#L55

I think something like this is a middle way:

next if basename($file) =~ m!\.(?:bak|swp|swpx|swx)$|~$|_flymake\.p[lm]$|\.#!;

But it requires more tests, hehe

[kraih]

@jhthorsen I mean it doesn't ignore files inside a directory with dot prefix, like .git/config. And that regex looks horrible to me. 😱

[jberger]

@jhthorsen makes a point about .git, you'd have to explicitly watch that directory for changes (or else watch .). Regarding the Plack regex, I said at the time that that didn't look very Mojo to me 😄

[jhthorsen]

Yeah, I think the regex is a mess as well. Let's not make it more complicated than we have to, and currently users are complaining about dotfiles.

I'm +1 on the patch, since it's simple and makes morbo behave like it used before ea8dc33#diff-52d3ad944c9afec616d8ccbcc2f1ce2eL48

[jberger]

I would be interested some performance metric if possible.

[kraih]

@jhthorsen I don't think that is true, it looks like the old code used to ignore directories with a dot prefix too.

[kraih]

Note that i'm not saying it should ignore directories like .git, just asking the question if it might be problematic not to do so. If users actually do -w ., then that's possibly thousands of additional files to check every second.

[kraih]

Quick benchmark for the Mojolicious repo on OS X with SSD.

~/repo/mojo $ perl -MMojo::Server::Morbo -Mojo -E 'my $m = Mojo::Server::Morbo->new(watch => ["."]); n { $m->modified_files } 10'
1.66039 wallclock secs ( 0.48 usr +  1.18 sys =  1.66 CPU) @  6.02/s (n=10)
~/repo/mojo $ find . | wc -l
    5048

[kraih]

Almost forgot, these are the results with @jhthorsen's patch applied. So there's a very noticeable performance cost.

~/repo/mojo $ perl -Ilib -MMojo::Server::Morbo -Mojo -E 'my $m = Mojo::Server::Morbo->new(watch => ["."]); n { $m->modified_files } 10'
2.49952 wallclock secs ( 1.30 usr +  1.20 sys =  2.50 CPU) @  4.00/s (n=10)

But if the dot prefix check gets moved behind the mtime/size check, it jumps back to 6.02/s. I strongly recommend you optimize that part.

[jhthorsen]

New patch:

diff --git a/lib/Mojo/Server/Morbo.pm b/lib/Mojo/Server/Morbo.pm
index 9217ea9..879a925 100644
--- a/lib/Mojo/Server/Morbo.pm
+++ b/lib/Mojo/Server/Morbo.pm
@@ -5,7 +5,9 @@ use Mojo::Base -base;
 #         effects of sudden, intense global warming.
 #  Morbo: Morbo is pleased but sticky."
 use Mojo::Server::Daemon;
-use Mojo::Util qw(deprecated files);
+use Mojo::Util qw(deprecated);
+use File::Find ();
+use File::Spec::Functions 'splitpath';
 use POSIX 'WNOHANG';

 has daemon => sub { Mojo::Server::Daemon->new };
@@ -20,17 +22,21 @@ sub check {

 sub modified_files {
   my $self = shift;
-
   my $cache = $self->{cache} ||= {};
   my @files;
-  for my $file (map { -f $_ && -r _ ? $_ : files $_ } @{$self->watch}) {
-    my ($size, $mtime) = (stat $file)[7, 9];
-    my $stats = $cache->{$file} ||= [$^T, $size];
-    next if $mtime <= $stats->[0] && $size == $stats->[1];
-    @$stats = ($mtime, $size);
-    push @files, $file;
-  }

+  my $wanted = sub {
+    return if -d $_;
+    my ($size, $mtime) = (stat _)[7, 9];
+    my $prev = $cache->{$File::Find::name} ||= [$^T, $size];
+    return if $mtime <= $prev->[0] && $size == $prev->[1];
+    return if (splitpath $File::Find::name)[2] =~ /^\./;
+    @$prev = ($mtime, $size);
+    push @files, $File::Find::name;
+  };
+
+  no warnings;    # skip "Can't stat" warnings
+  File::Find::find({no_chdir => 1, wanted => $wanted}, $_) for @{$self->watch};
   return \@files;
 }

Benchmark on 5163 files with n=50:

• with patch: 1.2915s
• without patch: 1.74053s

Here is more information from the discussion on IRC: http://irclog.perlgeek.de/mojo/2016-03-14#i_12180034

[kraih]

Just wanted to mention that @jhthorsen's latest patch should also result in the deprecation of Mojo::Util::files if it gets chosen.

[kraih]

This should really be two issues, the first about ignoring dotfiles efficiently with the current code, and the second about optimizing Morbo by deprecating Mojo::Util::files.

[aanoaa]

Is there no way to ignore file via pattern?
like lib/Foo/Controller/Bar_flymake.pm

is this wrong place to ask?