-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow user to select remote and branch when creating a PR #1889
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -730,11 +730,23 @@ func (self *BranchesController) createPullRequestMenu(selectedBranch *models.Bra | |
{ | ||
LabelColumns: fromToLabelColumns(branch.Name, self.c.Tr.SelectBranch), | ||
OnPress: func() error { | ||
if !branch.IsTrackingRemote() { | ||
return errors.New(self.c.Tr.PullRequestNoUpstream) | ||
} | ||
|
||
if len(self.c.Model().Remotes) == 1 { | ||
toRemote := self.c.Model().Remotes[0].Name | ||
self.c.Log.Debugf("PR will target the only existing remote '%s'", toRemote) | ||
return self.promptForTargetBranchNameAndCreatePullRequest(branch, toRemote) | ||
} | ||
|
||
self.c.Prompt(types.PromptOpts{ | ||
Title: branch.Name + " →", | ||
FindSuggestionsFunc: self.c.Helpers().Suggestions.GetRemoteBranchesSuggestionsFunc("/"), | ||
HandleConfirm: func(targetBranchName string) error { | ||
return self.createPullRequest(branch.Name, targetBranchName) | ||
Title: self.c.Tr.SelectTargetRemote, | ||
FindSuggestionsFunc: self.c.Helpers().Suggestions.GetRemoteSuggestionsFunc(), | ||
HandleConfirm: func(toRemote string) error { | ||
self.c.Log.Debugf("PR will target remote '%s'", toRemote) | ||
|
||
return self.promptForTargetBranchNameAndCreatePullRequest(branch, toRemote) | ||
}, | ||
}) | ||
|
||
|
@@ -764,6 +776,26 @@ func (self *BranchesController) createPullRequestMenu(selectedBranch *models.Bra | |
return self.c.Menu(types.CreateMenuOptions{Title: fmt.Sprint(self.c.Tr.CreatePullRequestOptions), Items: menuItems}) | ||
} | ||
|
||
func (self *BranchesController) promptForTargetBranchNameAndCreatePullRequest(fromBranch *models.Branch, toRemote string) error { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If an error can't be returned from this function then let's just remove the |
||
remoteDoesNotExist := lo.NoneBy(self.c.Model().Remotes, func(remote *models.Remote) bool { | ||
return remote.Name == toRemote | ||
}) | ||
if remoteDoesNotExist { | ||
return fmt.Errorf(self.c.Tr.NoValidRemoteName, toRemote) | ||
} | ||
|
||
self.c.Prompt(types.PromptOpts{ | ||
Title: fmt.Sprintf("%s → %s/", fromBranch.UpstreamBranch, toRemote), | ||
FindSuggestionsFunc: self.c.Helpers().Suggestions.GetRemoteBranchesForRemoteSuggestionsFunc(toRemote), | ||
HandleConfirm: func(toBranch string) error { | ||
self.c.Log.Debugf("PR will target branch '%s' on remote '%s'", toBranch, toRemote) | ||
return self.createPullRequest(fromBranch.UpstreamBranch, toBranch) | ||
}, | ||
}) | ||
|
||
return nil | ||
} | ||
|
||
func (self *BranchesController) createPullRequest(from string, to string) error { | ||
url, err := self.c.Helpers().Host.GetPullRequestURL(from, to) | ||
if err != nil { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -162,10 +162,26 @@ func (self *SuggestionsHelper) getRemoteBranchNames(separator string) []string { | |
}) | ||
} | ||
|
||
func (self *SuggestionsHelper) getRemoteBranchNamesForRemote(remoteName string) []string { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's remove the functions here that are not being called from anywhere. |
||
remote, ok := lo.Find(self.c.Model().Remotes, func(remote *models.Remote) bool { | ||
return remote.Name == remoteName | ||
}) | ||
if ok { | ||
return lo.Map(remote.Branches, func(branch *models.RemoteBranch, _ int) string { | ||
return branch.Name | ||
jesseduffield marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}) | ||
} | ||
return nil | ||
} | ||
|
||
func (self *SuggestionsHelper) GetRemoteBranchesSuggestionsFunc(separator string) func(string) []*types.Suggestion { | ||
return FilterFunc(self.getRemoteBranchNames(separator), self.c.UserConfig().Gui.UseFuzzySearch()) | ||
} | ||
|
||
func (self *SuggestionsHelper) GetRemoteBranchesForRemoteSuggestionsFunc(remoteName string) func(string) []*types.Suggestion { | ||
return FilterFunc(self.getRemoteBranchNamesForRemote(remoteName), self.c.UserConfig().Gui.UseFuzzySearch()) | ||
} | ||
|
||
func (self *SuggestionsHelper) getTagNames() []string { | ||
return lo.Map(self.c.Model().Tags, func(tag *models.Tag, _ int) string { | ||
return tag.Name | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
package branch | ||
|
||
import ( | ||
"github.com/jesseduffield/lazygit/pkg/config" | ||
. "github.com/jesseduffield/lazygit/pkg/integration/components" | ||
) | ||
|
||
var OpenPullRequestInvalidTargetRemoteName = NewIntegrationTest(NewIntegrationTestArgs{ | ||
Description: "Open up a pull request, specifying a non-existing target remote", | ||
ExtraCmdArgs: []string{}, | ||
Skip: false, | ||
SetupConfig: func(config *config.AppConfig) {}, | ||
SetupRepo: func(shell *Shell) { | ||
// Create an initial commit ('git branch set-upstream-to' bails out otherwise) | ||
shell.CreateFileAndAdd("file", "content1") | ||
shell.Commit("one") | ||
|
||
// Create a new branch | ||
shell.NewBranch("branch-1") | ||
|
||
// Create a couple of remotes | ||
shell.CloneIntoRemote("upstream") | ||
shell.CloneIntoRemote("origin") | ||
|
||
// To allow a pull request to be created from a branch, it must have an upstream set. | ||
shell.SetBranchUpstream("branch-1", "origin/branch-1") | ||
}, | ||
Run: func(t *TestDriver, keys config.KeybindingConfig) { | ||
// Open a PR for the current branch (i.e. 'branch-1') | ||
t.Views(). | ||
Branches(). | ||
Focus(). | ||
Press(keys.Branches.ViewPullRequestOptions) | ||
|
||
t.ExpectPopup(). | ||
Menu(). | ||
Title(Equals("View create pull request options")). | ||
Select(Contains("Select branch")). | ||
Confirm() | ||
|
||
// Verify that we're prompted to enter the remote and enter the name of a non-existing one. | ||
t.ExpectPopup(). | ||
Prompt(). | ||
Title(Equals("Select target remote")). | ||
Type("non-existing-remote"). | ||
Confirm() | ||
|
||
// Verify that this leads to an error being shown (instead of progressing to branch selection). | ||
t.ExpectPopup().Alert(). | ||
Title(Equals("Error")). | ||
Content(Contains("A remote named 'non-existing-remote' does not exist")). | ||
Confirm() | ||
}, | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,74 @@ | ||
package branch | ||
|
||
import ( | ||
"github.com/jesseduffield/lazygit/pkg/config" | ||
. "github.com/jesseduffield/lazygit/pkg/integration/components" | ||
) | ||
|
||
var OpenPullRequestSelectRemoteAndTargetBranch = NewIntegrationTest(NewIntegrationTestArgs{ | ||
Description: "Open up a pull request, specifying a remote and target branch", | ||
ExtraCmdArgs: []string{}, | ||
Skip: false, | ||
SetupConfig: func(config *config.AppConfig) { | ||
config.GetUserConfig().OS.OpenLink = "echo {{link}} > /tmp/openlink" | ||
}, | ||
SetupRepo: func(shell *Shell) { | ||
// Create an initial commit ('git branch set-upstream-to' bails out otherwise) | ||
shell.CreateFileAndAdd("file", "content1") | ||
shell.Commit("one") | ||
|
||
// Create a new branch and a remote that has that branch | ||
shell.NewBranch("branch-1") | ||
shell.CloneIntoRemote("upstream") | ||
|
||
// Create another branch and a second remote. The first remote doesn't have this branch. | ||
shell.NewBranch("branch-2") | ||
shell.CloneIntoRemote("origin") | ||
|
||
// To allow a pull request to be created from a branch, it must have an upstream set. | ||
shell.SetBranchUpstream("branch-2", "origin/branch-2") | ||
|
||
shell.RunCommand([]string{"git", "remote", "set-url", "origin", "https://github.com/my-personal-fork/lazygit"}) | ||
shell.RunCommand([]string{"git", "remote", "set-url", "upstream", "https://github.com/jesseduffield/lazygit"}) | ||
}, | ||
Run: func(t *TestDriver, keys config.KeybindingConfig) { | ||
// Open a PR for the current branch (i.e. 'branch-2') | ||
t.Views(). | ||
Branches(). | ||
Focus(). | ||
Press(keys.Branches.ViewPullRequestOptions) | ||
|
||
t.ExpectPopup(). | ||
Menu(). | ||
Title(Equals("View create pull request options")). | ||
Select(Contains("Select branch")). | ||
Confirm() | ||
|
||
// Verify that we're prompted to enter the remote | ||
t.ExpectPopup(). | ||
Prompt(). | ||
Title(Equals("Select target remote")). | ||
SuggestionLines( | ||
Equals("origin"), | ||
Equals("upstream")). | ||
ConfirmSuggestion(Equals("upstream")) | ||
|
||
// Verify that we're prompted to enter the target branch and that only those branches | ||
// present in the selected remote are listed as suggestions (i.e. 'branch-2' is not there). | ||
t.ExpectPopup(). | ||
Prompt(). | ||
Title(Equals("branch-2 → upstream/")). | ||
SuggestionLines( | ||
Equals("branch-1"), | ||
Equals("master")). | ||
ConfirmSuggestion(Equals("master")) | ||
|
||
// Verify that the expected URL is used (by checking the openlink file) | ||
// | ||
// Please note that when targeting a different remote - like it's done here in this test - | ||
// the link is not yet correct. Thus, this test is expected to fail once this is fixed. | ||
t.FileSystem().FileContent( | ||
"/tmp/openlink", | ||
Equals("https://github.com/my-personal-fork/lazygit/compare/master...branch-2?expand=1\n")) | ||
}, | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that when using the "against selected branch" option for a GitHub fork, GitHub won't automatically open the PR against the upstream. But that has nothing to do with my change (same behaviour on current
master
). Might be worth an additional look.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm not sure how best to address that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had another look at this: Apparently, to create a PR against a different remote (mainly the upstream I'd guess) the URL would need to look different.
Instead of something like
one would need to use
That means the "repo URL" of the target remote (
https://github.com/jesseduffield/lazygit/
instead ofhttps://github.com/moha-gh/lazygit
) is used and the full name of the source repo is prepended to the remote branch branch (with/
replaced by:
).To handle this correctly, the
createPullRequest()
would need to be extended I think - so one can pass target and source remote as well, allowing the function to build the URL correctly.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense to me. I'm happy for that to be handled as a separate pull request (given that it's currently broken on master). As you say, it'll require updating
HostHelper.GetPullRequestURL
to take extra arguments.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, then let's fix this separately 👍