Commit without pre-commit hooks action is independent on prefix #4374

kschweiger · 2025-03-07T19:32:48Z

As discussed in #1984, this PR modifies the code such that the w command always commits without pre-commit hooks. The skipHookPrefix is still considered when starting the commit messaged with the prefix in the regular commit command using c.

Skipping pre-commits also works when switching to the editor and adding a co author from the the w command

I think this PR could also remove the SkipHookPrefixNotConfigured text in english and all internationalisations but I was not sure how this is handled.

Cheatsheets are up-to-date (run go generate ./...)
Code has been formatted (see here)
Tests have been added/updated (see here for the integration test guide)
~~Text is internationalised (see here)~~
~~If a new UserConfig entry was added, make sure it can be hot-reloaded (see here)~~
~~Docs have been updated if necessary~~
You've read through your own file changes for silly mistakes etc

stefanhaller

This is a great start, but it does still need quite a bit of work; see comments below.

stefanhaller · 2025-03-08T12:50:06Z

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, verify bool) oscommands.ICmdObj {


This is a super confusing name for this variable; it's backwards. It should be renamed to noVerify, or probably better to skipHooks (because variables with no in them are an invitation for double negations, which I have a strong aversion against).

But see further discussion below.

stefanhaller · 2025-03-08T12:53:10Z

pkg/commands/git_commands/commit.go

 	cmdArgs := NewGitCmd("commit").
-		ArgIf(skipHookPrefix != "" && strings.HasPrefix(summary, skipHookPrefix), "--no-verify").
+		ArgIf(verify, "--no-verify").


I would propose to keep the previous logic here, and just "or" in the flag. The rationale is that there are several call sites that still want the old logic without having to pass in the flag themselves (I'll annotate them below).

Which means that the flag should probably be called forceSkipHooks, and be used like this:

ArgIf(forceSkipHooks || (skipHookPrefix != "" && strings.HasPrefix(summary, skipHookPrefix)), "--no-verify").

I can get behind this. I did not particularly like having to put the logic into the handleCommit function anyways.

I adapted the code as you suggested with 97e05e5

stefanhaller · 2025-03-08T14:27:07Z

pkg/commands/git_commands/commit_test.go

 			configSignoff:        false,
 			configSkipHookPrefix: "",
 			expectedArgs:         []string{"commit", "-m", "test"},
 		},
 		{
 			testName:             "Commit with --no-verify flag",
 			summary:              "WIP: test",
+			verify:               true,


If we do what I suggest above (keeping the prefix logic in CommitCmdObj), then it would be good to add two more test cases here for the different combinations of prefix vs flag.

See 97e05e5

stefanhaller · 2025-03-08T14:31:54Z

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 {


This is a regression; previously it was possible to move a custom patch into a new commit with a WIP prefix and have this skip the hooks; now this is no longer possible. Keeping the prefix logic in CommitCmdObj would fix this.

Note that there are other problems while working with custom patches currently doesn't work well with WIP prefixes; see #3532. But those are unrelated to your PR and can be fixed separately.

stefanhaller · 2025-03-08T14:35:56Z

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()


Another regression: when setting the squashMergeMessage config to start with the skipHooksPrefix, then we would previously skip the hooks when committing the result of a squash merge. You might argue whether this was intentional, but I do think it makes sense. Again, keeping the prefix logic in CommitCmdObj fixes this.

stefanhaller · 2025-03-08T14:36:56Z

pkg/gui/controllers/helpers/commits_helper.go

-	getCommitSummary     func() string
-	setCommitSummary     func(string)
-	getCommitDescription func() string
-	// getVerifyFlag                 func() bool
+	getCommitSummary              func() string
+	setCommitSummary              func(string)
+	getCommitDescription          func() string


This should go into the previous commit, I suppose. Also below.

True. I kind of expected you would squash on merge 😄

No, we don't do that here. On the contrary, I value clean and well-structured commit histories very much. (Not just for the end result of a branch when it is merged, but also during review, so I prefer a fixup-based workflow over just piling commits onto one another, which is what we are now doing here. Something to keep in mind for next time, it's too late for this one. 😄)

stefanhaller · 2025-03-08T14:43:39Z

pkg/integration/tests/commit/commit_skip_hooks.go

+
+		t.Views().Commits().Focus()
+		t.Views().Main().Content(Contains("foo bar"))
+		t.Views().Extras().Content(Contains("--no-verify"))


This is an interesting way of checking whether we passed the no-verify flag, but I'm not sure I like it much. If you had a test that committed twice, once with and once without the flag, you couldn't use this check for the second commit.

I'd prefer it if we installed an actual hook (using the technique in fail_hooks_then_commit_no_hooks.go), and have it write something to a file; we can then check whether the file exists (or what it contains) using t.FileSystem().PathPresent() or t.FileSystem().FileContent().

kschweiger

About 2a4911c:

I adapted this test to use an actual hook but I'm not quite certain this is what you meant. So this is more a draft and I'm happy to reiterate and update the other integration (e.g. CommitSwitchToEditorSkipHooks) tests if necessary.

Like this, the hook would update the file content and if you replace keys.Files.CommitChangesWithoutHook with keys.Files.CommitChanges the test fails.

Overall I'm fine with it and it is certainly more explicit but I'm not sure it is that much clearer than just looking at the --no-verify in the command log for this particular test.

stefanhaller · 2025-03-15T09:52:45Z

I adapted this test to use an actual hook but I'm not quite certain this is what you meant.

Yes, this is close to what I meant, but I was wrong. 😄 We can simplify the tests quite a bit by installing a hook that always fails. This way we don't need to check for the existence of files, or for text appearing in the Extras view, in order to tell whether the hook ran; we can tell whether it ran or not simply by whether the commit succeeded. I pushed 98ac340 to do that for some of the tests; this needs further adaptions for the others, and I think we now have some redundancy between the tests. I was too lazy to look at that in more detail, can you please clean this up?

Another thing I noticed now (and missed in the first round of review) is that there's unnecessary work happening with storing the forceSkipHooks flag in the OpenCommitMessagePanelOpts, and then passing it back to the OnConfirm handler. This is unnecessary and confusing because it is only used by the OnConfirm of the HandleCommitPressWithMessage call; all other callsites ignored the flag, and the concept of skipping hooks doesn't even make sense e.g. for creating a tag. It's much cleaner to simply capture the flag in the OnConfirm lambda in the only place where we need it; I pushed 2a86979 to do that.

Feel free to squash everything into one commit now, and then to push further fixups for the remaining test adaptations.

kschweiger · 2025-03-17T21:21:13Z

I squashed all the previous commit and refactored the tests.

Only fail_hooks_then_commit_no_hooks.go explicitly tests that the hook fail with the regular commit. I'm usually a fan of making integration tests rather explicit even if not really necessary. So I added a shared function for the other tests that basically tries to commit w/ hooks, fails and resets the working directory. In this case my rational is that they all share the same "hook" and if somebody down the line would decide to change it, the tests might still succeed even though the hook would not fail.

stefanhaller · 2025-03-22T10:01:37Z

@kschweiger Great work, looks good to me. I agree that it's good to add the sanity check that the hook works at the beginning of each test.

I had to add a fixup (47bec2d) because one of the tests failed after rebasing onto #4346. Will merge when green, thanks for the contribution!

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

stefanhaller · 2025-03-22T10:06:55Z

Oh, and I also added a commit to remove the obsolete text.

This was added in jesseduffield/lazygit#4374 which also changed the previous behaviour of the `w` keybinding to not include a default message. and discussed here jesseduffield/lazygit#1984 ^I found the solution from there too, thanks 🙏🏻

…#4456) We removed prefilling the skipHook prefix in b102646 (#4374) with the intention of making it clearer that using the prefix in normal commits and typing `w` to skip hooks are now two independent features. It turns out that some people liked it with prefilling the prefix and perceive it as a regression, so put it back in. But only if we don't have a preserved message; this is an important use case, when you try to make a normal commit, the hook fails, and then you want to make the same commit with skipping the hook, but with the same message that you already typed.

This MR contains the following updates: | Package | Update | Change | |---|---|---| | [jesseduffield/lazygit](https://github.com/jesseduffield/lazygit) | minor | `v0.48.0` -> `v0.50.0` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>jesseduffield/lazygit (jesseduffield/lazygit)</summary> ### [`v0.50.0`](https://github.com/jesseduffield/lazygit/releases/tag/v0.50.0) [Compare Source](jesseduffield/lazygit@v0.49.0...v0.50.0)  #### What's Changed ##### Enhancements 🔥 - Continue/abort a conflicted cherry-pick or revert by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4441 - Show todo items for pending cherry-picks and reverts by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4442 - Use "git cherry-pick" for implementing copy/paste of commits by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4443 - Allow reverting a range of commits by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4444 - Section headers for rebase todos and actual commits by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4463 - Focus the main view by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4429 - Auto-forward main branches after fetching by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4493 - Add new command "Move commits to new branch" by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#3876 - Strip the '+' and '-' characters when copying parts of a diff to the clipboard by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4519 - Reduce memory consumption of graph (pipe sets) by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4498 ##### Fixes 🔧 - Fix truncation of branches when scrolling branches panel to the left by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4483 - Fix nvim-remote commands for fish shell by [@SavingFrame](https://github.com/SavingFrame) in jesseduffield/lazygit#4506 - Disallow creating custom patches when the diff context size is 0 by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4522 - Remove double space between rebase todo and author columns by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4520 ##### Maintenance ⚙️ - Allow closing issues via github actions by [@jesseduffield](https://github.com/jesseduffield) in jesseduffield/lazygit#4515 ##### Docs 📖 - Add Debian installation instructions alongside Ubuntu by [@jmkim](https://github.com/jmkim) in jesseduffield/lazygit#4501 - fix wording of random tip by [@dawedawe](https://github.com/dawedawe) in jesseduffield/lazygit#4488 #### New Contributors - [@jmkim](https://github.com/jmkim) made their first contribution in jesseduffield/lazygit#4501 - [@SavingFrame](https://github.com/SavingFrame) made their first contribution in jesseduffield/lazygit#4506 - [@dawedawe](https://github.com/dawedawe) made their first contribution in jesseduffield/lazygit#4488 **Full Changelog**: jesseduffield/lazygit@v0.49.0...v0.50.0 ### [`v0.49.0`](https://github.com/jesseduffield/lazygit/releases/tag/v0.49.0) [Compare Source](jesseduffield/lazygit@v0.48.0...v0.49.0)  #### What's Changed ##### Enhancements 🔥 - Support fish when running shell command by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4350 - Add acme editor preset by [@rakoo](https://github.com/rakoo) in jesseduffield/lazygit#4360 - Support home and end as alternatives to '<' and '>' by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4396 - Drop the git config cache when getting focus by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4376 - Add a "Content of selected file" entry to the copy menu for commit files by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4341 - Add root node in file tree by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4346 - \[FEAT] Add Recursive Bulk Initialize and Update for Submodules by [@cesarandr](https://github.com/cesarandr) in jesseduffield/lazygit#4259 - Commit without pre-commit hooks action is independent on prefix by [@kschweiger](https://github.com/kschweiger) in jesseduffield/lazygit#4374 - Let users define custom icons and color for files on the config file by [@hasecilu](https://github.com/hasecilu) in jesseduffield/lazygit#4395 - Add "Absolute path" item to the file view's copy menu by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4410 - Allow range drop stashes by [@gaogao-qwq](https://github.com/gaogao-qwq) in jesseduffield/lazygit#4451 - More navigation keybindings for confirmation panel by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4404 - Resolve non-inline merge conflicts by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4431 - Add `runCommand` function to Go template syntax + add support for templates in git `branchPrefix` setting by [@ruudk](https://github.com/ruudk) in jesseduffield/lazygit#4438 - Show "(hooks disabled)" in title bar of commit message editor by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4467 - Add a command to select all commits of the current branch by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4448 ##### Fixes 🔧 - Use a waiting status for rewording a non-head commit by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4343 - Fix layout of options view for non-english languages by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4359 - Fix changing language while lazygit is running by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4361 - URL encode gitlab brackets to make consistent with branch names by [@ChrisMcD1](https://github.com/ChrisMcD1) in jesseduffield/lazygit#4336 - Fix commitPrefix setting with empty pattern by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4381 - Commit only tracked files in tracked filter view by [@parthokunda](https://github.com/parthokunda) in jesseduffield/lazygit#4386 - Revert [#4313](jesseduffield/lazygit#4313) (Skip post-checkout hook when discarding changes) by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4407 - Enhance support for GPG signed tags by [@ChrisMcD1](https://github.com/ChrisMcD1) in jesseduffield/lazygit#4394 - Fix checking out a file from a range selection of commits by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4423 - Fix discarding submodule changes in nested folders by [@brandondong](https://github.com/brandondong) in jesseduffield/lazygit#4317 - Better support for shell aliases by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4385 - Fix hyperlinks in last line of confirmation popups by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4454 - Fix wrong main view content after pressing `e` in a stack of branches by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4450 - fix: update vscode color to logo color by [@PeterCardenas](https://github.com/PeterCardenas) in jesseduffield/lazygit#4459 - Fix crash with drag selecting and staging by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4480 - Escape special characters in filenames when git-ignoring files by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4475 ##### Maintenance ⚙️ - Fix linter warnings by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4352 - Fix release schedule again by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4364 - Update to go 1.24 by [@radsaq](https://github.com/radsaq) in jesseduffield/lazygit#4377 - Add an integration test for a config with a negative refspec by [@radsaq](https://github.com/radsaq) in jesseduffield/lazygit#4382 - Specify a go release minor version by [@radsaq](https://github.com/radsaq) in jesseduffield/lazygit#4393 - Fix flaky integration test by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4432 - Some code cleanup by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4449 - Bump the minimum required git version to 2.22 by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4439 - Bump go-git, and get rid of github.com/imdario/mergo by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4460 - Skip date check when release worfklow is manually invoked by [@jesseduffield](https://github.com/jesseduffield) in jesseduffield/lazygit#4484 ##### Docs 📖 - Include empty arrays and maps in config docs by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4413 - Filter out deprecated user config fields from generated Config.md by [@karimkhaleel](https://github.com/karimkhaleel) in jesseduffield/lazygit#4416 - Corrected interactive rebase keybind example in README.md by [@NewtonChutney](https://github.com/NewtonChutney) in jesseduffield/lazygit#4426 - Update kidpix link in README to active url by [@ChrisMcD1](https://github.com/ChrisMcD1) in jesseduffield/lazygit#4425 ##### I18n 🌎 - Update translation files from Crowdin by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4473 #### New Contributors - [@rakoo](https://github.com/rakoo) made their first contribution in jesseduffield/lazygit#4360 - [@radsaq](https://github.com/radsaq) made their first contribution in jesseduffield/lazygit#4377 - [@cesarandr](https://github.com/cesarandr) made their first contribution in jesseduffield/lazygit#4259 - [@kschweiger](https://github.com/kschweiger) made their first contribution in jesseduffield/lazygit#4374 - [@NewtonChutney](https://github.com/NewtonChutney) made their first contribution in jesseduffield/lazygit#4426 - [@gaogao-qwq](https://github.com/gaogao-qwq) made their first contribution in jesseduffield/lazygit#4451 - [@ruudk](https://github.com/ruudk) made their first contribution in jesseduffield/lazygit#4438 **Full Changelog**: jesseduffield/lazygit@v0.48.0...v0.49.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

stefanhaller reviewed Mar 8, 2025

View reviewed changes

kschweiger commented Mar 10, 2025

View reviewed changes

stefanhaller added the enhancement New feature or request label Mar 15, 2025

kschweiger force-pushed the no_verify_wo_prefix branch from 98ac340 to 11842f7 Compare March 17, 2025 21:11

stefanhaller force-pushed the no_verify_wo_prefix branch from 11842f7 to 47bec2d Compare March 22, 2025 09:59

Commit without pre-commit hooks is independent on prefix

b102646

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

stefanhaller force-pushed the no_verify_wo_prefix branch from 47bec2d to b102646 Compare March 22, 2025 10:04

Remove text that is now unused

728f0d9

stefanhaller enabled auto-merge March 22, 2025 10:07

stefanhaller merged commit a51bf12 into jesseduffield:master Mar 22, 2025
15 checks passed

stefanhaller mentioned this pull request Mar 22, 2025

How to ignore pre commit/push hooks for commit #1984

Closed

stefanhaller mentioned this pull request Apr 3, 2025

Prefill the commit subject with the skipHook prefix when pressing w #4456

Merged

jesseduffield mentioned this pull request Apr 14, 2025

lazygit 0.49.0 Homebrew/homebrew-core#219995

Merged

Uh oh!

Commit without pre-commit hooks action is independent on prefix #4374

Commit without pre-commit hooks action is independent on prefix #4374

Uh oh!

Conversation

kschweiger commented Mar 7, 2025

Uh oh!

stefanhaller left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kschweiger left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stefanhaller commented Mar 15, 2025

Uh oh!

kschweiger commented Mar 17, 2025

Uh oh!

stefanhaller commented Mar 22, 2025

Uh oh!

stefanhaller commented Mar 22, 2025

Uh oh!

Uh oh!

Uh oh!

kschweiger left a comment •

edited

Loading