Rebase onto branch from a marked base commit

[stefanhaller]

• PR Description

Introduce a new command (shift-B by default) to mark the base commit for the next rebase. After that, invoking a rebase in the branches panel rebases from that base commit, effectively performing a git rebase --onto <selected-branch> <marked-base>.

Addresses #987.

• 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

[stefanhaller]

Marked the commit as "RFC" because this is not the most beautiful design. Suggestions for a better UI welcome; but I prefer this very explicit and in-your-face visualization over just coloring the sha.

[stefanhaller]

Another option would be to somehow darken the text of the commits from the marked commit down. Something like this (except for the background color, I'm not good with Photoshop 😅):

Unfortunately I have no idea how to achieve that, suggestions welcome.

[jesseduffield]

Haven't looked at the code but I'd like your thoughts on a more general pattern where you have a single concept of a 'marked' commit and then you have different actions associated with marked commits. For example:

• you can mark a commit in order to rebase from it
• you mark one or more in order to cherry pick them
• you can mark a commit in order to diff against it
• you can mark several commits to perform a bulk operation on them e.g. drop them, move them

This concept could be extended to other views as well e.g. mark a bunch of files and then discard them (which would spare some keybindings if you can do a drag-selection like you can when staging lines with the 'v' key)

Some pros/cons of a unified approach:
Pros:

• only one keybinding to remember for marking an item
• easy to carry intuitions from one mark-oriented action to another

Cons:

• if you need to do several things at once you won't be able to e.g. cherry pick while diffing, but I can't think of any such realistic examples
• the UI of a marked commit will be consistent, but that also means it's less targeted (can't show a label specific to the operation to be performed)
• you would need to have some error handling e.g. raise an error if the user has marked more than one commit but wants to 'rebase from marked commit'

I think the pros outweigh the cons.

This is not to say that we can't go with this PR's approach for now, given it'll take some time to do a unified marking UX. Keen to get your thoughts though.

[stefanhaller]

Yes, I have suggested this before (see last bullet point of this comment). However, I'm no longer so sure whether it's really a great idea.

• marking a commit to diff against should really be a feature separate from the others. It would be very strange and confusing if you want to copy/paste commits, or mark commits for a bulk move, and the diff view starts to show funny diffs that make no sense while you move around.
• some of the operations are across views (copy/paste), others only make sense within a single view (bulk move). I feel that especially for bulk operations we want a different UX than simply marking arbitrary sets of shas in arbitrary views (more on that below).
• for rebase from marked commit, I'm not sure that implicitly using the marked commit if there is one isn't too error-prone, as you might still have a marked commit from the last copy/paste or bulk operation without realizing it. So this would probably need a separate "rebase from marked commit" command (which would have to show an error if there isn't one). The approach in this PR doesn't need that.
• I really like the more targeted UI, especially for the "Rebase from here" marker that this PR adds.

Thoughts on bulk operations: My feeling is that this should not use a permanent marker like copy/paste does; instead, there should just be a way to extend the selection (highlight) to more than one line. For a contiguous range of lines this wouldn't be too hard; the canonical way to do that in other apps is shift-arrow. In my fork I have a commit that adds this for the staging panel, instead of the v range mode. This would very naturally extend to list views, I think. The only challenge then is how to support non-contiguous selections; in other apps this is done with command-clicking on Mac, or ctrl-clicking on Windows. I haven't looked into whether this would be possible with tcell. But even if we only supported contiguous selections for now, this would already be a huge improvement, especially for bulk-moving commits.

[jesseduffield]

I had a draft response for this but I lost it!

I agree with you that there are differences in the different use cases that could call for different UX flows. I also agree that we may not need to care about non-contiguous multi-selections (nothing comes to my mind where you would need it other than cherry picking but with cherry picking you could use the contiguous multi-select as a way to then add the commits to larger non-contiguous set before pasting them.

I'd like to think more about this and do some POCs but I'm happy to go with your PR's approach now regardless.

I'm yet to look at the code though

[jesseduffield]

Looking good!

When we show rebase menu we should tell the user that we're using the --onto arg. We could do this by having the second column of the menu show the command we're going to run in full, which should be simple enough by extracting out a GetRebaseCmdObj method and calling that then calling .ToString() on it. Or if that's too long to fit on one line we could include it in the tooltip.

As for the design, it's tricky because git itself makes things confusing by not including the base commit in the rebase which is not intuitive at all to me. Perhaps all the commits that will actually be rebased can be highlighted in some way. I'll spend some time playing with it locally.

It's also occurred to me after reading this stack overflow thread that the --onto arg can be used within a branch as a way to remove some commits. This is a contrived example, but makes me think we should try to get a keybinding that we can use for rebasing with any item that supports it (commits, branches, tags, remote branches, etc). Currently, the 'r' keyword is reserved for rewording a commit. I like the idea of having 'r' be used for rebase and 'R' being used for rewording (we already have 'R' for renaming branches). We'd need to put the reword-in-lazygit and reword-in-editor options behind a menu but it's only one extra keypress. Of course this PR doesn't need to do that, I'm just talking out loud.

[jesseduffield]

This needs i18n

[stefanhaller]

Does it? All the existing LogCommand calls I could find use literal strings.

[jesseduffield]

Wow right you are. That's a huge oversight on my part. My rule has been that anything which the user can see within lazygit should be i18n'd. That means that logs that go to the log file (e.g. self.c.Log.Warnf(...)) don't need to i18n. But LogCommand and LogAction do.

Could you i18n this one and then I'll raise an issue for the rest?

[jesseduffield]

Issue: #2848

[stefanhaller]

I'll wait with this until after #2852 is merged, so that I know where to put the new text.

[stefanhaller]

OK, I changed my mind. 😄 Since this PR was otherwise ready, I decided to merge it now. @KarlHeitmann, can you please take care of this command too in your #2852 PR? Thanks!

[KarlHeitmann]

Ok! I will fix it tonight.

[jesseduffield]

I'm 99% sure this isn't working the way you expect: lo.Ternary evaluates both the if and else expressions before picking which one to return (just cos that's how the language works and lo has no way to get around that). So this will unconditionally call both rebase functions.

I believe lo has lo.TernaryFunc which you can use to only evaluate the expression if it's chosen, but at that point I wonder if it's just easier to use an if/else statement.

lo.Ternary should really only be used when it's dealing expressions that evaluate without side effects and without much time/memory footprint.

In this case I'd use an opts object in RebaseBranch and let it internally deal with the baseCommit arg.

[stefanhaller]

Ouch; fixed in c2afab3. I wonder why it worked though, I had used this locally for a while, and the tests are green too.

[jesseduffield]

Beats me. Maybe by some weird quirk of git logic they're mutually idempotent? Or the first one runs to completion because it's evaluated first, and then the second one runs, fails with an error due to the state being different, and the error is ignored?

[stefanhaller]

Almost. What happened: when baseCommit was non-empty, it first did the rebase --onto, then the normal rebase afterwards. The latter succeeded too, but was a no-op, because the branch was already sitting on its target. Conversely, when baseCommit was empty, the first command would fail, and then the second command would run and do its job. Phew.

[jesseduffield]

Wow, crazy!

[jesseduffield]

same as above

[jesseduffield]

Here's an example where using ternary is harmless

[jesseduffield]

We could highlight the commits to be rebased like so:

Or even without highlighting the commit sha:

[jesseduffield]

Here's the patch for that btw:

diff --git a/pkg/gui/presentation/commits.go b/pkg/gui/presentation/commits.go
index 575b27700..46ae93898 100644
--- a/pkg/gui/presentation/commits.go
+++ b/pkg/gui/presentation/commits.go
@@ -102,6 +102,7 @@ func GetCommitListDisplayStrings(
 
 	lines := make([][]string, 0, len(filteredCommits))
 	var bisectStatus BisectStatus
+	willBeRebased := markedBaseCommit == ""
 	for i, commit := range filteredCommits {
 		unfilteredIdx := i + startIdx
 		bisectStatus = getBisectStatus(unfilteredIdx, commit.Sha, bisectInfo, bisectBounds)
@@ -111,11 +112,15 @@ func GetCommitListDisplayStrings(
 			showYouAreHereLabel = false
 		}
 		isMarkedBaseCommit := commit.Sha != "" && commit.Sha == markedBaseCommit
+		if isMarkedBaseCommit {
+			willBeRebased = true
+		}
 		lines = append(lines, displayCommit(
 			common,
 			commit,
 			cherryPickedCommitShaSet,
 			isMarkedBaseCommit,
+			willBeRebased,
 			diffName,
 			timeFormat,
 			shortTimeFormat,
@@ -265,6 +270,7 @@ func displayCommit(
 	commit *models.Commit,
 	cherryPickedCommitShaSet *set.Set[string],
 	isMarkedBaseCommit bool,
+	willBeRebased bool,
 	diffName string,
 	timeFormat string,
 	shortTimeFormat string,
@@ -276,7 +282,7 @@ func displayCommit(
 	bisectInfo *git_commands.BisectInfo,
 	isYouAreHereCommit bool,
 ) []string {
-	shaColor := getShaColor(commit, diffName, cherryPickedCommitShaSet, isMarkedBaseCommit, bisectStatus, bisectInfo)
+	shaColor := getShaColor(commit, diffName, cherryPickedCommitShaSet, bisectStatus, bisectInfo)
 	bisectString := getBisectStatusText(bisectStatus, bisectInfo)
 
 	actionString := ""
@@ -310,6 +316,9 @@ func displayCommit(
 	} else if isMarkedBaseCommit {
 		rebaseFromHere := style.FgYellow.Sprint(common.Tr.MarkedCommitMarker)
 		name = fmt.Sprintf("%s %s", rebaseFromHere, name)
+	} else if !willBeRebased {
+		willBeRebased := style.FgYellow.Sprint("✓")
+		name = fmt.Sprintf("%s %s", willBeRebased, name)
 	}
 
 	authorFunc := authors.ShortAuthor
@@ -362,7 +371,6 @@ func getShaColor(
 	commit *models.Commit,
 	diffName string,
 	cherryPickedCommitShaSet *set.Set[string],
-	isMarkedBaseCommit bool,
 	bisectStatus BisectStatus,
 	bisectInfo *git_commands.BisectInfo,
 ) style.TextStyle {
@@ -386,12 +394,8 @@ func getShaColor(
 	default:
 	}
 
-	if isMarkedBaseCommit {
-		shaColor = theme.MarkedBaseCommitTextStyle
-	} else if diffed {
+	if diffed {
 		shaColor = theme.DiffTerminalColor
-	} else if cherryPickedCommitShaSet.Includes(commit.Sha) {
-		shaColor = theme.CherryPickedCommitTextStyle
 	}
 
 	return shaColor

[stefanhaller]

When we show rebase menu we should tell the user that we're using the --onto arg. We could do this by having the second column of the menu show the command we're going to run in full,

The full command is way too long to show in the menu; it includes options like --autostash, --keep-empty, --no-autosquash, --rebase-merges, none of which are interesting to see in the menu.

Personally I also don't feel it's necessary to show the corresponding git command; if we have a good, intuitive visualization, it will be clear enough, or actually even clearer than the command line.

It's also occurred to me after reading this stack overflow thread that the --onto arg can be used within a branch as a way to remove some commits. This is a contrived example, but makes me think we should try to get a keybinding that we can use for rebasing with any item that supports it (commits, branches, tags, remote branches, etc). Currently, the 'r' keyword is reserved for rewording a commit. I like the idea of having 'r' be used for rebase and 'R' being used for rewording (we already have 'R' for renaming branches). We'd need to put the reword-in-lazygit and reword-in-editor options behind a menu but it's only one extra keypress. Of course this PR doesn't need to do that, I'm just talking out loud.

That would be #2206.

[stefanhaller]

Here's the patch for that btw:

Thanks, applied in fba9de8.

[jesseduffield]

The full command is way too long to show in the menu; it includes options like --autostash, --keep-empty, --no-autosquash, --rebase-merges, none of which are interesting to see in the menu.

Personally I also don't feel it's necessary to show the corresponding git command; if we have a good, intuitive visualization, it will be clear enough, or actually even clearer than the command line.

Ah, I actually missed that we were already saying Rebase 'rebase-onto-from-base' from marked base onto 'master' in the menu title. I'm happy with that: just so long as there's something to remind the user about the marked base.

In general though, as a design principle, I don't want to be obscuring the underlying git commands. We received a lot of feedback about lack of understanding about what commands were being run back before we introduced the command log, and since introducing it people have said it's one of the things they like most about lazygit. I actually want to move towards something like magit where you have complete control over what flags are used for each command, though I want to do it in a way where you only invoke that functionality when you want it, so that the typical flows are still efficient.

[jesseduffield]

One final thing: The Rebase from here label is imperative which I find confusing given that the other labels we have for similar situations are descriptive e.g. 'YOU ARE HERE'. How about 'Will rebase from here'?

[jesseduffield]

That would be #2206.

I've closed that due to staleness and raised an issue which captures the problem/solution: #2850 (keen to get your thoughts on my proposal btw)

[stefanhaller]

Here's the patch for that btw:

Thanks, applied in fba9de8.

I hadn't looked closely enough at the patch; it broke the coloring of cherry-picked commits. I don't think that was intentional, but I'm not sure how else it happened. I restored it in 4a0860c.

[stefanhaller]

One final thing: The Rebase from here label is imperative which I find confusing given that the other labels we have for similar situations are descriptive e.g. 'YOU ARE HERE'. How about 'Will rebase from here'?

Makes sense, changed in 305979e.

[jesseduffield]

LGTM

[jesseduffield]

LGTM