From b102646b20749767474d7b71658531ebd4d419e1 Mon Sep 17 00:00:00 2001 From: Korbinian Schweiger Date: Wed, 5 Mar 2025 20:41:29 +0100 Subject: [PATCH] Commit without pre-commit hooks is independent on prefix Add verify flag Add and update integration tests Rename verify to forceSkipHooks Adapt CommitSkipHooks integration test to actually use a hook Remove forceSkipHooks param from OnConfirm et al Simplify tests --- pkg/commands/git_commands/commit.go | 9 ++- pkg/commands/git_commands/commit_test.go | 26 +++++++- pkg/commands/git_commands/patch.go | 2 +- .../helpers/merge_and_rebase_helper.go | 2 +- .../helpers/working_tree_helper.go | 27 ++++---- .../tests/commit/commit_skip_hooks.go | 44 +++++++++++++ .../commit_switch_to_editor_skip_hooks.go | 64 +++++++++++++++++++ .../tests/commit/commit_wip_with_prefix.go | 18 ++++-- .../commit/fail_hooks_then_commit_no_hooks.go | 46 +++++++++++++ pkg/integration/tests/commit/shared.go | 35 ++++++++++ .../tests/commit/staged_without_hooks.go | 11 +++- pkg/integration/tests/test_list.go | 3 + 12 files changed, 254 insertions(+), 33 deletions(-) create mode 100644 pkg/integration/tests/commit/commit_skip_hooks.go create mode 100644 pkg/integration/tests/commit/commit_switch_to_editor_skip_hooks.go create mode 100644 pkg/integration/tests/commit/fail_hooks_then_commit_no_hooks.go diff --git a/pkg/commands/git_commands/commit.go b/pkg/commands/git_commands/commit.go index 46f07bdf1..cd6033b39 100644 --- a/pkg/commands/git_commands/commit.go +++ b/pkg/commands/git_commands/commit.go @@ -85,13 +85,11 @@ func (self *CommitCommands) ResetToCommit(hash string, strength string, envVars Run() } -func (self *CommitCommands) CommitCmdObj(summary string, description string) oscommands.ICmdObj { +func (self *CommitCommands) CommitCmdObj(summary string, description string, forceSkipHooks bool) oscommands.ICmdObj { messageArgs := self.commitMessageArgs(summary, description) - skipHookPrefix := self.UserConfig().Git.SkipHookPrefix - cmdArgs := NewGitCmd("commit"). - ArgIf(skipHookPrefix != "" && strings.HasPrefix(summary, skipHookPrefix), "--no-verify"). + ArgIf(forceSkipHooks || (skipHookPrefix != "" && strings.HasPrefix(summary, skipHookPrefix)), "--no-verify"). ArgIf(self.signoffFlag() != "", self.signoffFlag()). Arg(messageArgs...). ToArgv() @@ -108,8 +106,9 @@ func (self *CommitCommands) RewordLastCommitInEditorWithMessageFileCmdObj(tmpMes Arg("--allow-empty", "--amend", "--only", "--edit", "--file="+tmpMessageFile).ToArgv()) } -func (self *CommitCommands) CommitInEditorWithMessageFileCmdObj(tmpMessageFile string) oscommands.ICmdObj { +func (self *CommitCommands) CommitInEditorWithMessageFileCmdObj(tmpMessageFile string, forceSkipHooks bool) oscommands.ICmdObj { return self.cmd.New(NewGitCmd("commit"). + ArgIf(forceSkipHooks, "--no-verify"). Arg("--edit"). Arg("--file="+tmpMessageFile). ArgIf(self.signoffFlag() != "", self.signoffFlag()). diff --git a/pkg/commands/git_commands/commit_test.go b/pkg/commands/git_commands/commit_test.go index a522c81d0..a00a4ff23 100644 --- a/pkg/commands/git_commands/commit_test.go +++ b/pkg/commands/git_commands/commit_test.go @@ -53,6 +53,7 @@ func TestCommitCommitCmdObj(t *testing.T) { type scenario struct { testName string summary string + forceSkipHooks bool description string configSignoff bool configSkipHookPrefix string @@ -63,20 +64,39 @@ func TestCommitCommitCmdObj(t *testing.T) { { testName: "Commit", summary: "test", + forceSkipHooks: false, configSignoff: false, configSkipHookPrefix: "", expectedArgs: []string{"commit", "-m", "test"}, }, { - testName: "Commit with --no-verify flag", + testName: "Commit with --no-verify flag < only prefix", summary: "WIP: test", + forceSkipHooks: false, configSignoff: false, configSkipHookPrefix: "WIP", expectedArgs: []string{"commit", "--no-verify", "-m", "WIP: test"}, }, + { + testName: "Commit with --no-verify flag < skip flag and prefix", + summary: "WIP: test", + forceSkipHooks: true, + configSignoff: false, + configSkipHookPrefix: "WIP", + expectedArgs: []string{"commit", "--no-verify", "-m", "WIP: test"}, + }, + { + testName: "Commit with --no-verify flag < skip flag no prefix", + summary: "test", + forceSkipHooks: true, + configSignoff: false, + configSkipHookPrefix: "WIP", + expectedArgs: []string{"commit", "--no-verify", "-m", "test"}, + }, { testName: "Commit with multiline message", summary: "line1", + forceSkipHooks: false, description: "line2", configSignoff: false, configSkipHookPrefix: "", @@ -85,6 +105,7 @@ func TestCommitCommitCmdObj(t *testing.T) { { testName: "Commit with signoff", summary: "test", + forceSkipHooks: false, configSignoff: true, configSkipHookPrefix: "", expectedArgs: []string{"commit", "--signoff", "-m", "test"}, @@ -92,6 +113,7 @@ func TestCommitCommitCmdObj(t *testing.T) { { testName: "Commit with signoff and no-verify", summary: "WIP: test", + forceSkipHooks: true, configSignoff: true, configSkipHookPrefix: "WIP", expectedArgs: []string{"commit", "--no-verify", "--signoff", "-m", "WIP: test"}, @@ -107,7 +129,7 @@ func TestCommitCommitCmdObj(t *testing.T) { runner := oscommands.NewFakeRunner(t).ExpectGitArgs(s.expectedArgs, "", nil) instance := buildCommitCommands(commonDeps{userConfig: userConfig, runner: runner}) - assert.NoError(t, instance.CommitCmdObj(s.summary, s.description).Run()) + assert.NoError(t, instance.CommitCmdObj(s.summary, s.description, s.forceSkipHooks).Run()) runner.CheckForMissingCalls() }) } diff --git a/pkg/commands/git_commands/patch.go b/pkg/commands/git_commands/patch.go index bceaf9943..830ee2cb6 100644 --- a/pkg/commands/git_commands/patch.go +++ b/pkg/commands/git_commands/patch.go @@ -303,7 +303,7 @@ func (self *PatchCommands) PullPatchIntoNewCommit( return err } - if err := self.commit.CommitCmdObj(commitSummary, commitDescription).Run(); err != nil { + if err := self.commit.CommitCmdObj(commitSummary, commitDescription, false).Run(); err != nil { return err } diff --git a/pkg/gui/controllers/helpers/merge_and_rebase_helper.go b/pkg/gui/controllers/helpers/merge_and_rebase_helper.go index 40d9e6df2..04592519b 100644 --- a/pkg/gui/controllers/helpers/merge_and_rebase_helper.go +++ b/pkg/gui/controllers/helpers/merge_and_rebase_helper.go @@ -481,7 +481,7 @@ func (self *MergeAndRebaseHelper) SquashMergeCommitted(refName, checkedOutBranch "selectedRef": refName, "currentBranch": checkedOutBranchName, }) - err = self.c.Git().Commit.CommitCmdObj(message, "").Run() + err = self.c.Git().Commit.CommitCmdObj(message, "", false).Run() if err != nil { return err } diff --git a/pkg/gui/controllers/helpers/working_tree_helper.go b/pkg/gui/controllers/helpers/working_tree_helper.go index 1cf658818..acec444ad 100644 --- a/pkg/gui/controllers/helpers/working_tree_helper.go +++ b/pkg/gui/controllers/helpers/working_tree_helper.go @@ -86,7 +86,7 @@ func (self *WorkingTreeHelper) OpenMergeTool() error { return nil } -func (self *WorkingTreeHelper) HandleCommitPressWithMessage(initialMessage string) error { +func (self *WorkingTreeHelper) HandleCommitPressWithMessage(initialMessage string, forceSkipHooks bool) error { return self.WithEnsureCommittableFiles(func() error { self.commitsHelper.OpenCommitMessagePanel( &OpenCommitMessagePanelOpts{ @@ -95,8 +95,12 @@ func (self *WorkingTreeHelper) HandleCommitPressWithMessage(initialMessage strin SummaryTitle: self.c.Tr.CommitSummaryTitle, DescriptionTitle: self.c.Tr.CommitDescriptionTitle, PreserveMessage: true, - OnConfirm: self.handleCommit, - OnSwitchToEditor: self.switchFromCommitMessagePanelToEditor, + OnConfirm: func(summary string, description string) error { + return self.handleCommit(summary, description, forceSkipHooks) + }, + OnSwitchToEditor: func(filepath string) error { + return self.switchFromCommitMessagePanelToEditor(filepath, forceSkipHooks) + }, }, ) @@ -104,8 +108,8 @@ func (self *WorkingTreeHelper) HandleCommitPressWithMessage(initialMessage strin }) } -func (self *WorkingTreeHelper) handleCommit(summary string, description string) error { - cmdObj := self.c.Git().Commit.CommitCmdObj(summary, description) +func (self *WorkingTreeHelper) handleCommit(summary string, description string, forceSkipHooks bool) error { + cmdObj := self.c.Git().Commit.CommitCmdObj(summary, description, forceSkipHooks) self.c.LogAction(self.c.Tr.Actions.Commit) return self.gpgHelper.WithGpgHandling(cmdObj, self.c.Tr.CommittingStatus, func() error { self.commitsHelper.OnCommitSuccess() @@ -113,7 +117,7 @@ func (self *WorkingTreeHelper) handleCommit(summary string, description string) }) } -func (self *WorkingTreeHelper) switchFromCommitMessagePanelToEditor(filepath string) error { +func (self *WorkingTreeHelper) switchFromCommitMessagePanelToEditor(filepath string, forceSkipHooks bool) error { // We won't be able to tell whether the commit was successful, because // RunSubprocessAndRefresh doesn't return the error (it opens an error alert // itself and returns nil on error). But even if we could, we wouldn't have @@ -124,7 +128,7 @@ func (self *WorkingTreeHelper) switchFromCommitMessagePanelToEditor(filepath str self.c.LogAction(self.c.Tr.Actions.Commit) return self.c.RunSubprocessAndRefresh( - self.c.Git().Commit.CommitInEditorWithMessageFileCmdObj(filepath), + self.c.Git().Commit.CommitInEditorWithMessageFileCmdObj(filepath, forceSkipHooks), ) } @@ -140,12 +144,7 @@ func (self *WorkingTreeHelper) HandleCommitEditorPress() error { } func (self *WorkingTreeHelper) HandleWIPCommitPress() error { - skipHookPrefix := self.c.UserConfig().Git.SkipHookPrefix - if skipHookPrefix == "" { - return errors.New(self.c.Tr.SkipHookPrefixNotConfigured) - } - - return self.HandleCommitPressWithMessage(skipHookPrefix) + return self.HandleCommitPressWithMessage("", true) } func (self *WorkingTreeHelper) HandleCommitPress() error { @@ -173,7 +172,7 @@ func (self *WorkingTreeHelper) HandleCommitPress() error { } } - return self.HandleCommitPressWithMessage(message) + return self.HandleCommitPressWithMessage(message, false) } func (self *WorkingTreeHelper) WithEnsureCommittableFiles(handler func() error) error { diff --git a/pkg/integration/tests/commit/commit_skip_hooks.go b/pkg/integration/tests/commit/commit_skip_hooks.go new file mode 100644 index 000000000..d2e9431fb --- /dev/null +++ b/pkg/integration/tests/commit/commit_skip_hooks.go @@ -0,0 +1,44 @@ +package commit + +import ( + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +var blockingHook = `#!/bin/bash + +# For this test all we need is a hook that always fails +exit 1 +` + +var CommitSkipHooks = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "Commit with skip hook using CommitChangesWithoutHook", + ExtraCmdArgs: []string{}, + Skip: false, + SetupConfig: func(config *config.AppConfig) {}, + SetupRepo: func(shell *Shell) { + shell.CreateFile(".git/hooks/pre-commit", blockingHook) + shell.MakeExecutable(".git/hooks/pre-commit") + + shell.CreateFile("file.txt", "content") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + checkBlockingHook(t, keys) + + t.Views().Files(). + IsFocused(). + PressPrimaryAction(). + Lines( + Equals("A file.txt"), + ). + Press(keys.Files.CommitChangesWithoutHook) + + t.ExpectPopup().CommitMessagePanel(). + Title(Equals("Commit summary")). + Type("foo bar"). + Confirm() + + t.Views().Commits().Focus() + t.Views().Main().Content(Contains("foo bar")) + }, +}) diff --git a/pkg/integration/tests/commit/commit_switch_to_editor_skip_hooks.go b/pkg/integration/tests/commit/commit_switch_to_editor_skip_hooks.go new file mode 100644 index 000000000..008a217b4 --- /dev/null +++ b/pkg/integration/tests/commit/commit_switch_to_editor_skip_hooks.go @@ -0,0 +1,64 @@ +package commit + +import ( + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +var CommitSwitchToEditorSkipHooks = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "Commit, then switch from built-in commit message panel to editor", + ExtraCmdArgs: []string{}, + Skip: false, + SetupConfig: func(config *config.AppConfig) {}, + SetupRepo: func(shell *Shell) { + shell.CreateFile(".git/hooks/pre-commit", blockingHook) + shell.MakeExecutable(".git/hooks/pre-commit") + shell.CreateFile("file1", "file1 content") + shell.CreateFile("file2", "file2 content") + + // Set an editor that appends a line to the existing message. Since + // git adds all this "# Please enter the commit message for your changes" + // stuff, this will result in an extra blank line before the added line. + shell.SetConfig("core.editor", "sh -c 'echo third line >>.git/COMMIT_EDITMSG'") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Commits(). + IsEmpty() + + checkBlockingHook(t, keys) + + t.Views().Files(). + IsFocused(). + Lines( + Equals("▼ /").IsSelected(), + Equals(" ?? file1"), + Equals(" ?? file2"), + ). + SelectNextItem(). + PressPrimaryAction(). // stage one of the files + Press(keys.Files.CommitChangesWithoutHook) + + t.ExpectPopup().CommitMessagePanel(). + Type("first line"). + SwitchToDescription(). + Type("second line"). + SwitchToSummary(). + SwitchToEditor() + t.Views().Commits(). + Lines( + Contains("first line"), + ) + + t.Views().Commits().Focus() + t.Views().Main().Content(MatchesRegexp(`first line\n\s*\n\s*second line\n\s*\n\s*third line`)) + + // Now check that the preserved commit message was cleared: + t.Views().Files(). + Focus(). + PressPrimaryAction(). // stage the other file + Press(keys.Files.CommitChanges) + + t.ExpectPopup().CommitMessagePanel(). + InitialText(Equals("")) + }, +}) diff --git a/pkg/integration/tests/commit/commit_wip_with_prefix.go b/pkg/integration/tests/commit/commit_wip_with_prefix.go index 6223de04f..822a2b6ef 100644 --- a/pkg/integration/tests/commit/commit_wip_with_prefix.go +++ b/pkg/integration/tests/commit/commit_wip_with_prefix.go @@ -13,6 +13,9 @@ var CommitWipWithPrefix = NewIntegrationTest(NewIntegrationTestArgs{ cfg.GetUserConfig().Git.CommitPrefixes = map[string][]config.CommitPrefixConfig{"repo": {{Pattern: "^\\w+\\/(\\w+-\\w+).*", Replace: "[$1]: "}}} }, SetupRepo: func(shell *Shell) { + shell.CreateFile(".git/hooks/pre-commit", blockingHook) + shell.MakeExecutable(".git/hooks/pre-commit") + shell.NewBranch("feature/TEST-002") shell.CreateFile("test-wip-commit-prefix", "This is foo bar") }, @@ -20,6 +23,8 @@ var CommitWipWithPrefix = NewIntegrationTest(NewIntegrationTestArgs{ t.Views().Commits(). IsEmpty() + checkBlockingHook(t, keys) + t.Views().Files(). IsFocused(). PressPrimaryAction(). @@ -27,31 +32,30 @@ var CommitWipWithPrefix = NewIntegrationTest(NewIntegrationTestArgs{ t.ExpectPopup().CommitMessagePanel(). Title(Equals("Commit summary")). - InitialText(Equals("WIP")). - Type(" foo"). + Type("foo"). Cancel() t.Views().Files(). IsFocused(). - Press(keys.Files.CommitChanges) + Press(keys.Files.CommitChangesWithoutHook) t.ExpectPopup().CommitMessagePanel(). Title(Equals("Commit summary")). - InitialText(Equals("WIP foo")). + InitialText(Equals("foo")). Type(" bar"). Cancel() t.Views().Files(). IsFocused(). - Press(keys.Files.CommitChanges) + Press(keys.Files.CommitChangesWithoutHook) t.ExpectPopup().CommitMessagePanel(). Title(Equals("Commit summary")). - InitialText(Equals("WIP foo bar")). + InitialText(Equals("foo bar")). Type(". Added something else"). Confirm() t.Views().Commits().Focus() - t.Views().Main().Content(Contains("WIP foo bar. Added something else")) + t.Views().Main().Content(Contains("foo bar. Added something else")) }, }) diff --git a/pkg/integration/tests/commit/fail_hooks_then_commit_no_hooks.go b/pkg/integration/tests/commit/fail_hooks_then_commit_no_hooks.go new file mode 100644 index 000000000..6b328d234 --- /dev/null +++ b/pkg/integration/tests/commit/fail_hooks_then_commit_no_hooks.go @@ -0,0 +1,46 @@ +package commit + +import ( + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +var FailHooksThenCommitNoHooks = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "Verify that commit message can be reused in commit without hook after failing commit with hooks", + ExtraCmdArgs: []string{}, + Skip: false, + SetupConfig: func(config *config.AppConfig) { + }, + SetupRepo: func(shell *Shell) { + shell.CreateFile(".git/hooks/pre-commit", blockingHook) + shell.MakeExecutable(".git/hooks/pre-commit") + + shell.CreateFileAndAdd("one", "one") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Files(). + IsFocused(). + Lines( + Contains("one"), + ). + Press(keys.Files.CommitChanges). + Tap(func() { + t.ExpectPopup().CommitMessagePanel().Type("my message").Confirm() + + t.ExpectPopup().Alert().Title(Equals("Error")).Content(Contains("Git command failed")).Confirm() + }). + Press(keys.Files.CommitChangesWithoutHook). + Tap(func() { + t.ExpectPopup().CommitMessagePanel(). + InitialText(Equals("my message")). // it remembered the commit message + Confirm() + + t.Views().Commits(). + Lines( + Contains("my message"), + ) + }) + t.Views().Commits().Focus() + t.Views().Main().Content(Contains("my message")) + }, +}) diff --git a/pkg/integration/tests/commit/shared.go b/pkg/integration/tests/commit/shared.go index ee1d3e093..072cc8154 100644 --- a/pkg/integration/tests/commit/shared.go +++ b/pkg/integration/tests/commit/shared.go @@ -72,3 +72,38 @@ func checkCommitContainsChange(t *TestDriver, commitSubject string, change strin t.Views().Main(). Content(Contains(change)) } + +func checkBlockingHook(t *TestDriver, keys config.KeybindingConfig) { + // Shared function for tests using the blockingHook pre-commit hook for testing hook skipping + // Stage first file + t.Views().Files(). + IsFocused(). + PressPrimaryAction(). + Press(keys.Files.CommitChanges) + + // Try to commit with hook + t.ExpectPopup().CommitMessagePanel(). + Title(Equals("Commit summary")). + Type("Commit should fail"). + Confirm() + + t.ExpectPopup().Alert(). + Title(Equals("Error")). + Content(Contains("Git command failed.")). + Confirm() + + // Clear the message + t.Views().Files(). + IsFocused(). + Press(keys.Files.CommitChanges) + + t.ExpectPopup().CommitMessagePanel(). + Title(Equals("Commit summary")). + Clear(). + Cancel() + + // Unstage the file + t.Views().Files(). + IsFocused(). + PressPrimaryAction() +} diff --git a/pkg/integration/tests/commit/staged_without_hooks.go b/pkg/integration/tests/commit/staged_without_hooks.go index 0a020e58f..b4e16c8ea 100644 --- a/pkg/integration/tests/commit/staged_without_hooks.go +++ b/pkg/integration/tests/commit/staged_without_hooks.go @@ -11,6 +11,9 @@ var StagedWithoutHooks = NewIntegrationTest(NewIntegrationTestArgs{ Skip: false, SetupConfig: func(config *config.AppConfig) {}, SetupRepo: func(shell *Shell) { + shell.CreateFile(".git/hooks/pre-commit", blockingHook) + shell.MakeExecutable(".git/hooks/pre-commit") + shell. CreateFile("myfile", "myfile content\nwith a second line"). CreateFile("myfile2", "myfile2 content") @@ -19,6 +22,8 @@ var StagedWithoutHooks = NewIntegrationTest(NewIntegrationTestArgs{ t.Views().Commits(). IsEmpty() + checkBlockingHook(t, keys) + // stage the file t.Views().Files(). IsFocused(). @@ -50,12 +55,12 @@ var StagedWithoutHooks = NewIntegrationTest(NewIntegrationTestArgs{ Content(DoesNotContain("+myfile content").Contains("+with a second line")). Press(keys.Files.CommitChangesWithoutHook) - commitMessage := ": my commit message" - t.ExpectPopup().CommitMessagePanel().InitialText(Contains("WIP")).Type(commitMessage).Confirm() + commitMessage := "my commit message" + t.ExpectPopup().CommitMessagePanel().Type(commitMessage).Confirm() t.Views().Commits(). Lines( - Contains("WIP" + commitMessage), + Contains(commitMessage), ) t.Views().StagingSecondary(). diff --git a/pkg/integration/tests/test_list.go b/pkg/integration/tests/test_list.go index c826faf7c..cce631956 100644 --- a/pkg/integration/tests/test_list.go +++ b/pkg/integration/tests/test_list.go @@ -91,7 +91,9 @@ var tests = []*components.IntegrationTest{ commit.Checkout, commit.Commit, commit.CommitMultiline, + commit.CommitSkipHooks, commit.CommitSwitchToEditor, + commit.CommitSwitchToEditorSkipHooks, commit.CommitWipWithPrefix, commit.CommitWithFallthroughPrefix, commit.CommitWithGlobalPrefix, @@ -105,6 +107,7 @@ var tests = []*components.IntegrationTest{ commit.CreateTag, commit.DisableCopyCommitMessageBody, commit.DiscardOldFileChanges, + commit.FailHooksThenCommitNoHooks, commit.FindBaseCommitForFixup, commit.FindBaseCommitForFixupDisregardMainBranch, commit.FindBaseCommitForFixupOnlyAddedLines,