Skip to content

Conversation

@jawshooah
Copy link
Collaborator

Pre-push hooks are passed the name and url of the remote as arguments, and STDIN contains info about pushed commits, one per line, in the format local_ref local_sha1 remote_ref remote_sha1.

Copy link
Owner

Choose a reason for hiding this comment

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

ARGF has some additional semantics that aren't necessary here. I'd like it if we just read directly from STDIN.

However, I'd like to take this request a step further by having us inject STDIN as a dependency to the hook context itself. Then we could define the input_lines helper on Overcommit::HookContext::Base and reuse it in post-rewrite. To be clear, I'd like to see https://github.com/brigade/overcommit/blob/master/template-dir/hooks/overcommit-hook#L48 become:

context = Overcommit::HookContext.create(hook_type, config, ARGV, STDIN)

...and have the various HookContext classes take an additional argument input containing the input stream. I think this will make testing cleaner (it forces us to be explicit about what we're expecting in the input stream, even for hooks that don't read from STDIN).

Overcommit will automatically update itself when overcommit-hook changes, so this shouldn't have any negative consequences for users when upgrading.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's how I initially implemented it, but I didn't want this PR to grow beyond its intended scope. But I agree that constructor injection is the way to go here.

@sds sds merged commit 8764057 into sds:master Apr 3, 2015
@sds sds added the enhancement label Apr 3, 2015
@sds
Copy link
Owner

sds commented Apr 3, 2015

Thanks!

@jawshooah jawshooah deleted the pre-push branch April 3, 2015 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants