More robust todo rewriting #2547

stefanhaller · 2023-04-14T06:32:51Z

PR Description

This fixes editing a rebase todo commit and moving a todo commit up/down for the case where the lines in the git-rebase-todo file don't correspond one-to-one with what lazygit shows in its gui.

Plus a bunch of other fixes and minor improvements; see the individual commit messages for details.

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

github-actions · 2023-04-14T06:44:56Z

Uffizzi Preview deployment-22227 was deleted.

jesseduffield

Great work :) I appreciate the attention to detail, and the extra effort you went to do convert those strings into enums.

I've left some comments

jesseduffield · 2023-04-14T11:27:59Z

pkg/commands/models/commit.go

 )

+const (
+ActionNone todo.TodoCommand = 0


worth leaving a comment explaining that conveniently for us, the todo package starts the enum at 1, and given that it doesn't have a none value, we're setting ours to 0

I mentioned this in the commit message, but I can add it to the code too: c8ad201

jesseduffield · 2023-04-14T11:51:04Z

pkg/gui/presentation/commits.go

 } else {
 if len(commit.Tags) > 0 {
 tagString = theme.DiffTerminalColor.SetBold().Sprint(strings.Join(commit.Tags, " ")) + " "
+} else if commit.ExtraInfo != "" && commit.ExtraInfo != "(HEAD)" {


So far, ExtraInfo has been purely for display, but here we've got conditional logic based on its value. Could we store this info on a separate field? Also, I reckon the head commit should be green, with the heads of other branches being magenta, so that if you've scrolled down the commits panel it's unambiguous that you're not at the top.

As for the colors: I modeled these after what gets displayed in the expanded commits view, just replacing the actual text with a *. So I would find it strange if the colors change when you expand the view.

Considering this, I should probably remove the extra condition for ExtraInfo being "(HEAD)", since the expanded view shows it too. I initially found it redundant because you already have the big bold <-- YOU ARE HERE --- display, but being consistent with the expanded view is maybe more important. See 8ec66f5.

With this, my feeling is that it's now fine to use ExtraInfo here, and there's no need to store anything in another field. Basically we're saying "we're printing an abbreviated version of ExtraInfo here, expand the view if you want to see the full text of it".

jesseduffield · 2023-04-14T12:02:35Z

pkg/integration/components/runner.go

 return nil
 }

+gitVersion, err := getGitVersion()


running git --version takes around 10-40ms which will add up as we add more tests. Could we move this out so that we only need to call it once?

Sure, see 758df2a.

jesseduffield · 2023-04-14T12:04:03Z

pkg/integration/components/test.go

+type GitVersionRestriction struct {
+// Only one of these fields can be non-empty; use functions below to construct
+from string
+before string


I'd either have from/to or before/after for consistency. I prefer before/after. I'd also make clear here whether the values are inclusive or not

But I don't want them to be consistent (or symmetric), because one is inclusive and the other is not. From/To or Before/After don't express that, but From/Before does (or at least they do to me), and I think they read quite naturally when used in tests. If you don't like them, I'd rather go back to GreatThanOrEqual/LessThan.

Makes sense

jesseduffield · 2023-04-14T12:05:23Z

pkg/integration/components/test.go

+v, err := git_commands.ParseGitVersion(str)
+if err != nil {
+// No good way to return an error from in here
+panic("Invalid git version string: " + str)


This is fine: I've got no issues panicking in these tests when there's a programmer error

Cool, I wasn't sure. In that case I think we should do it for the other two cases too, and not return an error at all, this simplifies things a little bit. See e02983d.

jesseduffield · 2023-04-14T13:16:33Z

pkg/utils/rebaseTodo_test.go

+t.Run(s.testName, func(t *testing.T) {
+fileName := filepath.Join(os.TempDir(), "git-rebase-todo-test")
+
+err := os.WriteFile(fileName, []byte(strings.Join(s.todos, "\n")+"\n"), 0o600)


I would refactor the function to take the contents of the file, and to return the updated contents of the file (or just the rearranged todos, whichever is easier), so that in these unit tests we don't actually need to hit the filesystem

Makes sense. See e9999f3

I just realized that I need the functions taking a filename from another place in a future PR, so I added another fixup commit to move them back to the utils file, and rename the other ones to lowercase. See f10aa61.

jesseduffield · 2023-04-14T13:32:21Z

pkg/utils/rebaseTodo.go

+// pick and later in a merge)
+if t.Command == action && equalShas(t.Commit, sha) {
+// The todos are ordered backwards compared to our model commits, so
+// actually move the commit _up_ in the todos slice (i.e. towards 0)


I wonder if it would simplify the code to go todos = lo.Reverse(todos) before doing anything else, and then reverse again at the end?

This would just change a few + to - or v.v., but wouldn't otherwise change the complexity of the algorithm.

However, this is an excellent idea for DRY'ing the two functions, see below.

jesseduffield · 2023-04-14T13:32:37Z

pkg/utils/rebaseTodo.go

+}
+
+for i := range todos {
+t := &todos[i]


Could we instead go for i, t := range todos {?

Erm, yes. 😅 Also squashed into e9999f3.

jesseduffield · 2023-04-14T13:35:15Z

pkg/utils/rebaseTodo.go

+return fmt.Errorf("Todo %s not found in git-rebase-todo", sha)
+}
+
+func MoveTodoUp(fileName string, sha string, action todo.TodoCommand) error {


I get the feeling we can DRY this up given a lot of similarity between the two functions, but I haven't thought very hard about it

Yes, see e9999f3. 😄

jesseduffield · 2023-04-14T13:36:47Z

pkg/utils/rebaseTodo.go

+
+if j == len(todos) {
+// We expect callers to guard against this
+return fmt.Errorf("Trying to move last commit up")


I know we don't expect to return this error but I'd phrase it as The last commit cannot be moved up

Given that this message is now used for moving both up and down, I tried to come up with a more generic phrasing: "Destination position for moving todo out of range". Let me know if you have a better suggestion. (I don't think it's worth it to pass in the right error message for the direction we're moving in.)

stefanhaller · 2023-04-14T15:29:27Z

Thanks for the feedback! I think I addressed it all, so ready for another round.

jesseduffield

LGTM. I've just pushed a couple fixups based on me playing around with the code locally. Let me know what you think

jesseduffield · 2023-04-14T23:19:33Z

pkg/commands/git_commands/rebase.go

+// pick and later in a merge)
+if t.Command == commit.Action && t.Commit == commit.Sha {
+t.Command = action
+return utils.WriteRebaseTodoFile(fileName, todos)


Ah in that case I'm happy to go with that and re-evaluate once I've seen the whole thing

jesseduffield · 2023-04-14T23:21:48Z

pkg/integration/components/test.go

+type GitVersionRestriction struct {
+// Only one of these fields can be non-empty; use functions below to construct
+from string
+before string


Makes sense

jesseduffield · 2023-04-14T23:23:24Z

pkg/integration/tests/interactive_rebase/drop_todo_commit_with_update_ref.go

+shell.
+CreateNCommits(3).
+NewBranch("mybranch").
+CreateNCommitsStartingAt(3, 4)


Ah that makes sense. I assumed the first argument would be the starting number just because it's right next to StartingAt( but N comes first in the name so that makes sense to come first in the arguments.

jesseduffield · 2023-04-15T00:18:21Z

pkg/utils/slice.go

+
+// Returns a new slice with the element at index 'from' moved to index 'to'.
+// Does not mutate original slice.
+func MoveElement[T any](slice []T, from int, to int) []T {


Down the line I want to allow moving multiple commits at once as opposed to just one, but once we implement that it'll be trivially easy to update this PR's code to handle it

This is unrelated to the changes in this PR, but since we are doing the same thing for the commit.Action field in the next commit, it makes sense to do it for Status too for consistency. Modelling this as an enum feels more natural than modelling it as a string, since there's a finite set of possible values. And it saves a little bit of memory (not very much, since none of the strings were heap-allocated, but still).

The main reason for doing this (besides the reasons given for Status in the previous commit) is that it allows us to easily convert from TodoCommand to Action and back. This will be needed later in the branch. Fortunately, TodoCommand is one-based, so this allows us to add an ActionNone constant with the value 0.

So far the algorithm worked on the assumption that the output of the "git show" command corresponds one-to-one to the lines of the rebase-todo file. This assumption doesn't hold once we start to include todo lines that don't have a sha (like update-ref), or when the todo file contains multiple entries for the same sha. This should never happen normally, but it can if users manually edit the todo file and duplicate a line.

Useful when working with stacked branches.

This is useful when working with stacked branches, because you can now move "pick" entries across an update-ref command and you can tell exactly which branch the commit will end up in. It's also useful to spot situations where the --update-refs option didn't work as desired. For example, if you duplicate a branch and want to rebase only one of the branches but not the other (maybe for testing); if you have rebase.updateRefs=true in your git config, then rebasing one branch will move the other branch along. To solve this we'll have to introduce a way to delete the update-ref entry (maybe by hitting backspace?); this is out of scope for this PR, so for now users will have to type "git rebase --edit-todo" into the custom command prompt to sort this out. We will also have to prevent users from trying to turn update-ref commands into other commands like "pick" or "drop"; we'll do this later in this branch.

It can be used to specify which git versions a given test should or should not run on.

…e-ref The test shows how we are accidentally dropping the wrong commit in this case. We'll fix that in the next commit.

It used to work on the assumption that rebasing commits in lazygit's model correspond one-to-one to lines in the git-rebase-todo file, which isn't necessarily true (e.g. when users use "git rebase --edit-todo" at the custom command prompt and add a "break" between lines).

We already show "merge" todo entries when starting an interactive rebase with --rebase-merges outside of lazygit. Changing the type of a merge entry to "pick" or "edit" doesn't make sense and shouldn't be allowed. Earlier in this branch we have started to show "update-ref" entries, these can't be changed either (they can be moved, though). You might argue that it should be possible to change them to "drop", but in the case of "update-ref" this doesn't make sense either, because "drop" needs a Sha and we don't have one here. Also, you would then be able to later change it back to "pick", so we would have to remember that this isn't allowed for this particular drop entry; that's messy, so just disallow all editing.

stefanhaller · 2023-04-15T06:40:35Z

Thanks for the fixups, those are very nice improvements. I squashed and rebased on master again, ready to go from my side.

stefanhaller force-pushed the more-robust-todo-rewriting branch from 12b5880 to 3b2eaaa Compare April 14, 2023 06:42

This was referenced Apr 14, 2023

Interactive rebase with in-editor TODO edit #2370

Merged

Refactor for better encapsulation #2519

Merged

jesseduffield requested changes Apr 14, 2023

View reviewed changes

jesseduffield approved these changes Apr 15, 2023

View reviewed changes

jesseduffield reviewed Apr 15, 2023

View reviewed changes

stefanhaller added 11 commits April 15, 2023 08:36

Bump github.com/fsmiamoto/git-todo-parser to latest main version

62c5c32

Visualize branch heads in commits panel

740474c

Useful when working with stacked branches.

Add GitVersion field to NewIntegrationTestArgs

a304fed

It can be used to specify which git versions a given test should or should not run on.

Add integration test for dropping a todo commit when there's an updat…

860a8d1

…e-ref The test shows how we are accidentally dropping the wrong commit in this case. We'll fix that in the next commit.

Make moving todo commits more robust

dc4e88f

stefanhaller force-pushed the more-robust-todo-rewriting branch from 357064e to d675eb6 Compare April 15, 2023 06:37

jesseduffield merged commit 46718e2 into jesseduffield:master Apr 15, 2023

stefanhaller deleted the more-robust-todo-rewriting branch April 15, 2023 07:43

Uh oh!

More robust todo rewriting #2547

More robust todo rewriting #2547

Uh oh!

Conversation

stefanhaller commented Apr 14, 2023

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

Uh oh!

jesseduffield left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jesseduffield Apr 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jesseduffield Apr 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanhaller commented Apr 14, 2023

jesseduffield left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanhaller commented Apr 15, 2023

Labels

2 participants

github-actions bot commented Apr 14, 2023 •

edited

Loading

jesseduffield Apr 14, 2023 •

edited

Loading

jesseduffield Apr 14, 2023 •

edited

Loading