Page MenuHomePhabricator

Remove support for arc commit hooks
Closed, ResolvedPublic

Description

Arcanist has server-side commit hook modes:

  • arc svn-hook-pre-commit
  • arc git-hook-pre-receive
  • (No Mercurial implementation.)

These hooks were useful at Facebook to execute lint checks in order to block syntax errors, but they don't work very well in the general case.

In particular, a major issue is that there is no working copy when running in this mode, so lint checks need to execute in an alternate mode which takes all data over stdin. However:

  • A lot of linters can't do this.
  • A lot of linters have different behavior when taking data over stdin vs in files.
  • A lot of linters have configuration files on disk.
  • This mode is difficult to anticipate/understand.
  • This mode is difficult to test.
  • This mode is difficult to debug.

The net effect here is that these hook modes don't work for many linters, and make the development of all linters harder. We've also seen little-to-no interest in running these hooks (and we've never received requests to develop a Mercurial hook, or to integrate these hooks with hosted repositories).

In most cases, running these checks during arc land is probably better: although it has some drawbacks and we'll need to add a bit of configuration, there's far less magic to it, and it's easy for users to understand and for developers to test (see T5064). And, ultimately, CI should be the backstop for these checks.

Event Timeline

epriestley raised the priority of this task from to Normal.
epriestley updated the task description. (Show Details)
epriestley added projects: Arcanist, Lint.
epriestley added a subscriber: epriestley.
epriestley merged a task: Restricted Maniphest Task.Mar 27 2015, 12:34 PM
epriestley added subscribers: alokdhir, moos3, svemir and 2 others.

I think it would be reasonable to leave getCommitHookMode() around as a stub which just emits a deprecation warning and does return false; -- I believe none of the other changed APIs are likely to be used by third-party stuff which isn't materially affected by the changes.

Yeah that sounds reasonable.

Ah, nice. Happy to accept a diff to stub out getCommitHookMode() for a few months if anyone still has a use case.