From 4e0194d8f7ac9a8bf837907d6e636790b26f8411 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Tue, 14 Oct 2025 14:57:59 +0200 Subject: [PATCH] Replace MergeOpts struct with MergeVariant enum - Squash and FastForwardOnly are mutually exclusive, and instead of asserting this at runtime, model the API so that they can't be passed together. - FastForwardOnly is unused, so remove it; however, we are going to need --ff and --no-ff in the next commit, so add those instead. - Instead of putting the enum into the MergeOpts struct, replace the struct by the enum. We can reintroduce the struct when we add more arguments, but for now it's an unnecessary indirection. --- pkg/commands/git_commands/branch.go | 35 +++++++++++++------ pkg/commands/git_commands/branch_test.go | 30 +++++++++++----- .../helpers/merge_and_rebase_helper.go | 6 ++-- 3 files changed, 50 insertions(+), 21 deletions(-) diff --git a/pkg/commands/git_commands/branch.go b/pkg/commands/git_commands/branch.go index 13abbd697..ac5555b3d 100644 --- a/pkg/commands/git_commands/branch.go +++ b/pkg/commands/git_commands/branch.go @@ -250,20 +250,35 @@ func (self *BranchCommands) Rename(oldName string, newName string) error { return self.cmd.New(cmdArgs).Run() } -type MergeOpts struct { - FastForwardOnly bool - Squash bool -} +type MergeVariant int + +const ( + MERGE_VARIANT_REGULAR MergeVariant = iota + MERGE_VARIANT_FAST_FORWARD + MERGE_VARIANT_NON_FAST_FORWARD + MERGE_VARIANT_SQUASH +) + +func (self *BranchCommands) Merge(branchName string, variant MergeVariant) error { + extraArgs := func() []string { + switch variant { + case MERGE_VARIANT_REGULAR: + return []string{} + case MERGE_VARIANT_FAST_FORWARD: + return []string{"--ff"} + case MERGE_VARIANT_NON_FAST_FORWARD: + return []string{"--no-ff"} + case MERGE_VARIANT_SQUASH: + return []string{"--squash", "--ff"} + } + + panic("shouldn't get here") + }() -func (self *BranchCommands) Merge(branchName string, opts MergeOpts) error { - if opts.Squash && opts.FastForwardOnly { - panic("Squash and FastForwardOnly can't both be true") - } cmdArgs := NewGitCmd("merge"). Arg("--no-edit"). Arg(strings.Fields(self.UserConfig().Git.Merging.Args)...). - ArgIf(opts.FastForwardOnly, "--ff-only"). - ArgIf(opts.Squash, "--squash", "--ff"). + Arg(extraArgs...). Arg(branchName). ToArgv() diff --git a/pkg/commands/git_commands/branch_test.go b/pkg/commands/git_commands/branch_test.go index 37ad79613..a0c0096b9 100644 --- a/pkg/commands/git_commands/branch_test.go +++ b/pkg/commands/git_commands/branch_test.go @@ -122,14 +122,14 @@ func TestBranchMerge(t *testing.T) { scenarios := []struct { testName string userConfig *config.UserConfig - opts MergeOpts + variant MergeVariant branchName string expected []string }{ { testName: "basic", userConfig: &config.UserConfig{}, - opts: MergeOpts{}, + variant: MERGE_VARIANT_REGULAR, branchName: "mybranch", expected: []string{"merge", "--no-edit", "mybranch"}, }, @@ -142,7 +142,7 @@ func TestBranchMerge(t *testing.T) { }, }, }, - opts: MergeOpts{}, + variant: MERGE_VARIANT_REGULAR, branchName: "mybranch", expected: []string{"merge", "--no-edit", "--merging-args", "mybranch"}, }, @@ -155,16 +155,30 @@ func TestBranchMerge(t *testing.T) { }, }, }, - opts: MergeOpts{}, + variant: MERGE_VARIANT_REGULAR, branchName: "mybranch", expected: []string{"merge", "--no-edit", "--arg1", "--arg2", "mybranch"}, }, { - testName: "fast forward only", + testName: "fast-forward merge", userConfig: &config.UserConfig{}, - opts: MergeOpts{FastForwardOnly: true}, + variant: MERGE_VARIANT_FAST_FORWARD, branchName: "mybranch", - expected: []string{"merge", "--no-edit", "--ff-only", "mybranch"}, + expected: []string{"merge", "--no-edit", "--ff", "mybranch"}, + }, + { + testName: "non-fast-forward merge", + userConfig: &config.UserConfig{}, + variant: MERGE_VARIANT_NON_FAST_FORWARD, + branchName: "mybranch", + expected: []string{"merge", "--no-edit", "--no-ff", "mybranch"}, + }, + { + testName: "squash merge", + userConfig: &config.UserConfig{}, + variant: MERGE_VARIANT_SQUASH, + branchName: "mybranch", + expected: []string{"merge", "--no-edit", "--squash", "--ff", "mybranch"}, }, } @@ -174,7 +188,7 @@ func TestBranchMerge(t *testing.T) { ExpectGitArgs(s.expected, "", nil) instance := buildBranchCommands(commonDeps{runner: runner, userConfig: s.userConfig}) - assert.NoError(t, instance.Merge(s.branchName, s.opts)) + assert.NoError(t, instance.Merge(s.branchName, s.variant)) runner.CheckForMissingCalls() }) } diff --git a/pkg/gui/controllers/helpers/merge_and_rebase_helper.go b/pkg/gui/controllers/helpers/merge_and_rebase_helper.go index 88166ec49..caddc77ce 100644 --- a/pkg/gui/controllers/helpers/merge_and_rebase_helper.go +++ b/pkg/gui/controllers/helpers/merge_and_rebase_helper.go @@ -426,7 +426,7 @@ func (self *MergeAndRebaseHelper) MergeRefIntoCheckedOutBranch(refName string) e func (self *MergeAndRebaseHelper) RegularMerge(refName string) func() error { return func() error { self.c.LogAction(self.c.Tr.Actions.Merge) - err := self.c.Git().Branch.Merge(refName, git_commands.MergeOpts{}) + err := self.c.Git().Branch.Merge(refName, git_commands.MERGE_VARIANT_REGULAR) return self.CheckMergeOrRebase(err) } } @@ -434,7 +434,7 @@ func (self *MergeAndRebaseHelper) RegularMerge(refName string) func() error { func (self *MergeAndRebaseHelper) SquashMergeUncommitted(refName string) func() error { return func() error { self.c.LogAction(self.c.Tr.Actions.SquashMerge) - err := self.c.Git().Branch.Merge(refName, git_commands.MergeOpts{Squash: true}) + err := self.c.Git().Branch.Merge(refName, git_commands.MERGE_VARIANT_SQUASH) return self.CheckMergeOrRebase(err) } } @@ -442,7 +442,7 @@ func (self *MergeAndRebaseHelper) SquashMergeUncommitted(refName string) func() func (self *MergeAndRebaseHelper) SquashMergeCommitted(refName, checkedOutBranchName string) func() error { return func() error { self.c.LogAction(self.c.Tr.Actions.SquashMerge) - err := self.c.Git().Branch.Merge(refName, git_commands.MergeOpts{Squash: true}) + err := self.c.Git().Branch.Merge(refName, git_commands.MERGE_VARIANT_SQUASH) if err = self.CheckMergeOrRebase(err); err != nil { return err }