Fix pre-commit skip hook checks #3532
Conversation
stefanhaller
left a comment
There was a problem hiding this comment.
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.
|
|
||
| // 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.
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).
There was a problem hiding this comment.
I'm not entirely sure we do, there's this in the same file so I figured we don't have it ready. I can't find a place where we actually store the current message.
There was a problem hiding this comment.
there's this in the same file so I figured we don't have it ready.
That code needs to get the message from git because it needs the full message, including the body (what we call "description" in lazygit). You don't need that here, the subject is enough in this case, and you can just take that from the models.Commit object.
There was a problem hiding this comment.
Could you please point me in the direction of how to get models in this context?
We have Model() in ControllerCommon, but CommitCommands has GitCommon and I can't find anything resembling Model there, not even through code-completion.
There was a problem hiding this comment.
The immediate caller (RebaseCommands.ResetCommitAuthor) has the commit and could pass it in; either as the subject string, or as a skipHook bool (by doing the HasPrefix at the call site), as I said above. That's the simplest way.
However, I'd suggest to do it one level above in the call chain (LocalCommitsController.resetAuthor) and pass in the information from there; probably rather as a skipHooks flag than passing the subject. My thinking is that here we are in the git_commands package, and this is only responsible for carrying out git commands, but not have any logic otherwise; decisions like that should be made by clients and passed in as flags.
You probably want to have a shouldSkipCommitHooks(models.Commit *commit) helper function in LocalCommitsController to avoid the code repetition.
| 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.
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.
There was a problem hiding this comment.
I think it's safer not to run it, than to run it based on luck.
This tests for
pre-commithooks 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