( )⚙ D894 fsmonitor: warn when fsmonitor could be used

This is an archive of the discontinued Mercurial Phabricator instance.

fsmonitor: warn when fsmonitor could be used
ClosedPublic

Authored by indygreg on Oct 1 2017, 6:49 PM.

Details

Summary

fsmonitor can significantly speed up operations on large working
directories. But fsmonitor isn't enabled by default, so naive users
may not realize there is a potential to make Mercurial faster.

This commit introduces a warning to working directory updates when
fsmonitor could be used.

The following conditions must be met:

  • Working directory is previously empty
  • New working directory adds >= N files (currently 50,000)
  • Running on Linux or MacOS
  • fsmonitor not enabled
  • Warning not disabled via config override

Because of the empty working directory restriction, most users will
only see this warning during hg clone (assuming very few users
actually do an hg up null).

The addition of a warning may be considered a BC change. However, clone
has printed warnings before. Until recently, Mercurial printed a warning
with the server's certificate fingerprint when it wasn't explicitly
trusted for example. The warning goes to stderr. So it shouldn't
interfere with scripts parsing meaningful output.

The OS restriction was on the advice of Facebook engineers, who only
feel confident with watchman's stability on the supported platforms.

.. feature::

Print warning when fsmonitor isn't being used on a large repository

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

indygreg created this revision.Oct 1 2017, 6:49 PM
mbthomas accepted this revision.Oct 2 2017, 4:32 PM
mbthomas added a subscriber: mbthomas.

LGTM

mercurial/merge.py
1969–1973

I'm not sure if we're planning to work towards PEP8, but it (and I) prefer using parentheses to break long if conditionals, rather than backslashes.

krbullock requested changes to this revision.Oct 4 2017, 10:52 AM
krbullock added a subscriber: krbullock.
krbullock added inline comments.
mercurial/merge.py
1969–1973

It's been our style basically forever to use parens instead of backslashes. I'm honestly shocked we don't have a check-code rule for this.

tests/test-fsmonitor-warning.t
9 ↗(On Diff #2315)

We can't squeeze these into a test that's already set up? Brand-new test cases are discouraged, since so much time in our test suite is already burned on initializing repos.

This revision now requires changes to proceed.Oct 4 2017, 10:52 AM
quark added a subscriber: quark.Oct 17 2017, 7:40 PM
quark added inline comments.
tests/test-fsmonitor-warning.t
9 ↗(On Diff #2315)

There was no fsmonitor related tests. So I think this is fine.

durin42 requested changes to this revision.Oct 18 2017, 4:11 PM
durin42 added a subscriber: durin42.
durin42 added inline comments.
mercurial/merge.py
1969–1973

Yes, definitely use parens and not \.

tests/test-fsmonitor-warning.t
9 ↗(On Diff #2315)

You could easily fold this into any number of existing tests - really all you need is a repo with N files, and the ability to clone twice in a #if block.

indygreg updated this revision to Diff 2998.Oct 18 2017, 4:58 PM
durin42 accepted this revision.Oct 18 2017, 5:01 PM
This revision was automatically updated to reflect the committed changes.