-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix pre-commit skip hook checks #3532
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, I like how you add the failing test first. 😄
This is not a review yet, since is still a draft; just two early thoughts below.
@@ -22,7 +22,13 @@ func NewCommitCommands(gitCommon *GitCommon) *CommitCommands { | |||
|
|||
// ResetAuthor resets the author of the topmost commit | |||
func (self *CommitCommands) ResetAuthor() error { | |||
message, err := self.GetCommitMessage("HEAD") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems a bit wasteful to ask git for the commit message here when we have it in our model in memory already. Maybe we should pass in either the subject or a skipHook
bool into this function (and the other ones below).
return self.cmd.New(NewGitCmd("commit").Arg("--allow-empty", "--amend", "--only").ToArgv()) | ||
|
||
return self.cmd.New(NewGitCmd("commit"). | ||
// TODO: how to decide if we should add --no-verify if we're using the editor? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the best we can do is to decide based on the existing subject and hope they don't change it in the editor. Either that, or just never pass --no-verify
in this case.
This tests for
pre-commit
hooks in several previously unchecked situations, e.g. when amending, rewording, (re)setting the author, extracting custom patch to a new commit.It's a draft because I haven't implemented all of those yet and because those that have been implemented haven't yet been added as tests (some have, some haven't).
go generate ./...
)docs/Config.md
) have been updated if necessary