1386351 - `git mozreview push` doesn't run pre-push hooks

Status: RESOLVED FIXED

(MozReview Graveyard :: Integration: Git, enhancement)

Description

[Mark Banner (:standard8)]

Now we've got Lint hooks available (http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/install-git.html), I just noticed that when I do:

    git mozreview push

The pre-push hook doesn't get run.

This means pushes aren't aborted if I've done something wrong from a lint perspective.

Comment 1

[Mark Banner (:standard8)]

Attachment: WIP patch

This is a WIP patch that seems to mainly work, although it is really blocked by bug 1405588 as we need to know what arguments to be passing the pre-push script (if any).

Comment 2

[Mark Banner (:standard8)]

Attachment: Add calling the pre-push hook to 'git mozreview push'

Now that bug 1405588 has landed, it is much simpler to get this working, as we don't need to provide any arguments to the pre-push hook.

This adds calling the pre-push hook when doing `git mozreview push`.

If there's no hook installed, then this will give the developer a note for where to install the hook (I'm hoping to work on other bugs soon that will install the hooks during bootstrap).

One question, do you know how we get this deployed to developer's machines? Does it just land in the repository and filter out slowly, or is there some sort of trigger to update things (or something developers can do to get it quickly)?

Comment 3

[Mike Hommey [:glandium]]

Comment on attachment 8928925 [details] [diff] [review]
Add calling the pre-push hook to 'git mozreview push'

Review of attachment 8928925 [details] [diff] [review]:
-----------------------------------------------------------------

pre-push is a standard git hook, so if someone has a valid standard git pre-push hook, it's expecting command line arguments and information on the standard input.
See https://git-scm.com/docs/githooks#_pre_push

Comment 4

[Mark Banner (:standard8)]

Mike, do you have any suggestions on how we might get that data? I can see we can probably work out the local sha from the push_commit, and I guess there's a way we could get the local branch ref.

However, I can't see that we'd be likely to have a valid remote ref for the mozreview setup, as we don't have the remote branches... and if I'm reading the script write, we also won't be able to get the remote sha until after the push unless we do something complicated to detect.

Maybe we should just call tools/lint/hooks.py direct?

Comment 5

[Mike Hommey [:glandium]]

Ideally we'd just use git push, but mozreview push needs data it can't get through git push. So I guess... run the python script directly?

That said, we're switching to phabricator in less than 2 months, aren't we... this is all going to become irrelevant soon.

Comment 6

[Mark Banner (:standard8)]

(In reply to Mike Hommey [:glandium] from comment #5)
> That said, we're switching to phabricator in less than 2 months, aren't
> we... this is all going to become irrelevant soon.

Yes, hopefully. I'd still prefer to fix this now though, and encourage more people to use these hooks. We should be able to catch these before they get to phabricator, earlier is generally better for developers.

Comment 7

[Mark Banner (:standard8)]

Attachment: Make 'git mozreview push' run the Lint hook before allowing a push.

Updated patch to call the lint hook direct. This will need a slight modification to the lint hook itself that I'll attach in a second patch.

Comment 8

[Mark Banner (:standard8)]

Attachment: Let the Lint hook default to pre-push (for git) if being called directly.

This makes it so the lint hook defaults to pre-push when being run directly and within git.

Comment 9

[Mike Hommey [:glandium]]

Comment on attachment 8930426 [details] [diff] [review]
Make 'git mozreview push' run the Lint hook before allowing a push.

Review of attachment 8930426 [details] [diff] [review]:
-----------------------------------------------------------------

::: git/commands/git-mozreview
@@ +335,5 @@
>                           'repo at %s' % ROOT)
>  
> +    # Ensure Lint is run and passes
> +    if not run_lint(remote_name):
> +        raise AbortError('Lint run failed')

So one thing I didn't think through when I said running the hook directly would work is that the advantage of a hook is that it can be disabled. This can't. So if for some reason the hook doesn't work properly on a developer's machine, they have no recourse...

@@ +574,5 @@
> +    source_hook = os.path.join(source_dir, 'tools', 'lint', 'hooks.py')
> +
> +    if not os.path.exists(source_hook):
> +        ui = gethgui()
> +        ui.write('Warning: Could not find the Lint hook source at %s\n' % source_hook)

This warning is going to be useless noise for people using git mozreview for non-gecko trees. If the file doesn't exist, can't we just assume it's not a gecko tree and not say anything?

Comment 10

[Mark Banner (:standard8)]

Attachment: Make 'git mozreview push' run the Lint hook before allowing a push. v2

Thanks for the comments, I've adjusted the patch for them both.

Comment 11

[Mike Hommey [:glandium]]

Comment on attachment 8930967 [details] [diff] [review]
Make 'git mozreview push' run the Lint hook before allowing a push. v2

Review of attachment 8930967 [details] [diff] [review]:
-----------------------------------------------------------------

::: git/commands/git-mozreview
@@ +335,5 @@
>                           'repo at %s' % ROOT)
>  
> +    # Ensure Lint is run and passes
> +    if not args.skip_lint and not run_lint(remote_name):
> +        raise AbortError('Lint run failed')

Maybe add a sentence hinting that --skip-lint exists?

@@ +570,5 @@
>                    'can see them)\n')
>  
> +
> +def run_lint(remote_name):
> +    source_hook = os.path.join(source_dir, 'tools', 'lint', 'hooks.py')

It would be better to get source_dir here, so that you don't run one more git command when it's not needed.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b0b3aa75e9b
Let the Lint hook default to pre-push (for git) if being called directly. r=glandium

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/32fc19893ab5
Make 'git mozreview push' run the Lint hook before allowing a push. r=glandium

Comment 14

[Mark Banner (:standard8)]

Leaving open until the inbound part lands.

Comment 15

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/6b0b3aa75e9b

Comment 16

[Mark Banner (:standard8)]

Note to watchers: This has been landed in the relevant places, you won't get it until version-control-tools updates the bookmark (I'll find out who does that next week), and you run ./mach mercurial-setup.

However, if you do want to try it earlier, you can go into ~/.mozbuild/version-control-tools and pull and update to default (rather than the @ bookmark).

Comment 17

[Mike Hommey [:glandium]]

(In reply to Mark Banner (:standard8) from comment #16)
> Note to watchers: This has been landed in the relevant places, you won't get
> it until version-control-tools updates the bookmark (I'll find out who does
> that next week)

It's whoever pushes. This suggests you weren't on the @ bookmark locally when you pushed.

Comment 18

[Mark Banner (:standard8)]

The @ bookmark has now been updated.