Skip to content

Conversation

@proinsias
Copy link
Contributor

#479 added the pre-push hook for git-lfs.

This PR adds the additional hooks that are now available with git-lfs.

@proinsias
Copy link
Contributor Author

proinsias commented Jan 20, 2018

Not sure how to fix the following type of error:

Overcommit::Hook::PostCheckout::GitLfs when git lfs hook exits successfully should pass the check
     Failure/Error: result = execute(['git', 'lfs', 'post-checkout', previous_head, new_head, branch_checkout?])
       #<Double "context"> received unexpected message :branch_checkout? with (no args)

Copy link
Owner

@sds sds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the pull request, @proinsias. I think you've done more work than what's necessary, and can simplify substantially.

"was not found on your path.\nIf you no longer wish to use Git LFS, " \
'disable this hook by removing or setting \'enabled: false\' for GitLFS ' \
'hook in your .overcommit.yml file'
end
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You shouldn't need to perform this check explicitly. Simply specify the required_executable option in the hook configuration.

I can see that you're trying to provide a bit extra information, but my hope is that the existing error message is pretty straightforward, and that anyone working with a repo using LFS knows what Git LFS is.

'hook in your .overcommit.yml file'
end

result = execute(['git', 'lfs', 'post-checkout', previous_head, new_head, branch_checkout?])
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you read the documentation for adding existing Git hooks? If you are just executing a ready-made hook, you don't need to write any Ruby code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I've had the following two issues with this approach before, and so thought it would be helpful to have specific hooks (a la the git-lfs pre-push ones) in overcommit:

I believe the required_executable has to checked into git, and so:

  1. I have to put the hook manually in every repo.
  2. I can't use this approach for shared repos at work, where I'm the only one using overcommit.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarifying, @proinsias. I think when we added that restriction many years ago we were being overly cautious. Thinking on this, seems reasonable to allow ad hoc hooks to reference executables that are in your PATH.

The danger comes from someone creating/modifying a hook that changes the command to rm -rf / or similar, but this should be caught by the hook signing logic for the hook's configuration itself.

I'm open to a PR that modifies the HookSigner to support this. Otherwise I'll get to it when I can. Thanks!

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @proinsias, I took a stab at adding support for arbitrary commands as ad hoc hooks hooks in 5a0a4ec.

Could you try rebasing and rebuilding these hooks using that? Would love to get your feedback. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking into this now!

@proinsias
Copy link
Contributor Author

@sds – looks good to me!

@sds sds merged commit 88c82a3 into sds:master Feb 2, 2018
@proinsias proinsias deleted the proinsias/git-lfs-hooks branch August 15, 2018 01:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants