Add 'Quick start interactive rebase' action

[jesseduffield]

• PR Description

A common issue I have is that I want to move a commit from the top of my branch all the way down to the first commit on the branch. To do that, I need to navigate down to the first commit on my branch, press 'e' to start an interactive rebase, then navigate back up to the top of the branch, then move my commit back down to the base. This is annoying.

Similarly annoying is moving the commit one-by-one without explicitly starting an interactive rebase, because then each individual step is its own rebase which takes a while in aggregate.

This PR allows you to press 'i' from the commits view to start an interactive rebase from an 'appropriate' base. By appropriate, we mean that we want to start from the HEAD and stop when we reach the first merge commit or commit on the main branch. This may end up including more commits than you need, but it doesn't make a difference.

• Please check if the PR fulfills these requirements

• Cheatsheets are up-to-date (run go generate ./...)
• Code has been formatted (see here)
• Tests have been added/updated (see here for the integration test guide)
• Text is internationalised (see here)
• Docs (specifically docs/Config.md) have been updated if necessary
• You've read through your own file changes for silly mistakes etc

[codacy-production]

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
✅ +0.05% (target: -2.00%)	✅ 94.94%

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (5c888a0)	45840	37731	82.31%
Head commit (0f41834)	45997 (+157)	37885 (+154)	82.36% (+0.05%)

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 (#3213)	158	150	94.94%

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

_{You may notice some variations in coverage metrics with the latest Coverage engine update. For more details, visit the documentation}

[jesseduffield]

Hard to say if this should be deemed a feature or an enhancement, but it's certainly something that I as a user would like to know about so I'm labelling it a feature

[stefanhaller]

This is really nice, I can see myself using this often. A few comments inline below.

More thoughts about the issues that this addresses:

• for moving commits down, I often press 'e' right before the destination that I want to move to; I can then hold <c-j> and auto-repeat to move it down, knowing that it will stop at the 'e' commit. Thinking about this, that's really a workaround for the missing feature of marking one or more commits for moving, and then navigating to the destination and invoking a "move marked commits here" command. Maybe that's input for the range select discussion.
• I want the same for "squash all fixups above". It would be nice to have a command for "squash all fixups in this branch". This could be done by replacing the confirmation of "squash all fixups above" with a menu.
• For stacked branches, this will rebase the entire stack, which may or may not be what's intended. In many cases it's fine, probably, and if it's not, then users have to fall back to 'e'. Just mentioning it.

[stefanhaller]

Not sure I understand the reasoning for shift-I; I'd have expected 'i', like the PR description says. Not sure why 'e' needs another alias.

[jesseduffield]

I started with 'i' but it struck me that the most intuitive thing that 'i' would do would be to start an interactive rebase from the selected commit, whereas quick-start would be more of a special case. So I wanted to give us the freedom to change that in the future without worrying about backwards compatibility.

[stefanhaller]

the most intuitive thing that 'i' would do would be to start an interactive rebase from the selected commit,

Not sure about that; you could also argue that the most intuitive thing that 'i' would do is to do an interactive rebase of your current branch.

Thinking about this more, I'm also against calling this "quick start", as if it's just a special alternative for the "normal" way to start an interactive rebase. This is the normal way to start one (or it could become it), so I would rather call it "Start interactive rebase of current branch", or maybe even just "Start interactive rebase" (the tooltip could explain more). I have watched lazygit newcomers struggle to find out how to interactively rebase a branch, being surprised that they have to select a commit to rebase from.

And binding it to shift-I contributes to conveying that it's somehow special, or an expert or niche thing or something. I really think it deserves to be bound to 'i'.

[jesseduffield]

I'm happy to do that and see how users like it. I have a premonition that it is a special action for the following reasons:

• this feature isn't built into the git CLI so most people coming from the CLI will expect to be specifying a base commit by default
• the rules we've defined for finding the base commit are specific to our own workflows and may not reflect the typical workflow (e.g. maybe some people expect merges in their rebases, or some people directly commit to the main branch in personal projects, or do want stacked branches to be taken into consideration etc)

Nonetheless, fortune favours the brave, so let's go with 'i', call it 'Start interactive rebase' and explain the logic in a tooltip.

[stefanhaller]

• this feature isn't built into the git CLI so most people coming from the CLI will expect to be specifying a base commit by default

I see a lot of people do git rebase -i master on the command line. Which isn't the same, but comes close, at least in the intention to mean "rebase my current branch interactively".

Nonetheless, fortune favours the brave, so let's go with 'i', call it 'Start interactive rebase' and explain the logic in a tooltip.

🎉

[jesseduffield]

I see a lot of people do git rebase -i master on the command line.

Ah good point

[stefanhaller]

I'd prefer this to be a DisabledReason, like we do for amendTo and squashAllFixupsAbove. (We are not super consistent with this, some other commands only check it in the handler, but for new ones it would be good to use DisabledReasons rather more than less.)

More thoughts on this: Status.WorkingTreeState() is reasonably cheap, but it still needs to hit the file system, so we may not want to use it for DisabledReasons. Maybe we should switch to Model.WorkingTreeStateAtLastCommitRefresh for that, but then we should check Status.WorkingTreeState() again in the handler just to be sure. I guess.

[jesseduffield]

Fair point. Happy to use WorkingTreeStateAtLastCommitRefresh and we don't need to do another check inside the handler: it's actually harmless if we skip the check: it just means that we'll get 'YOU ARE HERE' on the head commit. That's not something I'm worried about.

[stefanhaller]

it's actually harmless if we skip the check: it just means that we'll get 'YOU ARE HERE' on the head commit. That's not something I'm worried about.

No, you'd get a cryptic error from git that you are already in a rebase. That would be pretty unpleasant. You could argue that it's so very unlikely that the model state is out of date that it's not worth worrying about it happening, and that I could agree with.

Also, I was thinking more about the two other places where we now use Status.WorkingTreeState() for disabled reasons; I think it would be good to change those, too.

[jesseduffield]

Ah sorry, I was thinking about the other edge case where the first commit is a merge (or main branch) commit.

Also, I was thinking more about the two other places where we now use Status.WorkingTreeState() for disabled reasons; I think it would be good to change those, too.

Agreed

[stefanhaller]

This should also be a DisabledReason. Maybe not using the FindIndexOf; I guess checking the merge state of Commits[0] should be enough for that.

[jesseduffield]

Can do

[stefanhaller]

Nitpick: there are two insteads in the last sentence.

[jesseduffield]

haha good catch

[stefanhaller]

One more thing that's important for stacked branches: for 'e' we added code to reselect the same commit again, we need to do the same thing here, for the case that the selection is in one of the lower branches of a stack of branches. See #2954

[jesseduffield]

• for moving commits down, I often press 'e' right before the destination that I want to move to; I can then hold <c-j> and auto-repeat to move it down, knowing that it will stop at the 'e' commit. Thinking about this, that's really a workaround for the missing feature of marking one or more commits for moving, and then navigating to the destination and invoking a "move marked commits here" command. Maybe that's input for the range select discussion.

It is nice to be able to hold down the move key to get to the destination, but it's annoying to have to specify the target inbetween. I also want a better UX for this, but I don't know how we would make it easy to move the commits without something like the Bottomless Rebase idea I did a writeup on today (ultimately deciding against it: #3214).

• I want the same for "squash all fixups above". It would be nice to have a command for "squash all fixups in this branch". This could be done by replacing the confirmation of "squash all fixups above" with a menu.

That sounds good to me.

• For stacked branches, this will rebase the entire stack, which may or may not be what's intended. In many cases it's fine, probably, and if it's not, then users have to fall back to 'e'. Just mentioning it.

I'm okay with that: I wouldn't want a 'coincidental stack' (which I get from time to time) to interfere with the expected behaviour.

[stefanhaller]

I also want a better UX for this, but I don't know how we would make it easy to move the commits without something like the Bottomless Rebase idea

We could do it without the bottomless rebase idea if we decouple the selection from a "marked to be acted on" state (similar to what we do for cherry-picking). You'd first mark the commits that you want to move, then navigate to the destination, then invoke the "move marked commits here" command.

We'd have to do something similar if we wanted to support non-contiguous selections, that's why I said it could be related to the multiselection discussion. But I'm not sure it's a good idea, and we already decided to only support contiguous selections, so best to forget it, maybe. 😄

[jesseduffield]

We'd have to do something similar if we wanted to support non-contiguous selections, that's why I said it could be related to the multiselection discussion. But I'm not sure it's a good idea, and we already decided to only support contiguous selections, so best to forget it, maybe. 😄

Yeah I'm happy to wait and see how we feel about things once we're comfortable with range select. We might find it's already so easy that we don't need to improve it.

[jesseduffield]

Working on the code, I think we still need a name for this feature, even if it's only used internally. Just calling it 'Start interactive rebase' is ambiguous because it could be talking about starting from a manually chosen commit or an automatically chosen one. I'm gonna use 'auto-start interactive rebase' internally if there's no objections

[stefanhaller]

I find "auto" a bit misleading in there, because it sounds like the rebase would start automatically (as in the "hidden rebase" case maybe).

I'd have suggested StartInteractiveRebaseForCurrentBranch, but I have no strong opinion.

[jesseduffield]

I see your point, though with StartInteractiveRebaseForCurrentBranch it could be misconstrued i.e. 'of course it's on my current branch'. I'm gonna lock in quick-start again because it really is a quick way to start a rebase compared to the manual approach.

[stefanhaller]

Works for me.

[jesseduffield]

ready for re-review @stefanhaller

[codacy-production]

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
✅ +0.05% (target: -2.00%)	✅ 94.65%

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (e9962b9)	45847	37738	82.31%
Head commit (ee106f9)	46028 (+181)	37909 (+171)	82.36% (+0.05%)

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 (#3213)	187	177	94.65%

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

_{You may notice some variations in coverage metrics with the latest Coverage engine update. For more details, visit the documentation}

[stefanhaller]

There's one problem left with the disabled reason text, I pushed a fixup for that. Apart from that, looks great now!

[stefanhaller]

It's weird to see a "Disabled:" tooltip in the keybindings menu that starts with "Could not".

Since this is now only used for the disabled reason and not for regular error checking in the handler, it should be rephrased and renamed. I pushed a fixup (6c9796a) to do this and to change the logic a bit to make this clearer.

With the show-disabled-reasons-as-toasts branch we will still have the problem that the text is much too long for the bottom line view; also, when used as a disabled tooltip in the menu it doesn't need to be this long, since it basically only repeats what the regular tooltip already says. Not totally sure what to do about this, but my feeling is that the Disabled reason should be shortened a lot to be just "No non-main commits to rebase" or something like that.

[jesseduffield]

Thanks for making that commit. There's two separate things I want to respond to here:

Relying on disabled checks

In the code I see:

// No need to check for nil here, our DisabledReason callback guarantees it's non-nil

The callback actually does not guarantee this, because a background refresh could change self.c.Model().Commits and the commit that was found before may no longer be found (for example it may be that in another process the main branch was checked out). Although this is super unlikely, if it does happen it will cause a panic via nil pointer exception, and I really don't want lazygit to ever panic.

This means that to be safe we need to unfortunately have some convoluted code and double-handling. The proper solution to this problem is to use a mutex to guarantee that the model does not change while a controller action is being processed and if I recall correctly that's something you've looked into recently. Until that work is done, I'm going to play it safe.

Disabled reasons as toasts

I really don't like the idea of shortening all disabled error messages to fit on one line: some errors are the result of a user accidentally doing something nonsensical, like trying to pull the same branch twice. For that, an error message like 'already pulling this branch' is all you need. But there are some errors that result from a user fundamentally misunderstanding something, and they need help, and that requires a longer error message.

A user might think that 'Start interactive rebase' just picks a convenient base commit somewhat arbitrarily and then get surprised when it fails on the main branch (note that they may have a valid reason to do that e.g. they're just working on a personal project with a single branch). At the very least, they need to be told that if they still want to do an interactive rebase, they can press 'e' on a commit. Expecting the user to go looking for the keybinding tooltip to get that extra info feels wrong. There's also the possibility that the user is on a feature branch but the last commit was a merge, and so our error message would need to capture that too.

In your show-disabled-reasons-as-toasts PR, I mentioned that we could let getDisabledReason function decide whether to show an error as a toast or an error message, and I think this is one such case. Showing 'already rebasing' as a toast is perfect because (almost) nobody believes that you should be able to start a rebase while already rebasing. So in this case we've got two possible error messages, one of which works as a toast and one which doesn't. So we could:

• instead of returning strings from our disabled reason functions, make a DisabledReason struct which contains two fields: a message, and a boolean for whether it's displayed as a toast or a popup, or
• decide whether to render something as a toast based on the length of the message

Also:

also, when used as a disabled tooltip in the menu it doesn't need to be this long, since it basically only repeats what the regular tooltip already says

I'm fine with this

[jesseduffield]

I retained your language change but reverted the other parts of the change

[stefanhaller]

I see. Too bad that we can't rely on the model not changing concurrently to main thread code (yet).

I made this assumption in other places before, so maybe those need to be audited for the same problem. I can't check myself right now because I'm not at my computer; one case that I remember off the top of my head is checkSelected in local_commits_controller.go. In that case it's probably still ok, since for the assumption to not hold someone would have to drop all commits of the repo in another process (I think?).

Anyway, for this PR I have no more objections.