Checkout pull-request without resetting existing branch #519
Conversation
|
/CC @tarsius I would appreciate early feedback. Thank you. |
|
I've seen you have pushed some cleanup fixes, thank you. Do you know if this is the right direction? Is there something I should work on? |
|
Just a though: if |
|
I've been using that PR for over a month now and am very happy with the result. |
a8e9273 to
ba9db11
Compare
| (let ((magit-inhibit-refresh t)) | ||
| (forge-branch-pullreq pullreq)))))) | ||
| (forge-branch-pullreq pullreq)) | ||
| branch)))) |
There was a problem hiding this comment.
I’m not certain I follow everything here, but this looks like it will ask twice if you select 'ask (once “Reset %s?”, and then “A branch named "%s" already exists … [r][c][a]” when forge-branch-pullreq is called) and if you select t, then it will still ask once in forge-branch-pullreq, no?
I think this change would be better made a bit deeper, in forge--pullreq-branch-select, because it’ll improve both forge-checkout-pullreq and forge-checkout-worktree (I use the latter mostly).
Then an option could be added to the existing prompt to support checking out the existing branch [u]se existing "%s" branch (a result you can’t currently get, and in your change is only possible unilaterally).
And finally, the defcustom would have options for every case, like 'reset, 'create, 'use, 'abort, and nil (for “ask”). Leaving nil as the default so that the behavior doesn’t change for users who don’t set it.
| @@ -50,6 +50,14 @@ Takes the pull-request as only argument and must return a directory." | |||
| :group 'forge | |||
| :type 'function) | |||
|
|
|||
| (defcustom forge-checkout-reset-existing-branch nil | |||
There was a problem hiding this comment.
I think with the current state of this branch that the default here should be t, as that will preserve the existing behavior. nil is “dangerous” in that it gives the user an existing branch without letting them know (and without them choosing that behavior). With the existing code they would only be handed a branch without question if it were newly-created.
(With my suggestions below, nil would be the correct default.)
|
Thank you very much for your feedback. This PR is only a draft to get some help from @tarsius as I don't understand everything the code is doing. I've been using the code for months now and am very happy about it. |
|
I'm sorry this has continuously been falling through the cracks. I'll try to have another look soon. |
|
I've merged something. The more I looked at the clearer it became that an existing branch should just be checked out and that offering the option to reset would lead down a rabbit whole. More details in the commit message. |
If the local branch that represents a pull-request already exists, then always use it as-is. Never try to update it to point at the same commit as the pull-request as it exists on a remote. Trying to do the latter would be a futile endeavor. If the local and remote branches have diverged, then the user is expected to reconcile that fact using appropriate general purpose Magit command, which performs the action that they feel is right in each particular case. Since we configure the fork remote and branch relationships as appropriate, it should be easy enough to reason about diverging history. We cannot know how to dwim if the user checks out / "creates" an existing branch, which is/isn't the current branch in this/another worktree, with history diverging on both sides / new commits only on the local/remote side; without/with any staged and/or unstaged changes; when we can/cannot use the same name as on the remote; the fallback branch "pr-N" does/doesn't exist; the pull-request comes from the upstream / a fork; pr metadata is/isn't already set up, and so on. Closes #519.
Closes #518.
This is work-in-progress. I'm opening the PR because I'm not sure I
understand everything and require early feedback. Can you please
advice me?