( )⚙ D5586 watchman: add verbose config knob

This is an archive of the discontinued Mercurial Phabricator instance.

watchman: add verbose config knob
ClosedPublic

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

Details

Summary

This new config knob allows to silent watchman log and warning messages when
watchman is unavailable.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

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

This new config knob defaults to off and we're therefore changing the behavior to not warn when watchman is unavailable. This is a bit concerning to me because someone could have fsmonitor enabled and I don't think they'd ever get an error if watchman isn't working correctly.

I support introducing the config knob. But I don't like silently suppressing watchman errors that could lead to false user expectations.

Is my understanding here correct?

(I would accept a patch that defaults the config option to on. I will continue reviewing this series.)

pulkit added a subscriber: pulkit.Jan 17 2019, 5:17 AM

This new config knob defaults to off and we're therefore changing the behavior to not warn when watchman is unavailable. This is a bit concerning to me because someone could have fsmonitor enabled and I don't think they'd ever get an error if watchman isn't working correctly.
I support introducing the config knob. But I don't like silently suppressing watchman errors that could lead to false user expectations.

I on +1 with what @indygreg said. These warning messages have proved much helpful for us internally.

In D5586#83031, @pulkit wrote:

This new config knob defaults to off and we're therefore changing the behavior to not warn when watchman is unavailable. This is a bit concerning to me because someone could have fsmonitor enabled and I don't think they'd ever get an error if watchman isn't working correctly.
I support introducing the config knob. But I don't like silently suppressing watchman errors that could lead to false user expectations.

I on +1 with what @indygreg said. These warning messages have proved much helpful for us internally.

I will update this patch to activate it by default.

This revision was automatically updated to reflect the committed changes.