pre-push should run pre-commit checks on each commit to be pushed

[ljharb]

I'd like to configure a bunch of pre-commit checks, but disable the pre-commit hook.

Then, on pre-push, I'd like to run the precommit checks on each commit I'm about to push, one at a time.

My philosophy here is that committing should be fast, like ⌘-S - I shouldn't have to comply with anything; linting, tests, etc. However, prior to pushing, I'll rebase everything down so that each commit represents a single, atomic, conceptual change - and that's both what I want to run checks on, and also the time where I want to check it - and also the time that I'm comfortable waiting a bit longer, since pushing can take awhile anyways.

[sds]

Hey @ljharb,

Thanks for suggesting a way we can make Overcommit work better with what is likely a popular development workflow (namely: making many tiny commits to save work and squashing into atomic chunks of work). It makes sense that you would want to minimize the time waiting for hook runs in individual commits and instead have this happen on push.

You can disable all pre-commit hooks via your .overcommit.yml configuration and then add pre-push hooks to run the tools you'd like to run before push. Assuming you can rely on the exit status of the tool to determine if it passed/failed, this can be quite easy.

PreCommit:
  ALL:
    enabled: false

PrePush:
  RuboCop:
    enabled: true
    command: ['rubocop']
  ScssLint:
    enabled: true
    command: ['scss-lint']

I offer this alternative because I'm hesitant to pursue the path of providing some way to "merge" your pre-commit hooks with your pre-push hooks automatically. This would require some significant thinking given we assume only one hook context can be invoked at a time.

I understand there is a desire here to avoid re-implementing pre-push hooks from pre-commit hooks, but pre-commit hooks are fundamentally not pre-push hooks, and it seems inappropriate to me to conflate the two. Better to explicitly define your pre-push hooks as I've shown above and disable all pre-commit hooks if you're not fond of them.

I'm open to dialoging on this, but at the end of the day a pull request might be more productive in fostering further discussion, as we'll have something concrete to point to when discussing the challenges introduced by a change like this. Thanks!

[lencioni]

@sds are pre-push hooks run on just the changed files, or are they able to be run that way?

[sds]

Currently pre-push hooks have the concept of pushed_refs which allows you to see which references are about to be pushed to the upstream.

We do not yet have a concept of which files have been changed, but you could in theory calculate this from that list of references.

[lencioni]

Do pre-push hooks stash unstaged changes for you like the pre-commit ones do?

[sds]

No, pre-push hooks currently do not stash any changes to tracked files.

[stu-blair]

@ljharb , did you ever come up with a solution that works for you?

I'm also interested in running Rubocop only on pre-push and not on pre-commit, ideally just on the modified code in the new commits. I imagine someone out there has solved this, but I can't find any examples.

[ljharb]

@stu-blair no, but to be fair I haven't tried particularly hard yet with overcommit.

In general I just don't see the point in pre-commit hooks, and think all those checks should be pre-push instead - by default ideally, but at the least, configurable.

[stu-blair]

Yes, I agree. hmm

[sds]

Just a minor update here to say that pre-push hooks now support retrieving the set of modified files based on the refs being pushed as of #536.

As was already pointed out, pre-push hooks do not stash any changes to tracked files before running hooks. This is potentially problematic as it means that if we were to do an intelligent conversion of pre-commit hooks to pre-push hooks, they could run against files with changes that are not actually being pushed to the remote ref.

Supporting this would require extracting the stashing/unstashing logic from Overcommit::HookContext::PreCommit to be reused in Overcommit::HookContext::PrePush.

Happy to merge a pull request with tests adding such support!