This is an archive of the discontinued Mercurial Phabricator instance.

watchman: add the possibility to set the exact watchman binary location
AbandonedPublic

Authored by lothiraldan on Jan 15 2019, 11:57 AM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Summary

This is necessary to make rolling releases of new watchman versions across
users.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

lothiraldan created this revision.Jan 15 2019, 11:57 AM

Overall I like the feature. But the class initialization logic is wonky. That's not your fault: you happen to be wading into a mess. Since it looks like there will be a v2 on this series, I'd encourage you to add some inline comments about the __init__ mess at the least, or ideally refactor it so the code is cleaner.

hgext/fsmonitor/pywatchman/__init__.py
569–573

Why the nested __init__?

Perhaps this initialization code could be cleaned up so we are assigning an instance instead of a class to self.transport?

lothiraldan added inline comments.Jan 17 2019, 11:46 AM
hgext/fsmonitor/pywatchman/__init__.py
569–573

I see several ways to refactor the code around:

  • Pass the watchman_exe parameter to all Transport class even if they don't need it. The CLIProcessTransport is already ignoring the timeout attribute.
  • Pass a functools.partial of CLIProcessTransport.__init__.
  • Pass the client instance to the transport, which will likely results in a memory leak due to the circular reference.
  • Store an instance as client.transport but that would requires to move passing the sockpath and update the transport to manage a ready state, which would be false until they get all the needed parameters.

I have a small preference for solution 1). Solution 4) seems the most heavyweight

lothiraldan abandoned this revision.Feb 13 2019, 11:23 AM

Resent as D5954