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 😄

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.

kschweiger added 2 commits March 7, 2025 20:01

Add verify flag

40e3827

Add and update integration tests

d6ecdd4

stefanhaller reviewed Mar 8, 2025

View reviewed changes

kschweiger added 2 commits March 10, 2025 21:24

Rename verify to forceSkipHooks

97e05e5

Adapt CommitSkipHooks integration test to actually use a hook

2a4911c

kschweiger commented Mar 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kschweiger commented Mar 7, 2025

stefanhaller left a comment

stefanhaller Mar 8, 2025

stefanhaller Mar 8, 2025

kschweiger Mar 10, 2025

kschweiger Mar 10, 2025

stefanhaller Mar 8, 2025

kschweiger Mar 10, 2025

stefanhaller Mar 8, 2025

stefanhaller Mar 8, 2025

stefanhaller Mar 8, 2025

kschweiger Mar 10, 2025

stefanhaller Mar 8, 2025

kschweiger left a comment •

edited

Loading

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

Are you sure you want to change the base?

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

Conversation

kschweiger commented Mar 7, 2025

stefanhaller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kschweiger left a comment • edited Loading

Choose a reason for hiding this comment

kschweiger left a comment •

edited

Loading