fix issue where upstream origin and branch were quoted together

fix issue where upstream origin and branch were quoted together

4 files changed, 246 insertions, 36 deletions

diff --git a/pkg/commands/sync.go b/pkg/commands/sync.go
index 583987625..4fc7d73bb 100644
--- a/pkg/commands/sync.go
+++ b/pkg/commands/sync.go
@@ -3,27 +3,46 @@ package commands
 import (
 	"fmt"
 	"sync"
+
+	"github.com/go-errors/errors"
 )
 
 // Push pushes to a branch
-func (c *GitCommand) Push(branchName string, force bool, upstream string, args string, promptUserForCredential func(string) string) error {
-	followTagsFlag := "--follow-tags"
-	if c.GetConfigValue("push.followTags") == "false" {
-		followTagsFlag = ""
+type PushOpts struct {
+	Force                   bool
+	UpstreamRemote          string
+	UpstreamBranch          string
+	SetUpstream             bool
+	PromptUserForCredential func(string) string
+}
+
+func (c *GitCommand) Push(opts PushOpts) error {
+	cmd := "git push"
+
+	if c.GetConfigValue("push.followTags") != "false" {
+		cmd += " --follow-tags"
 	}
 
-	forceFlag := ""
-	if force {
-		forceFlag = "--force-with-lease"
+	if opts.Force {
+		cmd += " --force-with-lease"
 	}
 
-	setUpstreamArg := ""
-	if upstream != "" {
-		setUpstreamArg = "--set-upstream " + c.OSCommand.Quote(upstream)
+	if opts.SetUpstream {
+		cmd += " --set-upstream"
+	}
+
+	if opts.UpstreamRemote != "" {
+		cmd += " " + c.OSCommand.Quote(opts.UpstreamRemote)
+	}
+
+	if opts.UpstreamBranch != "" {
+		if opts.UpstreamRemote == "" {
+			return errors.New(c.Tr.MustSpecifyOriginError)
+		}
+		cmd += " " + c.OSCommand.Quote(opts.UpstreamBranch)
 	}
 
-	cmd := fmt.Sprintf("git push %s %s %s %s", followTagsFlag, forceFlag, setUpstreamArg, args)
-	return c.OSCommand.DetectUnamePass(cmd, promptUserForCredential)
+	return c.OSCommand.DetectUnamePass(cmd, opts.PromptUserForCredential)
 }
 
 type FetchOptions struct {
diff --git a/pkg/commands/sync_test.go b/pkg/commands/sync_test.go
index 9f7ed1b01..2f67f91fc 100644
--- a/pkg/commands/sync_test.go
+++ b/pkg/commands/sync_test.go
@@ -14,10 +14,14 @@ func TestGitCommandPush(t *testing.T) {
 		testName          string
 		getGitConfigValue func(string) (string, error)
 		command           func(string, ...string) *exec.Cmd
-		forcePush         bool
+		opts              PushOpts
 		test              func(error)
 	}
 
+	prompt := func(passOrUname string) string {
+		return "\n"
+	}
+
 	scenarios := []scenario{
 		{
 			"Push with force disabled, follow-tags on",
@@ -30,7 +34,7 @@ func TestGitCommandPush(t *testing.T) {
 
 				return secureexec.Command("echo")
 			},
-			false,
+			PushOpts{Force: false, PromptUserForCredential: prompt},
 			func(err error) {
 				assert.NoError(t, err)
 			},
@@ -46,7 +50,7 @@ func TestGitCommandPush(t *testing.T) {
 
 				return secureexec.Command("echo")
 			},
-			true,
+			PushOpts{Force: true, PromptUserForCredential: prompt},
 			func(err error) {
 				assert.NoError(t, err)
 			},
@@ -62,7 +66,7 @@ func TestGitCommandPush(t *testing.T) {
 
 				return secureexec.Command("echo")
 			},
-			false,
+			PushOpts{Force: false, PromptUserForCredential: prompt},
 			func(err error) {
 				assert.NoError(t, err)
 			},
@@ -77,11 +81,161 @@ func TestGitCommandPush(t *testing.T) {
 				assert.EqualValues(t, []string{"push", "--follow-tags"}, args)
 				return secureexec.Command("test")
 			},
-			false,
+			PushOpts{Force: false, PromptUserForCredential: prompt},
 			func(err error) {
 				assert.Error(t, err)
 			},
 		},
+		{
+			"Push with force disabled, follow-tags off, upstream supplied",
+			func(string) (string, error) {
+				return "false", nil
+			},
+			func(cmd string, args ...string) *exec.Cmd {
+				assert.EqualValues(t, "git", cmd)
+				assert.EqualValues(t, []string{"push", "origin", "master"}, args)
+
+				return secureexec.Command("echo")
+			},
+			PushOpts{
+				Force:                   false,
+				UpstreamRemote:          "origin",
+				UpstreamBranch:          "master",
+				PromptUserForCredential: prompt,
+			},
+			func(err error) {
+				assert.NoError(t, err)
+			},
+		},
+		{
+			"Push with force disabled, follow-tags off, setting upstream",
+			func(string) (string, error) {
+				return "false", nil
+			},
+			func(cmd string, args ...string) *exec.Cmd {
+				assert.EqualValues(t, "git", cmd)
+				assert.EqualValues(t, []string{"push", "--set-upstream", "origin", "master"}, args)
+
+				return secureexec.Command("echo")
+			},
+			PushOpts{
+				Force:                   false,
+				UpstreamRemote:          "origin",
+				UpstreamBranch:          "master",
+				PromptUserForCredential: prompt,
+				SetUpstream:             true,
+			},
+			func(err error) {
+				assert.NoError(t, err)
+			},
+		},
+		{
+			"Push with force enabled, follow-tags off, setting upstream",
+			func(string) (string, error) {
+				return "false", nil
+			},
+			func(cmd string, args ...string) *exec.Cmd {
+				assert.EqualValues(t, "git", cmd)
+				assert.EqualValues(t, []string{"push", "--force-with-lease", "--set-upstream", "origin", "master"}, args)
+
+				return secureexec.Command("echo")
+			},
+			PushOpts{
+				Force:                   true,
+				UpstreamRemote:          "origin",
+				UpstreamBranch:          "master",
+				PromptUserForCredential: prompt,
+				SetUpstream:             true,
+			},
+			func(err error) {
+				assert.NoError(t, err)
+			},
+		},
+		{
+			"Push with remote branch but no origin",
+			func(string) (string, error) {
+				return "false", nil
+			},
+			func(cmd string, args ...string) *exec.Cmd {
+				return nil
+			},
+			PushOpts{
+				Force:                   true,
+				UpstreamRemote:          "",
+				UpstreamBranch:          "master",
+				PromptUserForCredential: prompt,
+				SetUpstream:             true,
+			},
+			func(err error) {
+				assert.Error(t, err)
+				assert.EqualValues(t, "Must specify a remote if specifying a branch", err.Error())
+			},
+		},
+		{
+			"Push with force disabled, follow-tags off, upstream supplied",
+			func(string) (string, error) {
+				return "false", nil
+			},
+			func(cmd string, args ...string) *exec.Cmd {
+				assert.EqualValues(t, "git", cmd)
+				assert.EqualValues(t, []string{"push", "origin", "master"}, args)
+
+				return secureexec.Command("echo")
+			},
+			PushOpts{
+				Force:                   false,
+				UpstreamRemote:          "origin",
+				UpstreamBranch:          "master",
+				PromptUserForCredential: prompt,
+			},
+			func(err error) {
+				assert.NoError(t, err)
+			},
+		},
+		{
+			"Push with force disabled, follow-tags off, setting upstream",
+			func(string) (string, error) {
+				return "false", nil
+			},
+			func(cmd string, args ...string) *exec.Cmd {
+				assert.EqualValues(t, "git", cmd)
+				assert.EqualValues(t, []string{"push", "--set-upstream", "origin", "master"}, args)
+
+				return secureexec.Command("echo")
+			},
+			PushOpts{
+				Force:                   false,
+				UpstreamRemote:          "origin",
+				UpstreamBranch:          "master",
+				PromptUserForCredential: prompt,
+				SetUpstream:             true,
+			},
+			func(err error) {
+				assert.NoError(t, err)
+			},
+		},
+		{
+			"Push with force enabled, follow-tags off, setting upstream",
+			func(string) (string, error) {
+				return "false", nil
+			},
+			func(cmd string, args ...string) *exec.Cmd {
+				assert.EqualValues(t, "git", cmd)
+				assert.EqualValues(t, []string{"push", "--force-with-lease", "--set-upstream", "origin", "master"}, args)
+
+				return secureexec.Command("echo")
+			},
+			PushOpts{
+				Force:                   true,
+				UpstreamRemote:          "origin",
+				UpstreamBranch:          "master",
+				PromptUserForCredential: prompt,
+				SetUpstream:             true,
+			},
+			func(err error) {
+				assert.NoError(t, err)
+			},
+		},
 	}
 
 	for _, s := range scenarios {
@@ -89,9 +243,7 @@ func TestGitCommandPush(t *testing.T) {
 			gitCmd := NewDummyGitCommand()
 			gitCmd.OSCommand.Command = s.command
 			gitCmd.getGitConfigValue = s.getGitConfigValue
-			err := gitCmd.Push("test", s.forcePush, "", "", func(passOrUname string) string {
-				return "\n"
-			})
+			err := gitCmd.Push(s.opts)
 			s.test(err)
 		})
 	}
diff --git a/pkg/gui/files_panel.go b/pkg/gui/files_panel.go
index 708eae1f5..06452a358 100644
--- a/pkg/gui/files_panel.go
+++ b/pkg/gui/files_panel.go
@@ -730,14 +730,27 @@ func (gui *Gui) pullWithMode(mode string, opts PullFilesOptions) error {
 	}
 }
 
-func (gui *Gui) pushWithForceFlag(force bool, upstream string, args string) error {
+type pushOpts struct {
+	force          bool
+	upstreamRemote string
+	upstreamBranch string
+	setUpstream    bool
+}
+
+func (gui *Gui) push(opts pushOpts) error {
 	if err := gui.createLoaderPanel(gui.Tr.PushWait); err != nil {
 		return err
 	}
 	go utils.Safe(func() {
-		branchName := gui.getCheckedOutBranch().Name
-		err := gui.GitCommand.WithSpan(gui.Tr.Spans.Push).Push(branchName, force, upstream, args, gui.promptUserForCredential)
-		if err != nil && !force && strings.Contains(err.Error(), "Updates were rejected") {
+		err := gui.GitCommand.WithSpan(gui.Tr.Spans.Push).Push(commands.PushOpts{
+			Force:                   opts.force,
+			UpstreamRemote:          opts.upstreamRemote,
+			UpstreamBranch:          opts.upstreamBranch,
+			SetUpstream:             opts.setUpstream,
+			PromptUserForCredential: gui.promptUserForCredential,
+		})
+
+		if err != nil && !opts.force && strings.Contains(err.Error(), "Updates were rejected") {
 			forcePushDisabled := gui.Config.GetUserConfig().Git.DisableForcePushing
 			if forcePushDisabled {
 				_ = gui.createErrorPanel(gui.Tr.UpdatesRejectedAndForcePushDisabled)
@@ -747,7 +760,10 @@ func (gui *Gui) pushWithForceFlag(force bool, upstream string, args string) erro
 				title:  gui.Tr.ForcePush,
 				prompt: gui.Tr.ForcePushPrompt,
 				handleConfirm: func() error {
-					return gui.pushWithForceFlag(true, upstream, args)
+					newOpts := opts
+					newOpts.force = true
+
+					return gui.push(newOpts)
 				},
 			})
 			return
@@ -774,27 +790,48 @@ func (gui *Gui) pushFiles() error {
 		if currentBranch.HasCommitsToPull() {
 			return gui.requestToForcePush()
 		} else {
-			return gui.pushWithForceFlag(false, "", "")
+			return gui.push(pushOpts{})
 		}
 	} else {
 		// see if we have an upstream for this branch in our config
-		upstream, err := gui.upstreamForBranchInConfig(currentBranch.Name)
+		upstreamRemote, upstreamBranch, err := gui.upstreamForBranchInConfig(currentBranch.Name)
 		if err != nil {
 			return gui.surfaceError(err)
 		}
 
-		if upstream != "" {
-			return gui.pushWithForceFlag(false, "", upstream)
+		if upstreamBranch != "" {
+			return gui.push(
+				pushOpts{
+					force:          false,
+					upstreamRemote: upstreamRemote,
+					upstreamBranch: upstreamBranch,
+				},
+			)
 		}
 
 		if gui.GitCommand.PushToCurrent {
-			return gui.pushWithForceFlag(false, "", "--set-upstream")
+			return gui.push(pushOpts{setUpstream: true})
 		} else {
 			return gui.prompt(promptOpts{
 				title:          gui.Tr.EnterUpstream,
 				initialContent: "origin " + currentBranch.Name,
 				handleConfirm: func(upstream string) error {
-					return gui.pushWithForceFlag(false, upstream, "")
+					var upstreamBranch, upstreamRemote string
+					split := strings.Split(upstream, " ")
+					if len(split) == 2 {
+						upstreamRemote = split[0]
+						upstreamBranch = split[1]
+					} else {
+						upstreamRemote = upstream
+						upstreamBranch = ""
+					}
+
+					return gui.push(pushOpts{
+						force:          false,
+						upstreamRemote: upstreamRemote,
+						upstreamBranch: upstreamBranch,
+						setUpstream:    true,
+					})
 				},
 			})
 		}
@@ -811,24 +848,24 @@ func (gui *Gui) requestToForcePush() error {
 		title:  gui.Tr.ForcePush,
 		prompt: gui.Tr.ForcePushPrompt,
 		handleConfirm: func() error {
-			return gui.pushWithForceFlag(true, "", "")
+			return gui.push(pushOpts{force: true})
 		},
 	})
 }
 
-func (gui *Gui) upstreamForBranchInConfig(branchName string) (string, error) {
+func (gui *Gui) upstreamForBranchInConfig(branchName string) (string, string, error) {
 	conf, err := gui.GitCommand.Repo.Config()
 	if err != nil {
-		return "", err
+		return "", "", err
 	}
 
 	for configBranchName, configBranch := range conf.Branches {
 		if configBranchName == branchName {
-			return fmt.Sprintf("%s %s", configBranch.Remote, configBranchName), nil
+			return configBranch.Remote, configBranchName, nil
 		}
 	}
 
-	return "", nil
+	return "", "", nil
 }
 
 func (gui *Gui) handleSwitchToMerge() error {
diff --git a/pkg/i18n/english.go b/pkg/i18n/english.go
index e3712b7b5..50508d63a 100644
--- a/pkg/i18n/english.go
+++ b/pkg/i18n/english.go
@@ -431,6 +431,7 @@ type TranslationSet struct {
 	SelectConfigFile                    string
 	NoConfigFileFoundErr                string
 	LcLoadingFileSuggestions            string
+	MustSpecifyOriginError              string
 	Spans                               Spans
 }
 
@@ -956,6 +957,7 @@ func englishTranslationSet() TranslationSet {
 		SelectConfigFile:                    "Select config file",
 		NoConfigFileFoundErr:                "No config file found",
 		LcLoadingFileSuggestions:            "loading file suggestions",
+		MustSpecifyOriginError:              "Must specify a remote if specifying a branch",
 		Spans: Spans{
 			// TODO: combine this with the original keybinding descriptions (those are all in lowercase atm)
 			CheckoutCommit:                    "Checkout commit",