Add convenience builder for git commands

Adds a convenience builder for git commands. Applies it in a couple of places that benefit from it.

I've got a larger PR that does a more sweeping change but I want some reviews on that before I go ahead with it.

pkg/commands/git_commands/branch_test.go

@@ -5,6 +5,7 @@ import (
 
 	"github.com/go-errors/errors"
 	"github.com/jesseduffield/lazygit/pkg/commands/oscommands"
+	"github.com/jesseduffield/lazygit/pkg/config"
 	"github.com/stretchr/testify/assert"
 )
 
@@ -99,12 +100,53 @@ func TestBranchDeleteBranch(t *testing.T) {
 }
 
 func TestBranchMerge(t *testing.T) {
-	runner := oscommands.NewFakeRunner(t).
+	scenarios := []struct {
-		Expect(`git merge --no-edit "test"`, "", nil)
+		testName   string
-	instance := buildBranchCommands(commonDeps{runner: runner})
+		userConfig *config.UserConfig
+		opts       MergeOpts
+		branchName string
+		expected   string
+	}{
+		{
+			testName:   "basic",
+			userConfig: &config.UserConfig{},
+			opts:       MergeOpts{},
+			branchName: "mybranch",
+			expected:   `git merge --no-edit "mybranch"`,
+		},
+		{
+			testName: "merging args",
+			userConfig: &config.UserConfig{
+				Git: config.GitConfig{
+					Merging: config.MergingConfig{
+						Args: "--merging-args", // it's up to the user what they put here
+					},
+				},
+			},
+			opts:       MergeOpts{},
+			branchName: "mybranch",
+			expected:   `git merge --no-edit --merging-args "mybranch"`,
+		},
+		{
+			testName:   "fast forward only",
+			userConfig: &config.UserConfig{},
+			opts:       MergeOpts{FastForwardOnly: true},
+			branchName: "mybranch",
+			expected:   `git merge --no-edit --ff-only "mybranch"`,
+		},
+	}
 
-	assert.NoError(t, instance.Merge("test", MergeOpts{}))
+	for _, s := range scenarios {
-	runner.CheckForMissingCalls()
+		s := s
+		t.Run(s.testName, func(t *testing.T) {
+			runner := oscommands.NewFakeRunner(t).
+				Expect(s.expected, "", nil)
+			instance := buildBranchCommands(commonDeps{runner: runner, userConfig: s.userConfig})
+
+			assert.NoError(t, instance.Merge(s.branchName, s.opts))
+			runner.CheckForMissingCalls()
+		})
+	}
 }
 
 func TestBranchCheckout(t *testing.T) {

pkg/commands/git_commands/commit_file_loader.go

@@ -1,7 +1,6 @@
 package git_commands
 
 import (
-	"fmt"
 	"strings"
 
 	"github.com/jesseduffield/generics/slices"
@@ -25,12 +24,18 @@ func NewCommitFileLoader(common *common.Common, cmd oscommands.ICmdObjBuilder) *
 
 // GetFilesInDiff get the specified commit files
 func (self *CommitFileLoader) GetFilesInDiff(from string, to string, reverse bool) ([]*models.CommitFile, error) {
-	reverseFlag := ""
+	cmdStr := NewGitCmd("diff").
-	if reverse {
+		Arg("--submodule").
-		reverseFlag = " -R "
+		Arg("--no-ext-diff").
-	}
+		Arg("--name-status").
+		Arg("-z").
+		Arg("--no-renames").
+		ArgIf(reverse, "-R").
+		Arg(from).
+		Arg(to).
+		ToString()
 
-	filenames, err := self.cmd.New(fmt.Sprintf("git diff --submodule --no-ext-diff --name-status -z --no-renames %s %s %s", reverseFlag, from, to)).DontLog().RunWithOutput()
+	filenames, err := self.cmd.New(cmdStr).DontLog().RunWithOutput()
 	if err != nil {
 		return nil, err
 	}

pkg/commands/git_commands/commit_loader.go

@@ -201,12 +201,14 @@ func (self *CommitLoader) getHydratedRebasingCommits(rebaseMode enums.RebaseMode
 	// note that we're not filtering these as we do non-rebasing commits just because
 	// I suspect that will cause some damage
 	cmdObj := self.cmd.New(
-		fmt.Sprintf(
+		NewGitCmd("show").
-			"git -c log.showSignature=false show %s --no-patch --oneline %s --abbrev=%d",
+			Config("log.showSignature=false").
-			strings.Join(commitShas, " "),
+			Arg("--no-patch").
-			prettyFormat,
+			Arg("--oneline").
-			20,
+			Arg(strings.Join(commitShas, " ")).
-		),
+			Arg(prettyFormat).
+			Arg("--abbrev=20").
+			ToString(),
 	).DontLog()
 
 	fullCommits := map[string]*models.Commit{}
@@ -375,8 +377,13 @@ func (self *CommitLoader) getMergeBase(refName string) string {
 
 	// We pass all configured main branches to the merge-base call; git will
 	// return the base commit for the closest one.
-	output, err := self.cmd.New(fmt.Sprintf("git merge-base %s %s",
+
-		self.cmd.Quote(refName), *self.quotedMainBranches)).DontLog().RunWithOutput()
+	output, err := self.cmd.New(
+		NewGitCmd("merge-base").
+			Arg(self.cmd.Quote(refName)).
+			Arg(*self.quotedMainBranches).
+			ToString(),
+	).DontLog().RunWithOutput()
 	if err != nil {
 		// If there's an error, it must be because one of the main branches that
 		// used to exist when we called getExistingMainBranches() was deleted
@@ -391,7 +398,9 @@ func (self *CommitLoader) getExistingMainBranches() string {
 		lo.FilterMap(self.UserConfig.Git.MainBranches,
 			func(branchName string, _ int) (string, bool) {
 				quotedRef := self.cmd.Quote("refs/heads/" + branchName)
-				if err := self.cmd.New(fmt.Sprintf("git rev-parse --verify --quiet %s", quotedRef)).DontLog().Run(); err != nil {
+				if err := self.cmd.New(
+					NewGitCmd("rev-parse").Arg("--verify").Arg("--quiet").Arg(quotedRef).ToString(),
+				).DontLog().Run(); err != nil {
 					return "", false
 				}
 				return quotedRef, true
@@ -413,9 +422,10 @@ func ignoringWarnings(commandOutput string) string {
 func (self *CommitLoader) getFirstPushedCommit(refName string) (string, error) {
 	output, err := self.cmd.
 		New(
-			fmt.Sprintf("git merge-base %s %s@{u}",
+			NewGitCmd("merge-base").
-				self.cmd.Quote(refName),
+				Arg(self.cmd.Quote(refName)).
-				self.cmd.Quote(strings.TrimPrefix(refName, "refs/heads/"))),
+				Arg(self.cmd.Quote(strings.TrimPrefix(refName, "refs/heads/")) + "@{u}").
+				ToString(),
 		).
 		DontLog().
 		RunWithOutput()
@@ -428,42 +438,22 @@ func (self *CommitLoader) getFirstPushedCommit(refName string) (string, error) {
 
 // getLog gets the git log.
 func (self *CommitLoader) getLogCmd(opts GetCommitsOptions) oscommands.ICmdObj {
-	limitFlag := ""
-	if opts.Limit {
-		limitFlag = " -300"
-	}
-
-	followFlag := ""
-	filterFlag := ""
-	if opts.FilterPath != "" {
-		followFlag = " --follow"
-		filterFlag = fmt.Sprintf(" %s", self.cmd.Quote(opts.FilterPath))
-	}
-
 	config := self.UserConfig.Git.Log
 
-	orderFlag := ""
+	cmdStr := NewGitCmd("log").
-	if config.Order != "default" {
+		Arg(self.cmd.Quote(opts.RefName)).
-		orderFlag = " --" + config.Order
+		ArgIf(config.Order != "default", "--"+config.Order).
-	}
+		ArgIf(opts.All, "--all").
-	allFlag := ""
+		Arg("--oneline").
-	if opts.All {
+		Arg(prettyFormat).
-		allFlag = " --all"
+		Arg("--abbrev=40").
-	}
+		ArgIf(opts.Limit, "-300").
+		ArgIf(opts.FilterPath != "", "--follow").
+		ArgIf(opts.FilterPath != "", self.cmd.Quote(opts.FilterPath)).
+		Arg("--no-show-signature").
+		ToString()
 
-	return self.cmd.New(
+	return self.cmd.New(cmdStr)
-		fmt.Sprintf(
-			"git log %s%s%s --oneline %s%s --abbrev=%d%s --no-show-signature --%s",
-			self.cmd.Quote(opts.RefName),
-			orderFlag,
-			allFlag,
-			prettyFormat,
-			limitFlag,
-			40,
-			followFlag,
-			filterFlag,
-		),
-	).DontLog()
 }
 
 const prettyFormat = `--pretty=format:"%H%x00%at%x00%aN%x00%ae%x00%d%x00%p%x00%s"`

pkg/commands/git_commands/deps_test.go

@@ -150,3 +150,9 @@ func buildBranchCommands(deps commonDeps) *BranchCommands {
 
 	return NewBranchCommands(gitCommon)
 }
+
+func buildFlowCommands(deps commonDeps) *FlowCommands {
+	gitCommon := buildGitCommon(deps)
+
+	return NewFlowCommands(gitCommon)
+}

pkg/commands/git_commands/flow_test.go

@@ -0,0 +1,92 @@
+package git_commands
+
+import (
+	"testing"
+
+	"github.com/jesseduffield/lazygit/pkg/commands/git_config"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestStartCmdObj(t *testing.T) {
+	scenarios := []struct {
+		testName   string
+		branchType string
+		name       string
+		expected   string
+	}{
+		{
+			testName:   "basic",
+			branchType: "feature",
+			name:       "test",
+			expected:   "git flow feature start test",
+		},
+	}
+
+	for _, s := range scenarios {
+		s := s
+		t.Run(s.testName, func(t *testing.T) {
+			instance := buildFlowCommands(commonDeps{})
+
+			assert.Equal(t,
+				instance.StartCmdObj(s.branchType, s.name).ToString(),
+				s.expected,
+			)
+		})
+	}
+}
+
+func TestFinishCmdObj(t *testing.T) {
+	scenarios := []struct {
+		testName               string
+		branchName             string
+		expected               string
+		expectedError          string
+		gitConfigMockResponses map[string]string
+	}{
+		{
+			testName:               "not a git flow branch",
+			branchName:             "mybranch",
+			expected:               "",
+			expectedError:          "This does not seem to be a git flow branch",
+			gitConfigMockResponses: nil,
+		},
+		{
+			testName:               "feature branch without config",
+			branchName:             "feature/mybranch",
+			expected:               "",
+			expectedError:          "This does not seem to be a git flow branch",
+			gitConfigMockResponses: nil,
+		},
+		{
+			testName:      "feature branch with config",
+			branchName:    "feature/mybranch",
+			expected:      "git flow feature finish mybranch",
+			expectedError: "",
+			gitConfigMockResponses: map[string]string{
+				"--local --get-regexp gitflow.prefix": "gitflow.prefix.feature feature/",
+			},
+		},
+	}
+
+	for _, s := range scenarios {
+		s := s
+		t.Run(s.testName, func(t *testing.T) {
+			instance := buildFlowCommands(commonDeps{
+				gitConfig: git_config.NewFakeGitConfig(s.gitConfigMockResponses),
+			})
+
+			cmd, err := instance.FinishCmdObj(s.branchName)
+
+			if s.expectedError != "" {
+				if err == nil {
+					t.Errorf("Expected error, got nil")
+				} else {
+					assert.Equal(t, err.Error(), s.expectedError)
+				}
+			} else {
+				assert.NoError(t, err)
+				assert.Equal(t, cmd.ToString(), s.expected)
+			}
+		})
+	}
+}

pkg/commands/git_commands/git_command_builder.go

@@ -0,0 +1,65 @@
+package git_commands
+
+import "fmt"
+
+// convenience struct for building git commands. Especially useful when
+// including conditional args
+type GitCommandBuilder struct {
+	command string
+}
+
+func NewGitCmd(command string) *GitCommandBuilder {
+	return &GitCommandBuilder{command: command}
+}
+
+func (self *GitCommandBuilder) Arg(flag string) *GitCommandBuilder {
+	if flag == "" {
+		return self
+	}
+
+	self.command += " " + flag
+
+	return self
+}
+
+func (self *GitCommandBuilder) ArgIf(include bool, flag string) *GitCommandBuilder {
+	if include {
+		return self.Arg(flag)
+	}
+
+	return self
+}
+
+func (self *GitCommandBuilder) ArgIfElse(isTrue bool, onTrue string, onFalse string) *GitCommandBuilder {
+	if isTrue {
+		return self.Arg(onTrue)
+	} else {
+		return self.Arg(onFalse)
+	}
+}
+
+func (self *GitCommandBuilder) Args(args []string) *GitCommandBuilder {
+	for _, arg := range args {
+		self.Arg(arg)
+	}
+
+	return self
+}
+
+func (self *GitCommandBuilder) Config(value string) *GitCommandBuilder {
+	// config settings come before the command
+	self.command = fmt.Sprintf("-c %s %s", value, self.command)
+
+	return self
+}
+
+func (self *GitCommandBuilder) RepoPath(value string) *GitCommandBuilder {
+	// repo path comes before the command
+	self.command = fmt.Sprintf("-C %s %s", value, self.command)
+
+	return self
+}
+
+func (self *GitCommandBuilder) ToString() string {
+	return "git " + self.command
+}

pkg/commands/git_commands/git_command_builder_test.go

@@ -0,0 +1,56 @@
+package git_commands
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestGitCommandBuilder(t *testing.T) {
+	scenarios := []struct {
+		input    string
+		expected string
+	}{
+		{
+			input: NewGitCmd("push").
+				Arg("--force-with-lease").
+				Arg("--set-upstream").
+				Arg("origin").
+				Arg("master").
+				ToString(),
+			expected: "git push --force-with-lease --set-upstream origin master",
+		},
+		{
+			input:    NewGitCmd("push").ArgIf(true, "--test").ToString(),
+			expected: "git push --test",
+		},
+		{
+			input:    NewGitCmd("push").ArgIf(false, "--test").ToString(),
+			expected: "git push",
+		},
+		{
+			input:    NewGitCmd("push").ArgIfElse(true, "-b", "-a").ToString(),
+			expected: "git push -b",
+		},
+		{
+			input:    NewGitCmd("push").ArgIfElse(false, "-a", "-b").ToString(),
+			expected: "git push -b",
+		},
+		{
+			input:    NewGitCmd("push").Args([]string{"-a", "-b"}).ToString(),
+			expected: "git push -a -b",
+		},
+		{
+			input:    NewGitCmd("push").Config("user.name=foo").Config("user.email=bar").ToString(),
+			expected: "git -c user.email=bar -c user.name=foo push",
+		},
+		{
+			input:    NewGitCmd("push").RepoPath("a/b/c").ToString(),
+			expected: "git -C a/b/c push",
+		},
+	}
+
+	for _, s := range scenarios {
+		assert.Equal(t, s.input, s.expected)
+	}
+}