Conversation
There was a problem hiding this comment.
Thanks for the pull request, @benbernard!
Overall this looks good–I've left some comments inline.
To answer your higher-level questions:
- Let's keep the signature in
.git/configfor now since it provides backwards compatibility (user's existing signatures will continue to work without issue). We'll remove it in a future version. - By tests for the "configuration stuff" I assume you mean some of the signing logic in the
Configurationclass? We didn't have any unit tests, no. We were relying on this integration spec to test hook signing. If you add an appropriate test there we will be happy, but would of course love to have some unit tests if you're up for it. - See 2.
- Logical or atomic commits are ideal, so stick with what you're currently doing. Ideally we have commits that don't fail on their own (so you can
git bisect), but this isn't rigorously followed all the time in this repository.
| File.join(Overcommit::Utils.repo_root, @hash['plugin_directory'] || '.git-hooks') | ||
| end | ||
|
|
||
| # Returns absolute path to directoy for the history of signatures |
|
|
||
| def signature_history | ||
| if @hash['signature_directory'] | ||
| @hash['signature_history'].to_i |
There was a problem hiding this comment.
Can you elaborate on why setting the signature history is dependent on whether the signature_directory configuration option is set? It seems like we should respect signature_history regardless.
| # likely to be used, and will be read sooner | ||
| signatures = [] | ||
| if has_history_file | ||
| signatures = (File.readlines history_file).first(@config.signature_history - 1) |
There was a problem hiding this comment.
The parentheses here seem odd. Let's just do:
File.readlines(history_file).first(@config.signature_history - 1)| end | ||
| end | ||
|
|
||
| def signature_in_history_file(signature) |
There was a problem hiding this comment.
Let's end this with ? for consistency with other boolean-returning methods.
Same with has_history_file below.
|
|
||
| found = false | ||
| File.open(history_file, 'r') do |fh| | ||
| # Process the header |
| line.chomp | ||
|
|
||
| if line == signature | ||
| found = true |
There was a problem hiding this comment.
Could simply return true here and then false at the end of this method since there doesn't seem to be a strong need for the found variable.
There was a problem hiding this comment.
Ah, I realize you might have done this because of the block, perhaps for readability. Keep it if you like.
| def signature_directory | ||
| File.join(Overcommit::Utils.repo_root, | ||
| '.git', | ||
| @hash['signature_directory'] || 'overcommit-signatures') |
There was a problem hiding this comment.
Can you elaborate on why allowing this to be customized is useful? I'm fearful of an overzealous user changing this to a directory of files which are tracked by their Git repository because it can be "helpful" to others in storing previous signatures. Of course this is problematic since now an attacker can modify said files.
I anticipate this is a highly-unlikely scenario, but I would prefer to stick to the YAGNI principle unless there's a solid reason to allow this to be customized. Curious to hear your thoughts here.
|
Any change of an update here? I'd love to see this functionality. |
|
Haven't had a chance to get back to this, all the comments make sense, just haven't had a chance to make tests |
|
Totally understand – just showing my interest! Thanks for working on this! |
|
I'm not using overcommit anymore (haven't in years, i'm going to close this PR, if anyone wants to take this over the line, feel free |
3 participants
This is in reference to #300
I am very inexperienced with ruby, so I've probably done some strange / wrong things here, would love your feedback on anything like that.
I also haven't written tests yet. I wanted to get this up here and gather any feedback on the approach, and I have a couple of questions
Obviously, not ready to merge, and just looking for feedback here.