jesseduffield
Add new command "Move commits to new branch"
- PR Description
Add a new command "Move commits to new branch" (bound to N by default), which is useful if you have just started some new work, you already made some commits, and then you realize that you forgot to create a new branch first, accidentally making those commits on main or whichever other feature branch you happened to be on.
This commit prompts you to type a name for the new branch; it then creates this new branch starting from the current branch (so unlike the n command, the selection doesn't matter), hard-resets the current branch to its upstream, and checks out the new branch. If you had uncommitted changes in your working copy, those are automatically stashed and brought along.
Inspired by Magit's magit-branch-spinoff command.
The conditions under which the command is available are rather restrictive: the current branch must have an upstream, it must not be behind its upstream, but it must be ahead of it (otherwise there wouldn't be any commits to move, and you might as well just create a new branch normally).
- Please check if the PR fulfills these requirements
- [x] Cheatsheets are up-to-date (run
go generate ./...) - [x] Code has been formatted (see here)
- [x] Tests have been added/updated (see here for the integration test guide)
- [x] Text is internationalised (see here)
- [ ] If a new UserConfig entry was added, make sure it can be hot-reloaded (see here)
- [ ] Docs have been updated if necessary
- [x] You've read through your own file changes for silly mistakes etc
Coverage summary from Codacy
See diff coverage on Codacy
| Coverage variation | Diff coverage |
|---|---|
| Report missing for 05ae0800c3ff3fd7160a5705a8fbac7f50745624[^1] | :white_check_mark: 85.71% |
Coverage variation details
| Coverable lines | Covered lines | Coverage | |
|---|---|---|---|
| Common ancestor commit (05ae0800c3ff3fd7160a5705a8fbac7f50745624) | Report Missing | Report Missing | Report Missing |
| Head commit (67a61e41df89f7354ee4a4c0b8248dba5bdb9e04) | 49902 | 42730 | 85.63% |
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>
Diff coverage details
| Coverable lines | Covered lines | Diff coverage | |
|---|---|---|---|
| Pull request (#3876) | 175 | 150 | 85.71% |
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%
See your quality gate settings Change summary preferences
Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more [^1]: Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct.
![codacy-production[bot] avatar](https://avatars.githubusercontent.com/in/56611?v=4 "Published by a pub.dev verified publisher"){kind=small}
Code LGTM. One thing that would be good is if we could change the prompt or add a description in a secondary panel so that we're being clear about what we're about to do: right now there's no way to know whether you've pressed 'n' or 'N' after pressing the key, because the two prompts are identical.
I thought about this, or rather about an extra confirmation panel that comes before ("are you sure you want to..."). After all, a hard reset is involved, and this feels like a confirmation is in order. But then I realized that there's really no potential for losing data with the current approach, so I left it out. I can look into changing the prompt text though.
However, while testing it more I noticed that this only really works in the expected way when you start on master. If you start on an unrelated feature branch, it will create the new branch as a stacked branch on top of that one, which might sometimes be what you want if you are into stacked branches, but most of the time it's probably not. You want to create the new branch off of master most of the time.
If we want to achieve this, it has a few consequences:
- we can provide the command only in the branches panel, and the selection becomes important. The new branch will be created off of the selected branch. (Probably a good thing because that makes it more similar to
n.) - the simple approach of creating the new branch and hard resetting the other one is not enough. We do actually need to cherry-pick the commits to bring them over (except in the stacked branches example, so we need to check if the new branch contains the commits already).
- this means we can now get conflicts, both when cherry-picking and when popping the auto-stash. Neither could happen with the current approach.
Very keen to hear your thoughts on this @jesseduffield
we can provide the command only in the branches panel, and the selection becomes important. The new branch will be created off of the selected branch. (Probably a good thing because that makes it more similar to n.)
I actually don't think that's a good thing: the purpose is to move the commits from the checked out branch to a new branch. The selection shouldn't matter.
However, while testing it more I noticed that this only really works in the expected way when you start on master. If you start on an unrelated feature branch, it will create the new branch as a stacked branch on top of that one, which might sometimes be what you want if you are into stacked branches, but most of the time it's probably not. You want to create the new branch off of master most of the time.
I'm fine if we just only allow this command from the main branch
we can provide the command only in the branches panel, and the selection becomes important. The new branch will be created off of the selected branch. (Probably a good thing because that makes it more similar to n.)
I actually don't think that's a good thing: the purpose is to move the commits from the checked out branch to a new branch. The selection shouldn't matter.
Hm, ok. That means we wouldn't provide the option of creating a new branch stacked on the current one. Maybe that's fine as it should be rather rare, but I do remember that I was in this situation once or twice.
What we need to do then is to create the new branch off of the current branch's base branch.
I'm fine if we just only allow this command from the main branch
This restriction would make it almost useless for me. I almost never have main checked out, so I don't accidentally make commits there; I'm much more likely to run into the problem while I'm on an unrelated feature branch.
I see. For whatever reason I seem to only get into this state from the master branch.
If we need to handle the case where this happens on a feature branch, then it seems to me that we'd need to specify two things: the commits we want to move, and the branch we want to use as the new base (defaulting to master). So I'm thinking we could make it that the keybinding is only present from the commits panel (after you've selected the commits to move) and the branch is specified via an input prompt. Or as you say, we could just programmatically work out the base branch; I'm fine with that too.
Then we would:
- copy the commit hashes
- checkout a new branch off of the specified base
- cherry-pick the commits
- hard reset the original branch to remove the commits (which we can do without checking it out, see here)
If there are conflicts with the cherry-pick, we could just skip the fourth step and leave that as an exercise for the user.
What do you think of that?
Hm, I'm not sure. What you propose would only work if the selected range is at the end of the branch. Otherwise we'd have to drop the commits from the branch, which is not a simple reset, so there's potential for conflicts both at the source and the destination.
I'm not sure we need this much flexibility though; I think I'd be fine with restricting it to moving the unpushed commits. If you have the need to move an arbitrary commit range (whether at the end of the branch or in the middle), you still have to do that manually, like today.
So I'm still in favor of offering the command only in the branches panel; you select the target branch, and it works on the unpushed commits of the current branch. If you're on a branch that doesn't have an upstream (yet), the command is disabled.
My feeling is that this will cover most real-world cases, but of course that's hard to tell without using it for a while and seeing how much it helps.
@stefanhaller are you waiting on my re-review for this PR or were there more changes to push?
No, I was planning to implement the behavior that I proposed above, but didn't get around to it yet. Setting to draft in the meantime to make that clearer.
@stefanhaller hey! i'm really interested in this feature, it's one of the main things i miss from magit.
is there any chance you can take another look? i'd even be happy to pay a bounty for this
no stress if not, i can always reinstall emacs just for this command :p
@ar1a I down-prioritized this a bit because I don't need it very often myself. I can't promise when I find the time to take this up again.
I'd be curious what you think about the discussion above. I'm having a hard time working out what magit's behavior is just from the description of its magit-branch-spinoff command (not a magit user myself).
Oh, and I'm surprised that you'd rather switch to magit just for that command, instead of using the next best workaround inside of lazygit, which is to select the commits you made on the wrong branch, type shift-C to copy them, reset the current branch (using g h), create the new branch off of wherever you want it to start, and pressing shift-V to paste your commits there. I find this reasonably easy, to the point that I'd rather use this manual, explicit way of doing it than to use a command which I'm not totally sure will behave exactly the way I want it to.
I didn't know about that workflow! It turns out I was still on lazygit ~0.40, which didn't have branch selection iirc. It definitely saves me the hassle of re setting up magit.
Oh, and I'm surprised that you'd rather switch to magit just for that command
to be fair, magit is really good.
if you haven't found it yet, here's magit's source code for this: https://github.com/magit/magit/blob/28d272ce0bcecc2e312d22ed15a48ad4cea564eb/lisp/magit-branch.el#L459-L531
I'm not sure we need this much flexibility though; I think I'd be fine with restricting it to moving the unpushed commits. If you have the need to move an arbitrary commit range (whether at the end of the branch or in the middle), you still have to do that manually, like today.
I agree with this, the behaviour I would expect from magit is a sort of "dumb" solution: Just base it off of the current branch you're on, don't go all the way back to main. If the user wants to do a multi-branch spinoff they will probably need to do it manually via the copying commits strategy as described in previous comments.
I think this feature is definitely still worth implementing. Having an easily pressed key when you fuck up and start working on the wrong branch is much easier to deal with than remembering the key sequence here https://github.com/jesseduffield/lazygit/pull/3876#issuecomment-2692643703
(in fact, i think the next few times I need this I would have to come back to this thread to remind myself)
if you haven't found it yet, here's magit's source code for this: https://github.com/magit/magit/blob/28d272ce0bcecc2e312d22ed15a48ad4cea564eb/lisp/magit-branch.el#L459-L531
Thanks, but I speak very little lisp, so this is of limited usefulness to me...
the behaviour I would expect from magit is a sort of "dumb" solution: Just base it off of the current branch you're on, don't go all the way back to
main
Doesn't that mean that you will always get a stack of branches when you are starting on another feature branch? This may occasionally be what I want, but most of the time it's not, so I'd then have to manually rebase the branch onto main, away from that other feature branch. While that's easy using lazygit's shift-B feature, it's still cumbersome.
I'm wondering if we should prompt the user whether they want to stay on the other feature branch, or move off of it onto main.
I'm wondering if we should prompt the user whether they want to stay on the other feature branch, or move off of it onto main.
Ok, re-reading the discussion above I realize that I earlier suggested to select the target branch in the branches panel; that would make this prompt unnecessary. However, I actually no longer think this suggestion makes sense, for a number of reasons:
- If I want to select master, I first need to make sure it's up to date. For me, since I never use master locally it is usually behind its upstream, so I first have to fast-forward it. While that's easy (
f) it can take a bit of time, and it's annoying to have to do that. The prompt suggested above doesn't have this problem. (Incidentally, the same problem exists for just creating a new branch off of master; I sometimes wish we had a command for creating a branch off of origin/master.) - Selecting any other branch besides master or the one that you are currently on doesn't seem to make any sense to me, so this flexibility is unnecessary.
So I would now suggest to offer the command in branches view or commits view, as in the original version, and the selection doesn't matter; then, if the checked out branch is master, just do the simple thing, and if it is some other branch, show the prompt whether we should stay there and create a stack (in which case we do the simple thing here as well), or if we should move to master, in which case we need to do the slightly more complicated cherry-pick approach.
Hope this makes sense; I'll see if I can find some time to prototype this.
Thanks, but I speak very little lisp, so this is of limited usefulness to me...
understandable 😅, it definitely seems indecipherable at a glance. especially since emacs lisp has its own functions for a lot of things, you sort of just need to be using emacs to understand it
So I would now suggest to offer the command in branches view or commits view, as in the original version, and the selection doesn't matter; then, if the checked out branch is master, just do the simple thing, and if it is some other branch, show the prompt whether we should stay there and create a stack (in which case we do the simple thing here as well), or if we should move to master, in which case we need to do the slightly more complicated cherry-pick approach.
i like this! i think this makes a lot of sense and really fits with lazygits design
Coverage summary from Codacy
See diff coverage on Codacy
| Coverage variation | Diff coverage |
|---|---|
| Report missing for bd1e34b39d7e580f1edd7daf080076b2ee3390ce[^1] | :white_check_mark: 90.72% |
Coverage variation details
| Coverable lines | Covered lines | Coverage | |
|---|---|---|---|
| Common ancestor commit (bd1e34b39d7e580f1edd7daf080076b2ee3390ce) | Report Missing | Report Missing | Report Missing |
| Head commit (f65166ae91a847d08358040a5d1858eca78ee234) | 55871 | 48476 | 86.76% |
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>
Diff coverage details
| Coverable lines | Covered lines | Diff coverage | |
|---|---|---|---|
| Pull request (#3876) | 345 | 313 | 90.72% |
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%
See your quality gate settings Change summary preferences
[^1]: Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct.
I pushed a new version that implements the proposal from my last comment. Would be good to get some thorough testing on this!
Testing this out locally and I get the error due to the current branch (which happens to be the main branch) being behind the upstream. @stefanhaller what is that error defending against? I would very much like to just ignore that edge case and have lazygit allow me to move the commits
Testing this out locally and I get the error due to the current branch (which happens to be the main branch) being behind the upstream. @stefanhaller what is that error defending against? I would very much like to just ignore that edge case and have lazygit allow me to move the commits
Ok, this requires a bit of a longer explanation. For main branches it would indeed be nice to support this case, and I have seen this myself and found it annoying. However, it would be quite a bit of work to support this case, because we would no longer be able to share the code between main branches and stacked feature branches. In both cases we currently hard-reset the current branch to its upstream; this would have to change, we would have to hard-reset to the merge base of the branch and its upstream. And for feature branches we don't want to support that case, because if you have ↓3↑3, there's no way to tell whether a coworker pushed 3 commits, and you pushed 3 unrelated commits (in which case you could totally move those 3 commits to another branch and reset to the merge base), or whether you reworded a commit, in which case you don't want to hard-reset the current branch to anything. (This latter case can't happen for main branches, because you typically don't rebase them.)
I decided not to invest the work to support this case because with the "auto-forward main branches" PR it will happen a lot less often.