Interactive rebase with in-editor TODO edit by AzraelSec · Pull Request #2370 · jesseduffield/lazygit

AzraelSec · 2023-01-17T00:31:39Z

PR Description
This PR closes issue Suggestion: shortcut to interactive rebase on top of selected commit/branch #733. In particular, it adds the possibility to perform interactive rebases after editing the TODO file using an external editor.

I've updated the already existing integration tests to make them pass, but it would be a good idea to add one for this specific feature later on. I've tried to include it inside this same branch but I couldn't easily handle the interaction with the external editor.

Disclamer: first contribution, hope I made it well but forgive any error I could have made 🙏🏻

Please check if the PR fulfills these requirements

Cheatsheets are up-to-date (run go run scripts/cheatsheet/main.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

jesseduffield · 2023-01-29T02:50:20Z

@AzraelSec Thanks for raising a PR. Currently this PR has us opening up a TODO file in the editor but what I want instead is to start an interactive rebase within lazygit, as if we had hit e on one of the commits in the commits panel. So we shouldn't see an actual TODO file in an editor.

If you have a use case for viewing the TODO file in say vim and editing that yourself, we should have a separate keybinding for that which you can use inside the commits panel when we're in the middle of an interactive rebase.

Let me know if you have any questions :)

AzraelSec · 2023-01-29T16:02:38Z

@AzraelSec Thanks for raising a PR. Currently this PR has us opening up a TODO file in the editor but what I want instead is to start an interactive rebase within lazygit, as if we had hit e on one of the commits in the commits panel. So we shouldn't see an actual TODO file in an editor.

If you have a use case for viewing the TODO file in say vim and editing that yourself, we should have a separate keybinding for that which you can use inside the commits panel when we're in the middle of an interactive rebase.

Let me know if you have any questions :)

Ouch, I'm sorry @jesseduffield I clearly misunderstood what the commenters were talking about while discussing the issue! Okay, I got what you mean and it ultimately makes sense 😅 I'll try to implement this feature with the UX you expect 👍🏻

For what regards the editing of the TODO file using an external editor (as I'd prefer to do), would it make sense to you to integrate it? Do you prefer me to close this PR and open a separate issue where we can discuss it before opening a new PR?

Thank you! 🙏🏻

jesseduffield · 2023-01-29T22:06:02Z

I'm happy for us to continue using this PR and to just leave out functionality around editing the TODO file in an editor :)

joihn · 2023-03-28T15:36:50Z

the desired feature can be achieved via a custom command:
#2517 (comment)

AzraelSec · 2023-03-29T08:07:48Z

@joihn I've been inactive for a while, but yesterday I should have reached a good implementation of this feature. So let me rebase my branch and see what @jesseduffield thinks is the best option. It would take me just some hours 👍🏻

AzraelSec · 2023-03-29T13:46:07Z

@jesseduffield, the PR should now introduce the discussed feature as you were expecting. Let me know if it satisfies your requirements 👍🏻 Thanks!

github-actions · 2023-04-02T01:09:20Z

Uffizzi Preview deployment-20987 was deleted.

jesseduffield

Nice work, I've left some comments.

The alternative approach to this would be to obtain the final todo file before we start the rebase. That would require us to do more work upfront in finding out which commits need to be rebased, but it would simplify the interface with the daemon. Given our goal of sticking as close to the git CLI's way of doing things as possible, I think the approach this PR takes is the right one. That is, if we did try to work out the commits to rebase ourselves, we'd risk diverging from git's natural behaviour.

jesseduffield · 2023-04-02T01:14:49Z

pkg/gui/controllers/helpers/merge_and_rebase_helper.go

-		map[string]string{
-			"checkedOutBranch": checkedOutBranch,
-			"selectedBranch":   ref,
+	menuItems := []*types.MenuItem{


We still want to tell the user that we're rebasing from checkedOutBranch to ref. So I'm thinking:

Title: Rebase '{{checkedOutBranch}}' onto '{{ref}}' Item 1: s: Simple rebase Item 2: i: Interactive rebase

And I'd add a tooltip for item 2 saying 'Begin an interactive rebase with a break at the start, so you can update the TODO commits before continuing'

jesseduffield · 2023-04-02T01:16:48Z

pkg/commands/git_commands/rebase.go

 // we tell git to run lazygit to edit the todo list, and we pass the client
 // lazygit a todo string to write to the todo file
-func (self *RebaseCommands) PrepareInteractiveRebaseCommand(baseShaOrRoot string, todoLines []TodoLine, overrideEditor bool) oscommands.ICmdObj {
+func (self *RebaseCommands) PrepareInteractiveRebaseCommand(baseShaOrRoot string, todoLines []TodoLine, overrideEditor bool, prepend bool) oscommands.ICmdObj {


We've got quite a few positional arguments here now. Let's create a PrepareInteractiveRebaseCommandOpts struct which can be passed as the single argument to this method.

jesseduffield · 2023-04-02T01:24:16Z

pkg/app/daemon/daemon.go

-	RebaseTODOEnvKey string = "LAZYGIT_REBASE_TODO"
+	DaemonKindEnvKey   string = "LAZYGIT_DAEMON_KIND"
+	RebaseTODOEnvKey   string = "LAZYGIT_REBASE_TODO"
+	PrependLinesEnvKey string = "LAZYGIT_PREPEND_LINES"


I'm not sure I'm following this code right, can you confirm:
When we want to do an interactive rebase (with a break) onto a ref, we pass LAZYGIT_REBASE_TODO="break" and LAZYGIT_PREPEND_LINES=true and then when the lazygit daemon is invoked to update the git-rebase-todo file, that file will already have all the commits from the original branch that we're about to rebase onto the ref, and we'll then prepend the break action onto the existing TODO lines so that in lazygit we can modify the remaining TODO lines. Is that right?

If that's correct, we should have a comment explaining what we're doing because it's not immediately obvious from looking at the code. We can put that comment above the PrependLinesEnvKey const

Yes, your assumption is correct. I'll introduce a comment as you suggested to give more context on the reasons behind this implementation.

pkg/app/daemon/daemon.go

jesseduffield · 2023-04-02T05:17:38Z

Also @AzraelSec could you add an integration test for when the user chooses the interactive rebase option?

stefanhaller · 2023-04-02T06:31:39Z

The alternative approach to this would be to obtain the final todo file before we start the rebase. That would require us to do more work upfront in finding out which commits need to be rebased, but it would simplify the interface with the daemon. Given our goal of sticking as close to the git CLI's way of doing things as possible, I think the approach this PR takes is the right one. That is, if we did try to work out the commits to rebase ourselves, we'd risk diverging from git's natural behaviour.

I agree that crafting a todo file ourselves is not a good idea, and we should let git do the work; it knows better how to do that. For example, when people have rebase.updateRefs in their git config and work with stacked branches, git puts extra instructions in the todo file to update the branches. We'd have to learn how to do that too, which could be a lot of work.

In fact, we have this problem already for the cases where we do craft our own todo file, e.g. when you press "e" on a commit. I'm working on changing these to let git produce the todo file, using a very similar mechanism like this PR does. This way it will also be able to support --rebase-merges (though we might have to make that optional, not sure about that yet).

AzraelSec · 2023-04-02T21:23:54Z

@jesseduffield I should have applied all the changes requested 👍🏻

I used the logic you and @stefanhaller were discussing precisely because having a list of commits consistent with the canonical git behaviour ahead of time is challenging. We are configuring our tool to perform some actions during the rebase in "headless" mode: the requester instance of lazygit needs to tell the second one what to do with the git-rebase-todo content. It would be great to have a way of describing this as generically as possible. For this reason, I opted for a prepend lines flag instead of a more specific break-before-rebasig one.

In the future, it would be great to think about a more complex way of encoding the commands we want the daemon to execute, a complete parsing mechanism of the default git-rebase-todo and a more powerful form of expressing the actions that the daemon can run. But all of this is OP and should be left for another time.

stefanhaller · 2023-04-03T08:00:41Z

This is great, I'm looking forward to having this feature; I didn't realize at first how often I would find it useful (I was a bit skeptical about it's usefulness when it was initially proposed 😄).

However, I'd like to point out a potential problem: at the moment, lazygit works on the assumption that the number of lines in the git-rebase-todo file is the same as the number of rebase commits in the model. It uses this assumption when changing the type of a rebase commit (e.g. from pick to drop), and when moving commits up or down. This assumption holds for the cases where lazygit crafts a todo file itself, e.g. when hitting "e" (which I would like to change at some point), but it doesn't necessarily hold for cases where you start an interactive rebase outside of lazygit, e.g. when --rebase-merges or --update-refs is used. It can even happen inside of lazygit, for example when using the "Squash all fixups above this one" command, and then there's a conflict further down; if the global git config rebase.updateRefs is true and there are stacked branches, hitting "drop" in this situation will drop the wrong commit.

I'm working on a PR to fix this, I hope to have it ready by the end of the week at the latest; just pointing this out because the new command in this PR will expose people to this situation more often, so we might want to hold off merging this until the fix is in.

AzraelSec · 2023-04-12T10:24:07Z

@stefanhaller don't want to bother or rush you, but do you have any updates on this? I'd like to move this PR forward at some point. Thanks! 🙏🏻

stefanhaller · 2023-04-12T13:01:18Z

I have a branch that has been ready for almost a week now; I didn't make a PR yet because I didn't want to swamp Jesse with too many review requests. Looks like he's too busy to review things right now, and I have a few other PRs open that I first want to get merged before I open new ones.

AzraelSec · 2023-04-13T07:53:48Z

I see; no problem, thank you. Could you please keep this PR up-to-date on this? Thanks!

jesseduffield · 2023-04-13T11:35:17Z

@stefanhaller if you've got that branch available feel free to raise it and I can give it a review, if it's a blocker for this PR. I'm happy to do the extra work of rebasing my refactor PR if it means we can keep things moving now

stefanhaller · 2023-04-14T06:45:13Z

The PR is here: #2547

stefanhaller · 2023-04-14T15:48:21Z

@AzraelSec There are several files in your PR that have formatting problems (that's why the linter check failed). Can you please run gofumpt on all files to fix these? Also, some of them have trailing whitespace here and there, and I'm not sure gofumpt fixes that or not, so you may have to instruct your editor to do that.

AzraelSec · 2023-04-14T17:43:14Z

@AzraelSec There are several files in your PR that have formatting problems (that's why the linter check failed). Can you please run gofumpt on all files to fix these? Also, some of them have trailing whitespace here and there, and I'm not sure gofumpt fixes that or not, so you may have to instruct your editor to do that.

@stefanhaller I've fixed the code and executed all the actions locally to be sure they pass; everything should be fine now. Thank you for the heads-up! 😄

…f confirm the previous popup

`PrepareInteractiveRebaseCommand` function

… work

jesseduffield · 2023-04-15T07:26:38Z

I've pushed a small change to the copy and I've rebased on master. Once CI passes, I'll merge :)

…on 'Sentence case')

stefanhaller · 2023-04-15T07:58:15Z

As I started to use this feature more, I realized that I always have to switch focus to the commits panel when picking "interactive". It would be nice if it did this for me automatically. But I suppose we can also add this as a followup if you'd rather merge now.

jesseduffield · 2023-04-15T08:00:10Z

@stefanhaller I was just thinking about that myself. A couple times I've switched focus on behalf of the user and ended up deciding against it but in this case the only reason you're taking the interactive approach is so that you can mess around in the commits view so I reckon we switch focus here (in this PR). A simple PushContext call should suffice. Are you happy to make that change @AzraelSec ?

AzraelSec · 2023-04-15T08:11:44Z

@stefanhaller @jesseduffield good point! Yes, sure, that makes sense. Let me change it, I'll be back in minutes.

AzraelSec · 2023-04-15T09:05:23Z

@stefanhaller @jesseduffield I should have correctly moved the focus after selecting an interactive rebase 👍🏻 I've also changed the integration test to include the focus check to avoid any unintentional change in the future.

jesseduffield · 2023-04-15T09:17:14Z

Thanks for making this @AzraelSec !

AzraelSec · 2023-04-15T09:19:37Z

@jesseduffield @stefanhaller thanks to you for your feedback! Hope to continue contributing to lazygit in the future 💪

jesseduffield · 2023-04-15T09:20:06Z

Looking forward to it

AzraelSec force-pushed the rebase-with-todo-edit branch 2 times, most recently from 2a5591b to 52e7f3b Compare January 17, 2023 00:45

AzraelSec force-pushed the rebase-with-todo-edit branch from 52e7f3b to 5191e04 Compare January 26, 2023 09:39

joihn mentioned this pull request Mar 28, 2023

[Feature request] interactive rebase onto another branch #2517

Closed

AzraelSec force-pushed the rebase-with-todo-edit branch from 5191e04 to 282f8bb Compare March 28, 2023 22:53

AzraelSec force-pushed the rebase-with-todo-edit branch from 282f8bb to 51ec8c6 Compare March 29, 2023 13:44

jesseduffield requested changes Apr 2, 2023

View reviewed changes

AzraelSec force-pushed the rebase-with-todo-edit branch from 4ae485b to 8cfa431 Compare April 2, 2023 18:48

AzraelSec requested a review from jesseduffield April 2, 2023 21:24

stefanhaller mentioned this pull request Apr 14, 2023

Refactor for better encapsulation #2519

Merged

7 tasks

AzraelSec force-pushed the rebase-with-todo-edit branch 2 times, most recently from 757c1aa to c22c77c Compare April 14, 2023 17:14

AzraelSec and others added 6 commits April 15, 2023 17:26

feat: let interactive rebase prepend commands to the default todo file

368f9c8

feat: allow to perform a rebase with breaking before the first commit

a3fdf91

test: update the UI to follow the new rebase type selection instead o…

3422b1e

…f confirm the previous popup

extract out function

711be78

refactor: introduce a struct to pack the

ddcd6be

`PrepareInteractiveRebaseCommand` function

test: add integration test to verify the interactive rebase correctly…

b82b6a2

… work

jesseduffield force-pushed the rebase-with-todo-edit branch from a07f0d7 to 6b85134 Compare April 15, 2023 07:26

use lowercase text for menu items (as we're still yet to standardise …

8f1f712

…on 'Sentence case')

jesseduffield force-pushed the rebase-with-todo-edit branch from 6b85134 to 8f1f712 Compare April 15, 2023 07:29

stefanhaller mentioned this pull request Apr 15, 2023

Support stacked branches #2552

Merged

6 tasks

AzraelSec added 2 commits April 15, 2023 10:42

chore: add focus on local commits after interactively rebase

28501fb

test: check focus on commits after performing an advanced rebase

08a445e

jesseduffield merged commit 1efb565 into jesseduffield:master Apr 15, 2023

OliverJAsh mentioned this pull request May 3, 2023

Suggestion: shortcut to interactive rebase on top of selected commit/branch #733

Closed

Uh oh!

Conversation

AzraelSec commented Jan 17, 2023

Uh oh!

jesseduffield commented Jan 29, 2023

Uh oh!

AzraelSec commented Jan 29, 2023

Uh oh!

jesseduffield commented Jan 29, 2023

Uh oh!

joihn commented Mar 28, 2023

Uh oh!

AzraelSec commented Mar 29, 2023

Uh oh!

AzraelSec commented Mar 29, 2023

Uh oh!

github-actions bot commented Apr 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jesseduffield left a comment

Choose a reason for hiding this comment

Uh oh!

jesseduffield Apr 2, 2023

Choose a reason for hiding this comment

Uh oh!

jesseduffield Apr 2, 2023

Choose a reason for hiding this comment

Uh oh!

jesseduffield Apr 2, 2023

Choose a reason for hiding this comment

Uh oh!

AzraelSec Apr 2, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jesseduffield commented Apr 2, 2023

Uh oh!

stefanhaller commented Apr 2, 2023

Uh oh!

AzraelSec commented Apr 2, 2023

Uh oh!

stefanhaller commented Apr 3, 2023

Uh oh!

AzraelSec commented Apr 12, 2023

Uh oh!

stefanhaller commented Apr 12, 2023

Uh oh!

AzraelSec commented Apr 13, 2023

Uh oh!

jesseduffield commented Apr 13, 2023

Uh oh!

stefanhaller commented Apr 14, 2023

Uh oh!

stefanhaller commented Apr 14, 2023

Uh oh!

AzraelSec commented Apr 14, 2023

Uh oh!

jesseduffield commented Apr 15, 2023

Uh oh!

stefanhaller commented Apr 15, 2023

Uh oh!

jesseduffield commented Apr 15, 2023

Uh oh!

AzraelSec commented Apr 15, 2023

Uh oh!

AzraelSec commented Apr 15, 2023

Uh oh!

jesseduffield commented Apr 15, 2023

Uh oh!

AzraelSec commented Apr 15, 2023

Uh oh!

jesseduffield commented Apr 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

github-actions bot commented Apr 2, 2023 •

edited

Loading