1559446 - Git and HG lint commit hooks disagree about whether to block committing

Status: ASSIGNED

(Developer Infrastructure :: Lint and Formatting, enhancement, P3)

Description

[Michael Cooper [:mythmon]]

In Mercurial, setting up the pre-commit hooks installs a hook that prints errors, but does not prevent committing code.

In Git, the equivalent hook does block creating the commit.

This is confusing, and doesn't match the expectations of at least some developers. These should be made more consistent, or at least better documented.

Comment 1

[Michael Cooper [:mythmon]]

Attachment: Bug 1559446 - Provide a blocking version of the HG commit hook r=standard8!

Comment 2

[Mark Banner (:standard8) (afk until 21st July)]

That's strange, git doesn't block for me when creating the commit:

$ git commit -m "temp" -a
/Users/mark/dev/gecko/browser/components/search/SearchTelemetry.jsm
  64:22  error  Parsing error: Unexpected token [  (eslint)

✖ 1 problem (1 error, 0 warnings)
[D34744 78747ae7a678d] temp
 1 file changed, 1 insertion(+), 1 deletion(-)

$ ls -l .git/hooks/pre-commit
lrwxr-xr-x  1 mark  staff  25  6 Jul  2017 .git/hooks/pre-commit -> ../../tools/lint/hooks.py

We originally decided not to block on pre-commit, so that developers could still do the commits and manage their work trees even with issues. We'd only block on push (although that doesn't work at the moment due to moz-phab, bug 1491880).

Comment 3

[Michael Cooper [:mythmon]]

Oh, I think I misread the code, and made some assumptions about it. Re-reading it, you're right that it doesn't block. Sorry for the mistake.

My main concern was actually that I want the hooks to (optionally) block, because not blocking is a very surprising thing to me and some other developers I've talked to. Blocking is also how all the other hooks I've worked with outside of mozilla-central work.

Perhaps this bug should be re-summarized, or should be closed and I can file a new one.

Comment 4

[Andrew Halberstadt [:ahal]]

It would be pretty easyl to offer both blocking and non-blocking hooks and let developers choose whichever they want.

Comment 5

[Mark Banner (:standard8) (afk until 21st July)]

I think as long as we keep it status quo for now, as in non-blocking by default, I'm fine with adding blocking options.

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 7

[Suhaib Mujahid [:suhaib]]

Sorry, there was a problem with the detection of inactive users. I'm reverting the change.