refactor

6 files changed, 58 insertions, 87 deletions

diff --git a/pkg/commands/oscommands/cmd_obj.go b/pkg/commands/oscommands/cmd_obj.go
index 192871af0..8fb7fa313 100644
--- a/pkg/commands/oscommands/cmd_obj.go
+++ b/pkg/commands/oscommands/cmd_obj.go
@@ -12,6 +12,7 @@ type ICmdObj interface {
 	// using NewFromArgs, the output won't be quite the same as what you would type
 	// into a terminal e.g. 'sh -c git commit' as opposed to 'sh -c "git commit"'
 	ToString() string
+
 	AddEnvVars(...string) ICmdObj
 	GetEnvVars() []string
 
@@ -22,9 +23,16 @@ type ICmdObj interface {
 	// runs the command and runs a callback function on each line of the output. If the callback returns true for the boolean value, we kill the process and return.
 	RunAndProcessLines(onLine func(line string) (bool, error)) error
 
-	// Marks the command object as readonly, so that when it is run, we don't log it to the user.
-	// We only want to log commands to the user which change state in some way.
+	// Be calling DontLog(), we're saying that once we call Run(), we don't want to
+	// log the command in the UI (it'll still be logged in the log file). The general rule
+	// is that if a command doesn't change the git state (e.g. read commands like `git diff`)
+	// then we don't want to log it. If we are changing something (e.g. `git add .`) then
+	// we do. The only exception is if we're running a command in the background periodically
+	// like `git fetch`, which technically does mutate stuff but isn't something we need
+	// to notify the user about.
 	DontLog() ICmdObj
+
+	// This returns false if DontLog() was called
 	ShouldLog() bool
 }
 
diff --git a/pkg/commands/oscommands/cmd_obj_builder.go b/pkg/commands/oscommands/cmd_obj_builder.go
index 5929d6faf..a493ae651 100644
--- a/pkg/commands/oscommands/cmd_obj_builder.go
+++ b/pkg/commands/oscommands/cmd_obj_builder.go
@@ -21,9 +21,8 @@ type ICmdObjBuilder interface {
 }
 
 type CmdObjBuilder struct {
-	runner    ICmdObjRunner
-	logCmdObj func(ICmdObj)
-	platform  *Platform
+	runner   ICmdObjRunner
+	platform *Platform
 }
 
 // poor man's version of explicitly saying that struct X implements interface Y
@@ -76,8 +75,27 @@ func (self *CmdObjBuilder) CloneWithNewRunner(decorate func(ICmdObjRunner) ICmdO
 	decoratedRunner := decorate(self.runner)
 
 	return &CmdObjBuilder{
-		runner:    decoratedRunner,
-		logCmdObj: self.logCmdObj,
-		platform:  self.platform,
+		runner:   decoratedRunner,
+		platform: self.platform,
 	}
 }
+
+func (self *CmdObjBuilder) Quote(message string) string {
+	var quote string
+	if self.platform.OS == "windows" {
+		quote = `\"`
+		message = strings.NewReplacer(
+			`"`, `"'"'"`,
+			`\"`, `\\"`,
+		).Replace(message)
+	} else {
+		quote = `"`
+		message = strings.NewReplacer(
+			`\`, `\\`,
+			`"`, `\"`,
+			`$`, `\$`,
+			"`", "\\`",
+		).Replace(message)
+	}
+	return quote + message + quote
+}
diff --git a/pkg/commands/oscommands/cmd_obj_runner.go b/pkg/commands/oscommands/cmd_obj_runner.go
index 0c6bf086e..8eb6d437d 100644
--- a/pkg/commands/oscommands/cmd_obj_runner.go
+++ b/pkg/commands/oscommands/cmd_obj_runner.go
@@ -22,10 +22,6 @@ type cmdObjRunner struct {
 var _ ICmdObjRunner = &cmdObjRunner{}
 
 func (self *cmdObjRunner) Run(cmdObj ICmdObj) error {
-	if cmdObj.ShouldLog() {
-		self.logCmdObj(cmdObj)
-	}
-
 	_, err := self.RunWithOutput(cmdObj)
 	return err
 }
diff --git a/pkg/commands/oscommands/dummies.go b/pkg/commands/oscommands/dummies.go
index f44cc9b72..8a97589ed 100644
--- a/pkg/commands/oscommands/dummies.go
+++ b/pkg/commands/oscommands/dummies.go
@@ -13,9 +13,8 @@ func NewDummyOSCommand() *OSCommand {
 
 func NewDummyCmdObjBuilder(runner ICmdObjRunner) *CmdObjBuilder {
 	return &CmdObjBuilder{
-		runner:    runner,
-		logCmdObj: func(ICmdObj) {},
-		platform:  dummyPlatform,
+		runner:   runner,
+		platform: dummyPlatform,
 	}
 }
 
diff --git a/pkg/commands/oscommands/os.go b/pkg/commands/oscommands/os.go
index 0d2a936ef..bf1bb9ac6 100644
--- a/pkg/commands/oscommands/os.go
+++ b/pkg/commands/oscommands/os.go
@@ -22,11 +22,11 @@ type OSCommand struct {
 	Platform *Platform
 	Getenv   func(string) string
 
-	// callback to run before running a command, i.e. for the purposes of logging
-	onRunCommand func(CmdLogEntry)
-
-	// something like 'Staging File': allows us to group cmd logs under a single title
-	CmdLogSpan string
+	// callback to run before running a command, i.e. for the purposes of logging.
+	// the string argument is the command string e.g. 'git add .' and the bool is
+	// whether we're dealing with a command line command or something more general
+	// like 'Opening PR URL', or something handled by Go's standard library.
+	logCommandFn func(string, bool)
 
 	removeFile func(string) error
 
@@ -42,36 +42,6 @@ type Platform struct {
 	OpenLinkCommand string
 }
 
-// TODO: make these fields private
-type CmdLogEntry struct {
-	// e.g. 'git commit -m "haha"'
-	cmdStr string
-	// Span is something like 'Staging File'. Multiple commands can be grouped under the same
-	// span
-	span string
-
-	// sometimes our command is direct like 'git commit', and sometimes it's a
-	// command to remove a file but through Go's standard library rather than the
-	// command line
-	commandLine bool
-}
-
-func (e CmdLogEntry) GetCmdStr() string {
-	return e.cmdStr
-}
-
-func (e CmdLogEntry) GetSpan() string {
-	return e.span
-}
-
-func (e CmdLogEntry) GetCommandLine() bool {
-	return e.commandLine
-}
-
-func NewCmdLogEntry(cmdStr string, span string, commandLine bool) CmdLogEntry {
-	return CmdLogEntry{cmdStr: cmdStr, span: span, commandLine: commandLine}
-}
-
 // NewOSCommand os command runner
 func NewOSCommand(common *common.Common, platform *Platform) *OSCommand {
 	c := &OSCommand{
@@ -82,7 +52,7 @@ func NewOSCommand(common *common.Common, platform *Platform) *OSCommand {
 	}
 
 	runner := &cmdObjRunner{log: common.Log, logCmdObj: c.LogCmdObj}
-	c.Cmd = &CmdObjBuilder{runner: runner, logCmdObj: c.LogCmdObj, platform: platform}
+	c.Cmd = &CmdObjBuilder{runner: runner, platform: platform}
 
 	return c
 }
@@ -94,13 +64,13 @@ func (c *OSCommand) LogCmdObj(cmdObj ICmdObj) {
 func (c *OSCommand) LogCommand(cmdStr string, commandLine bool) {
 	c.Log.WithField("command", cmdStr).Info("RunCommand")
 
-	if c.onRunCommand != nil {
-		c.onRunCommand(NewCmdLogEntry(cmdStr, c.CmdLogSpan, commandLine))
+	if c.logCommandFn != nil {
+		c.logCommandFn(cmdStr, commandLine)
 	}
 }
 
-func (c *OSCommand) SetOnRunCommand(f func(CmdLogEntry)) {
-	c.onRunCommand = f
+func (c *OSCommand) SetLogCommandFn(f func(string, bool)) {
+	c.logCommandFn = f
 }
 
 // To be used for testing only
@@ -145,26 +115,6 @@ func (c *OSCommand) Quote(message string) string {
 	return c.Cmd.Quote(message)
 }
 
-func (self *CmdObjBuilder) Quote(message string) string {
-	var quote string
-	if self.platform.OS == "windows" {
-		quote = `\"`
-		message = strings.NewReplacer(
-			`"`, `"'"'"`,
-			`\"`, `\\"`,
-		).Replace(message)
-	} else {
-		quote = `"`
-		message = strings.NewReplacer(
-			`\`, `\\`,
-			`"`, `\"`,
-			`$`, `\$`,
-			"`", "\\`",
-		).Replace(message)
-	}
-	return quote + message + quote
-}
-
 // AppendLineToFile adds a new line in file
 func (c *OSCommand) AppendLineToFile(filename, line string) error {
 	c.LogCommand(fmt.Sprintf("Appending '%s' to file '%s'", line, filename), false)
@@ -236,15 +186,6 @@ func (c *OSCommand) FileExists(path string) (bool, error) {
 	return true, nil
 }
 
-// GetLazygitPath returns the path of the currently executed file
-func (c *OSCommand) GetLazygitPath() string {
-	ex, err := os.Executable() // get the executable path for git to use
-	if err != nil {
-		ex = os.Args[0] // fallback to the first call argument if needed
-	}
-	return `"` + filepath.ToSlash(ex) + `"`
-}
-
 // PipeCommands runs a heap of commands and pipes their inputs/outputs together like A | B | C
 func (c *OSCommand) PipeCommands(commandStrings ...string) error {
 	cmds := make([]*exec.Cmd, len(commandStrings))
@@ -333,3 +274,12 @@ func (c *OSCommand) RemoveFile(path string) error {
 func GetTempDir() string {
 	return filepath.Join(os.TempDir(), "lazygit")
 }
+
+// GetLazygitPath returns the path of the currently executed file
+func GetLazygitPath() string {
+	ex, err := os.Executable() // get the executable path for git to use
+	if err != nil {
+		ex = os.Args[0] // fallback to the first call argument if needed
+	}
+	return `"` + filepath.ToSlash(ex) + `"`
+}
diff --git a/pkg/commands/rebasing.go b/pkg/commands/rebasing.go
index 8827246ab..b930604ea 100644
--- a/pkg/commands/rebasing.go
+++ b/pkg/commands/rebasing.go
@@ -59,7 +59,7 @@ func (c *GitCommand) InteractiveRebase(commits []*models.Commit, index int, acti
 // we tell git to run lazygit to edit the todo list, and we pass the client
 // lazygit a todo string to write to the todo file
 func (c *GitCommand) PrepareInteractiveRebaseCommand(baseSha string, todo string, overrideEditor bool) (oscommands.ICmdObj, error) {
-	ex := c.OSCommand.GetLazygitPath()
+	ex := oscommands.GetLazygitPath()
 
 	debug := "FALSE"
 	if c.Debug {
@@ -267,8 +267,8 @@ func (c *GitCommand) GenericMergeOrRebaseAction(commandType string, command stri
 }
 
 func (c *GitCommand) runSkipEditorCommand(command string) error {
-	cmdObj := c.OSCommand.Cmd.New(command)
-	lazyGitPath := c.OSCommand.GetLazygitPath()
+	cmdObj := c.Cmd.New(command)
+	lazyGitPath := oscommands.GetLazygitPath()
 	return cmdObj.
 		AddEnvVars(
 			"LAZYGIT_CLIENT_COMMAND=EXIT_IMMEDIATELY",