Allow user to select remote and branch when creating a PR #1889

moha-gh · 2022-04-22T05:31:01Z

When creating a PR against a selected branch (via O = "create pull request
options"), the user will first be asked to select a remote (if there is more
than one). After that, the suggestion area is populated with all remote branches
at that origin - instead of all local ones. After all, creating a PR against a
branch that doesn't exist on the remote won't work.

Please note that for the "PR is not filed against 'origin' remote" use case
(e.g. when contributing via a fork that is 'origin' to a GitHub project that is
'upstream'), the opened URL will not be correct. This is not a regression and
will be fixed in an upcoming PR.

Fixes #1826.

moha-gh

@jesseduffield: Would be great if you could have a look, I need a few pointers to continue here :)

pkg/gui/controllers/helpers/suggestions_helper.go

moha-gh · 2022-04-22T05:37:42Z

pkg/gui/controllers/branches_controller.go

@@ -375,7 +375,7 @@ func (self *BranchesController) createPullRequestMenu(selectedBranch *models.Bra
 				OnPress: func() error {


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.

hmm not sure how best to address that

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

https://github.com/moha-gh/lazygit/compare/master...mh%2Fpr-suggest-remote-branches-only?expand=1

one would need to use

https://github.com/jesseduffield/lazygit/compare/master...moha-gh:lazygit:mh/pr-suggest-remote-branches-only?expand=1

That means the "repo URL" of the target remote (https://github.com/jesseduffield/lazygit/ instead of https://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.

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.

Ok, then let's fix this separately 👍

jesseduffield

Nice work, left some feedback.

One thing I wonder is, would it make more sense to first choose a remote and then choose the branch, rather than select from branches across all remotes?

pkg/gui/controllers/helpers/suggestions_helper.go

jesseduffield · 2022-05-06T09:58:22Z

pkg/gui/controllers/branches_controller.go

@@ -375,7 +375,7 @@ func (self *BranchesController) createPullRequestMenu(selectedBranch *models.Bra
 				OnPress: func() error {


hmm not sure how best to address that

jesseduffield · 2022-05-06T09:58:43Z

pkg/gui/controllers/branches_controller.go

@@ -375,7 +375,7 @@ func (self *BranchesController) createPullRequestMenu(selectedBranch *models.Bra
 				OnPress: func() error {
 					return self.c.Prompt(types.PromptOpts{
 						Title:               branch.Name + " →",
-						FindSuggestionsFunc: self.helpers.Suggestions.GetBranchNameSuggestionsFunc(),
+						FindSuggestionsFunc: self.helpers.Suggestions.GetRemoteBranchesWithoutRemotePrefixSuggestionsFunc(),


I've got no problem with long names

moha-gh · 2022-05-25T12:45:04Z

One thing I wonder is, would it make more sense to first choose a remote and then choose the branch, rather than select from branches across all remotes?

Definitely. I'm pretty busy right now, but hope to find some time in the next one or two weeks to incorporate the feedback and update the PR :)

moha-gh · 2022-06-18T14:57:19Z

pkg/gui/controllers/branches_controller.go

-						FindSuggestionsFunc: self.helpers.Suggestions.GetBranchNameSuggestionsFunc(),
-						HandleConfirm: func(targetBranchName string) error {
-							return self.createPullRequest(branch.Name, targetBranchName)
+						Title:               "Select Target Remote",


I'd make this translatable when finalizing the PR.

When you do, be sure to use Sentence case

moha-gh · 2022-06-18T14:57:56Z

pkg/gui/controllers/helpers/suggestions_helper.go

+	for _, remote := range self.model.Remotes {
+		if remote.Name == remoteName {


Is there an easier/nicer way to find the matching remote?

@moha-gh you could use lo.Find (I think that's what it's called). Can't recall if it's actually less verbose all things considered

moha-gh · 2022-06-18T15:02:33Z

One thing I wonder is, would it make more sense to first choose a remote and then choose the branch, rather than select from branches across all remotes?

Definitely. I'm pretty busy right now, but hope to find some time in the next one or two weeks to incorporate the feedback and update the PR :)

@jesseduffield - That took a bit longer than expected :( I've changed the implementation to match the discussed approach (Select origin first, branch from that origin second). If there is only a single remote, the first step is skipped automatically. Could you have another look at the changes to see whether that's going in the right direction? If so, I'd clean up the PR for a final review.

jesseduffield · 2022-09-24T06:38:42Z

Sorry for the late re-review @moha-gh , I'm happy with the direction you've taken. Let me know when it's ready for another pass

moha-gh · 2022-09-27T11:41:50Z

@jesseduffield : Thanks, no worries. I'll try to get the PR rebased and cleaned up soon'ish :)

github-actions · 2023-03-20T09:06:36Z

Uffizzi Ephemeral Environment `deployment-19547`

☁️ https://app.uffizzi.com/github.com/jesseduffield/lazygit/pull/1889

📄 View Application Logs etc.

What is Uffizzi? Learn more!

jesseduffield · 2024-11-09T07:05:13Z

@moha-gh this is still a draft PR. Did you intend to return to this?

moha-gh · 2024-11-11T06:14:37Z

@jesseduffield : The issue has been bugging me again lately, so yes, I'd like to give this another try. I'll see whether I come round to it in the next two weeks.

moha-gh · 2024-11-15T10:37:29Z

@jesseduffield : I've rebased the PR and tried to adapt it to the various changes that happened since the last update. Could you have another look, especially re. the error handling (not sure whether I modified that correctly, now that some functions don't return errors anymore).

What still remains is the issue that (at least for GitHub) creating a PR from a fork against a upstream repo still doesn't work correctly. I left a comment in the discussion here in the PR. I'd be nice to get this fixed as well - since that was one of the main motivations for starting to work on this PR :)

jesseduffield

See comments

jesseduffield · 2022-07-01T07:37:23Z

pkg/gui/controllers/helpers/suggestions_helper.go

+	for _, remote := range self.model.Remotes {
+		if remote.Name == remoteName {


@moha-gh you could use lo.Find (I think that's what it's called). Can't recall if it's actually less verbose all things considered

jesseduffield · 2024-11-17T00:56:52Z

pkg/gui/controllers/helpers/suggestions_helper.go

@@ -162,10 +162,38 @@ func (self *SuggestionsHelper) getRemoteBranchNames(separator string) []string {
 	})
 }

+func (self *SuggestionsHelper) getRemoteBranchNamesForRemote(remoteName string) []string {


Let's remove the functions here that are not being called from anywhere.

jesseduffield · 2024-11-17T01:00:07Z

pkg/gui/controllers/branches_controller.go

@@ -764,6 +777,19 @@ 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 {


If an error can't be returned from this function then let's just remove the error return from the function signature

jesseduffield · 2024-11-17T01:00:26Z

pkg/gui/controllers/branches_controller.go

-						FindSuggestionsFunc: self.helpers.Suggestions.GetBranchNameSuggestionsFunc(),
-						HandleConfirm: func(targetBranchName string) error {
-							return self.createPullRequest(branch.Name, targetBranchName)
+						Title:               "Select Target Remote",


When you do, be sure to use Sentence case

jesseduffield · 2024-11-17T01:04:08Z

pkg/gui/controllers/branches_controller.go

@@ -375,7 +375,7 @@ func (self *BranchesController) createPullRequestMenu(selectedBranch *models.Bra
 				OnPress: func() error {


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.

jesseduffield · 2024-11-17T01:04:38Z

pkg/gui/controllers/branches_controller.go

@@ -728,13 +728,26 @@ func (self *BranchesController) createPullRequestMenu(selectedBranch *models.Bra
 				},
 			},
 			{
+				// TODO: Replace with "Select remote and branch"?


I think the current label is fine

moha-gh · 2024-11-19T10:33:59Z

@jesseduffield : Updated the PR to (hopefully) take all review comments into account. Also added error handling for the "user enters an invalid remote name" case. Previously, that would have resulted in an empty selection box for the branch name.

jesseduffield

Nice, just one thing on syntax.

It would be good to have an integration test as well. Giving a rough sketch, it would look something like this:

var OpenPullRequestNoUpstream = NewIntegrationTest(NewIntegrationTestArgs{
	Description:  "Open up a pull request, specifying an origin",
	ExtraCmdArgs: []string{},
	Skip:         false,
	SetupConfig: func(config *config.AppConfig) {
		config.GetUserConfig().OS.OpenLink = "echo {{link}} > /tmp/openlink"
	},
	SetupRepo: func(shell *Shell) {
		shell.NewBranch("branch-1")
		shell.NewBranch("branch-2")

		// Create a couple remotes
		shell.CloneIntoRemote("origin")
		shell.CloneIntoRemote("upstream")

		shell.RunCommand([]string{"git", "remote", "set-url", "origin", "https://github.com/jesseduffield/lazygit"})
		shell.RunCommand([]string{"git", "remote", "set-url", "upstream", "https://github.com/jesseduffield/other_repo"})
	},
	Run: func(t *TestDriver, keys config.KeybindingConfig) {
		// Open a PR for the current branch
		// Verify that we're prompted to enter the remote
		// Select a remote and ensure that the expected URL is used (by checking the openlink file)
	},
})

Let me know if you need any pointers

jesseduffield · 2024-11-19T11:46:30Z

pkg/gui/controllers/branches_controller.go

@@ -764,6 +776,28 @@ 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 {
+	_, ok := lo.Find(self.c.Model().Remotes, func(remote *models.Remote) bool {


Better to use lo.NoneBy here

Updated, thanks for the hint.

moha-gh · 2024-11-20T08:30:33Z

@jesseduffield : Thanks for the test template. Used it to create one test where the entered name of the remote is invalid (and thus, an error should be shown). The other one mimics the successful creation of a PR. One thing I'm not sure about is how to properly validate the link. Opening the file in Go (and not via shell commands) and checking it's contents? Do you have an idea there?

jesseduffield · 2024-11-21T08:02:38Z

@moha-gh you could just navigate to the newly created file in the files view and assert the contents of the main view upon doing so. Alternatively you could add a method to the shell struct called AssertFileContents(path, expectedContents) that fails if the contents aren't as expected.

stefanhaller · 2024-11-21T12:58:07Z

Alternatively you could add a method to the shell struct called AssertFileContents(path, expectedContents) that fails if the contents aren't as expected.

We already have the FileContent function that we use exactly for this purpose, e.g. here.

moha-gh · 2024-11-21T14:48:40Z

@stefanhaller , @jesseduffield : Great, thanks for the pointers! I've finalized the test case and marked the PR as ready :)

When creating a PR against a selected branch (via O = "create pull request options"), the user will first be asked to select a remote (if there is more than one). After that, the suggestion area is populated with all remote branches at that origin - instead of all local ones. After all, creating a PR against a branch that doesn't exist on the remote won't work. Please note that for the "PR is not filed against 'origin' remote" use case (e.g. when contributing via a fork that is 'origin' to a GitHub project that is 'upstream'), the opened URL will not be correct. This is not a regression and will be fixed in an upcoming PR. Fixes jesseduffield#1826.

jesseduffield

LGTM, great work. I'll merge once these tests pass

moha-gh commented Apr 22, 2022

View reviewed changes

jesseduffield requested changes May 6, 2022

View reviewed changes

moha-gh force-pushed the mh/pr-suggest-remote-branches-only branch from 8e00faa to 5310d37 Compare June 18, 2022 14:53

moha-gh commented Jun 18, 2022

View reviewed changes

moha-gh force-pushed the mh/pr-suggest-remote-branches-only branch from c6f4f80 to dede117 Compare November 15, 2024 10:13

jesseduffield reviewed Nov 17, 2024

View reviewed changes

moha-gh force-pushed the mh/pr-suggest-remote-branches-only branch 3 times, most recently from c6096cc to d758517 Compare November 19, 2024 10:32

jesseduffield reviewed Nov 19, 2024

View reviewed changes

moha-gh force-pushed the mh/pr-suggest-remote-branches-only branch 2 times, most recently from 85f674c to e0934db Compare November 20, 2024 08:19

moha-gh force-pushed the mh/pr-suggest-remote-branches-only branch from e0934db to fee001e Compare November 21, 2024 14:03

moha-gh changed the title ~~[WIP] pkg/gui: Only suggest remote branches when creating a PR against a selected branch~~ pkg/gui: Allow user to select remote and branch when creating a PR Nov 21, 2024

moha-gh marked this pull request as ready for review November 21, 2024 14:38

jesseduffield added the enhancement New feature or request label Nov 23, 2024

jesseduffield changed the title ~~pkg/gui: Allow user to select remote and branch when creating a PR~~ Allow user to select remote and branch when creating a PR Nov 23, 2024

jesseduffield force-pushed the mh/pr-suggest-remote-branches-only branch from fee001e to 949e131 Compare November 23, 2024 00:53

jesseduffield approved these changes Nov 23, 2024

View reviewed changes

jesseduffield merged commit c3cf48c into jesseduffield:master Nov 23, 2024
16 checks passed

moha-gh deleted the mh/pr-suggest-remote-branches-only branch November 25, 2024 06:26

		@@ -375,7 +375,7 @@ func (self BranchesController) createPullRequestMenu(selectedBranch models.Bra
		OnPress: func() error {

		for _, remote := range self.model.Remotes {
		if remote.Name == remoteName {

Allow user to select remote and branch when creating a PR #1889

Allow user to select remote and branch when creating a PR #1889

Conversation

moha-gh commented Apr 22, 2022 • edited Loading

moha-gh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jesseduffield left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moha-gh commented May 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moha-gh commented Jun 18, 2022

jesseduffield commented Sep 24, 2022

moha-gh commented Sep 27, 2022

github-actions bot commented Mar 20, 2023 • edited Loading

Uffizzi Ephemeral Environment deployment-19547

jesseduffield commented Nov 9, 2024

moha-gh commented Nov 11, 2024

moha-gh commented Nov 15, 2024

jesseduffield left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moha-gh commented Nov 19, 2024

jesseduffield left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moha-gh commented Nov 20, 2024 • edited Loading

jesseduffield commented Nov 21, 2024

stefanhaller commented Nov 21, 2024

moha-gh commented Nov 21, 2024

jesseduffield left a comment

Choose a reason for hiding this comment

moha-gh commented Apr 22, 2022 •

edited

Loading

moha-gh commented May 25, 2022 •

edited

Loading

github-actions bot commented Mar 20, 2023 •

edited

Loading

Uffizzi Ephemeral Environment `deployment-19547`

moha-gh commented Nov 20, 2024 •

edited

Loading