Improve undo action to restore files upon undoing a commit

[gabriellanata]

• PR Description

Right now, undoing a commit performs a hard reset, which also discards all the changes from that commit. This PR adds new config options (and a new undo section) which allow users to choose between hard and soft reset modes when undoing commits.

Personally, I think that the default should be soft, because the state before the commit had the files, so undoing a commit should put the files where they were before. But this PR keeps hard as the default and does not change current behavior.

• 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)
• If a new UserConfig entry was added, make sure it can be hot-reloaded (see here)
• Docs have been updated if necessary
• You've read through your own file changes for silly mistakes etc

[gabriellanata]

Note: I was not able to get the tests to run on a codespace. It shows a "POTENTIAL DEADLOCK" error every time I run it, but the other pre-existing tests next to this one show the same error, so i dont think it's an issue with my newly added tests. I'm not a go programmer so i'm not sure how to debug that further. This is the error I was getting:

 *  Executing task: go generate pkg/integration/tests/tests.go && go run cmd/integration_test/main.go cli pkg/integration/tests/undo/undo_commit_soft.go 

Generating test_list.go...
POTENTIAL DEADLOCK: Recursive locking:
current goroutine 2 lock 0xc0002f59d0
../../../../../../pkg/gui/controllers/helpers/refresh_helper.go:447 helpers.(*RefreshHelper).refreshBranches { self.c.Mutexes().RefreshingBranchesMutex.Lock() } <<<<<
../../../../../../pkg/gui/controllers/helpers/refresh_helper.go:446 helpers.(*RefreshHelper).refreshBranches { func (self *RefreshHelper) refreshBranches(refreshWorktrees bool, keepBranchSelectionIndex bool, loadBehindCounts bool) { }
../../../../../../pkg/gui/controllers/helpers/refresh_helper.go:258 helpers.(*RefreshHelper).refreshReflogCommitsConsideringStartup.func1 { self.refreshBranches(false, true, true) }
../../../../../../vendor/github.com/jesseduffield/gocui/gui.go:702 gocui.(*Gui).onWorkerAux {  }
../../../../../../vendor/github.com/jesseduffield/gocui/task.go:22 gocui.(*TaskImpl).Done { func (self *TaskImpl) Done() { }

Previous place where the lock was grabbed (same goroutine)
../../../../../../pkg/gui/controllers/helpers/refresh_helper.go:447 helpers.(*RefreshHelper).refreshBranches { self.c.Mutexes().RefreshingBranchesMutex.Lock() } <<<<<
../../../../../../pkg/gui/controllers/helpers/refresh_helper.go:446 helpers.(*RefreshHelper).refreshBranches { func (self *RefreshHelper) refreshBranches(refreshWorktrees bool, keepBranchSelectionIndex bool, loadBehindCounts bool) { }
../../../../../../pkg/gui/controllers/helpers/refresh_helper.go:273 helpers.(*RefreshHelper).refreshReflogAndBranches { self.refreshBranches(refreshWorktrees, keepBranchSelectionIndex, loadBehindCounts) }
../../../../../../pkg/gui/controllers/helpers/refresh_helper.go:129 helpers.(*RefreshHelper).Refresh.func2.2 { if self.c.AppState.LocalBranchSortOrder == "recency" { }
../../../../../../pkg/gui/controllers/helpers/refresh_helper.go:107 helpers.(*RefreshHelper).Refresh.func2.1.1 { f() }
../../../../../../vendor/github.com/jesseduffield/gocui/gui.go:702 gocui.(*Gui).onWorkerAux {  }
../../../../../../vendor/github.com/jesseduffield/gocui/task.go:22 gocui.(*TaskImpl).Done { func (self *TaskImpl) Done() { }

Other goroutines holding locks:
goroutine 2 lock 0xc0002f59e0
../../../../../../pkg/gui/controllers/helpers/refresh_helper.go:322 helpers.(*RefreshHelper).refreshCommitsWithLimit { self.c.Mutexes().LocalCommitsMutex.Lock() } <<<<<
../../../../../../pkg/gui/controllers/helpers/refresh_helper.go:321 helpers.(*RefreshHelper).refreshCommitsWithLimit { func (self *RefreshHelper) refreshCommitsWithLimit() error { }
../../../../../../pkg/gui/controllers/helpers/refresh_helper.go:277 helpers.(*RefreshHelper).refreshCommitsAndCommitFiles { _ = self.refreshCommitsWithLimit() }
../../../../../../pkg/gui/controllers/helpers/refresh_helper.go:107 helpers.(*RefreshHelper).Refresh.func2.1.1 { f() }
../../../../../../vendor/github.com/jesseduffield/gocui/gui.go:702 gocui.(*Gui).onWorkerAux {  }
../../../../../../vendor/github.com/jesseduffield/gocui/task.go:22 gocui.(*TaskImpl).Done { func (self *TaskImpl) Done() { }


2025/01/13 08:11:04 exit status 2
exit status 1

[jesseduffield]

I'd like to explore how we can do this without adding any new config items, because our config is already too big. Could we instead prompt the user to select soft/mixed/hard whenever they go to undo a commit? Or should it always do soft?

As for rebases, I don't see why we would ever want to do a soft/mixed reset. What use case are you thinking of there? Is it just for the sake of not clobbering changes in the filetree when resetting? Because if so, a better solution could be to stash the working tree changes and then unstash after.

Keen to get your thoughts on this too @stefanhaller

[stefanhaller]

First, I agree about trying to avoid adding new config options if possible.

Second, I feel that @gabriellanata is "abusing" undo for something that it wasn't meant for. For me, undo means "put my repo back into the state that it was in before the last operation (no matter how I arrived at that state)". Ideally this would be a command that is built into git (git undo), like in some other more modern VCSs (e.g. jujutsu). We are approximating it with git reset --hard, but it's not conceptually the same as git reset for me.

If you want to do a reset but keep your changes, I find it more appropriate to actually perform a reset operation, i.e. select the commit that you want to reset to, and press g. This gives you control over how hard you want your reset to be (with "mixed" being the default), and it also has the advantage that it gives you control over how many commits you want to get rid of. Useful if you made a bunch of WIP commits and then want to go back and create real commits from those changes.

Because if so, a better solution could be to stash the working tree changes and then unstash after

I agree, but it seems we already do that. :)

[jesseduffield]

I'm interested in playing out what the downsides are of always soft-resetting when undoing a commit. Suppose that I:

1. start on branch A
2. checkout branch B
3. commit some code
4. commit some more code

Now I undo all of these:
4) I get the code back in my working tree
3) same deal
2) branch A is checked out again

This could cause an issue if branch A has conflicts with what's in your working tree, and the reason this isn't clean is that there's no undo action for writing the code in the first place. But, this doesn't seem so bad to me, in fact it seems superior to the current behaviour.

What if we commit some code, the rebase onto another branch, then commit some more code. Undoing that would involve a soft reset, then a hard reset (but with a stash pushed/popped), then another soft reset. Again, this doesn't seem so bad. It sounds like the worst that could happen is conflicts.

Reflecting on my own usage, I typically only use undo after doing some rebase or cherry pick operation. If I had just committed some code and I wanted to easily undo that, I don't see an issue with that being a soft reset.

Can you guys think of any especially egregious situations that could come about if we enforce that undoing a commit always uses a soft reset?

[stefanhaller]

Egregious, no, but confusing, perhaps. When I use undo I typically don't pay much attention to what operation it is that I'm undoing, and I would find it confusing if undoing sometimes leaves stuff in my working copy and other times it doesn't.

[gabriellanata]

Second, I feel that @gabriellanata is "abusing" undo for something that it wasn't meant for. For me, undo means "put my repo back into the state that it was in before the last operation (no matter how I arrived at that state)". Ideally this would be a command that is built into git (git undo), like in some other more modern VCSs (e.g. jujutsu). We are approximating it with git reset --hard, but it's not conceptually the same as git reset for me.

I completely agree with the "put my repo back into the state that it was in before the last operation (no matter how I arrived at that state)". But IMO this is not what is happening right now.

1. My repo had staged changes on the working directory
2. I commit all my changes
3. I undo
4. My working directory is empty and the staged changes were lost

To me, that is not the state that my repo was in before the last operation as I would have expected my changes to be back in the working directory. One simple example which is what i wanted to do: I committed my changes and immediately realized i committed one additional file accidentally. So I ran undo which removed the commit but didn't put my files back.

When I use undo I typically don't pay much attention to what operation it is that I'm undoing, and I would find it confusing if undoing sometimes leaves stuff in my working copy and other times it doesn't.

I find it more confusing if i had files on my working directory before the previous operation and performing an undo doesn't put them back. The first time I ran it I even had to check multiple times if I had done something wrong.

I'd like to explore how we can do this without adding any new config items, because our config is already too big.

I agree that maybe rebase undo config is not a requirement. I added for completeness of the configuration options, but if the concern is bloating then it can definitely be removed.

I'm interested in playing out what the downsides are of always soft-resetting when undoing a commit.

As i said in the original PR description and on the example above, this is IMO what makes the most sense. I was adding it as a config option because I didn't want to break existing workflows, but that is up to you. Happy to update my PR to whatever you prefer

[stefanhaller]

Hm, ok. I still find it surprising and confusing, but that's not a blocking objection if I'm the only one who feels this way.

I committed my changes and immediately realized i committed one additional file accidentally. So I ran undo

For the record, what I do in such a case is to press enter on the commit, press space on the file that was accidentally added, and press <c-p> i to move it to the index. The advantage is that it preserves my meticulously crafted commit message.

[jesseduffield]

Something that just occurred to me: a few times now I've accidentally amended the wrong commit (whether that's amending the head commit or another commit) and whenever I do that it is a real pain trying to undo the change, because often it's not as simple as just picking one or two files to pull back out: sometimes a file you committed was already part of the commit you amended so you need to get very surgical to undo the operation. Last time this happened I used a diff between the current commit and the old reflog commit and then copied the changes from that via a custom patch and then hard reset to the old commit and applied the changes, but it was a real hassle. If I could just press undo to get the same result, that would be a huge improvement.

If I look at the reflog in both cases, then soft-resetting on a commit operation (and hard-resetting with auto-stash on the rebase operation) works perfectly:

• amend head commit: this is just a single action: 'commit (amend)' so you can soft reset that and you're back to the original state
• amend deeper commit: this is two actions: first a commit operation for a fixup commit, and then a rebase to apply the fixup in the desired location. If you hard reset with the rebase (auto-stashing) and then soft reset the commit, you're back to the original state.

So I am thinking that an undo action on a commit should always be a soft reset, and rebase should stay as-is (hard reset with autostash). The only time you as a user perform a commit operation is when you have code in your working tree that you commit. Undoing that action via hard reset necessarily means you're not properly undoing the action because now that code is gone.

[stefanhaller]

That makes a lot of sense.

[gabriellanata]

I updated the PR, let me know what you think

[stefanhaller]

Ok, I'm persuaded now that soft-resetting when undoing a commit is better. The one situation that won me over is amending the head commit. For all other situations of creating commits it's much less of a deal for me, because it's so easy to explicitly reset to the commit before instead of undoing; but after an amend I don't see the commit to reset to unless I manually look for it in the reflog view, which is cumbersome. So that's a nice improvement indeed.

There's something strange about redoing though. When I undo an amend I get the previous changes back (staged), but when I then immediately redo, I then still have those changes in the unstaged changes, but the reverse of those changes staged. I find this very confusing. At first I suspected this might be improved by doing a mixed reset rather than soft, but that makes it so that I only have the reverse of the original changes as unstaged changes. This doesn't make sense to me, and after thinking about it more I am coming to the conclusion that soft-resetting should only be done for undo; redo should still hard-reset.

(Testing this a bit more with normal, non-amend commits: same here. After redoing I want to get back to the state after making the commit, at which point my working copy was empty. It doesn't make sense to put the reverse of the committed changes in the working copy when redoing a commit.)

A few remarks about the code changes:

• In your initial commit you had a nice way of avoiding code duplication between the COMMIT and REBASE cases by having variables for the two things that are different between the two cases; unfortunately you got rid of that and duplicated the code now. I think it would be nice to go back to having the code only once, with variables for resetMode and resetPrompt.
• Please don't touch files in pkg/i18n/translations. It is nice that you want to adapt those proactively, but we don't change these files here; the translations are maintained on https://crowdin.com/project/lazygit, so you'd have to make those changes there after the PR is merged (and after the english language file is uploaded to Crowdin, which I don't do often enough...).
• I'd prefer it if you rebased your branch onto master instead of merging master into your branch.

[jesseduffield]

The redo case is interesting. Playing around with it locally where I have two changed files, and only commit one of them: hard+autostash indeed is better than soft+autostash. Soft without autostash seems good too (faster, no prompt required) but there's probably cases where it's problematic like if you've made changes since undoing that would cause conflicts (an auto-stash will handle those conflicts properly)

and after the english language file is uploaded to Crowdin, which I don't do often enough...

We should set up a cron via github actions for that, or even do it for every merged PR.

[stefanhaller]

Soft without autostash seems good too

For the changes that you didn't commit, yes; those get preserved in the same way either with autostash or with soft reset. But for the changes in the commit that you are redoing it doesn't make sense to preserve them in reverse in the index after redo, like soft reset does.

and after the english language file is uploaded to Crowdin, which I don't do often enough...

We should set up a cron via github actions for that, or even do it for every merged PR.

That would depend on getting the crowdin command-line tool to work, which I didn't succeed with so far.

[jesseduffield]

For the changes that you didn't commit, yes; those get preserved in the same way either with autostash or with soft reset. But for the changes in the commit that you are redoing it doesn't make sense to preserve them in reverse in the index after redo, like soft reset does.

They won't be preserved though: I tested this myself and the outcome with soft without autosquash was identical to hard with autosquash. Nevertheless, hard+autosquash is probably more robust.

That would depend on getting the crowdin command-line tool to work, which I didn't succeed with so far.

Ah, yes

[stefanhaller]

They won't be preserved though: I tested this myself and the outcome with soft without autosquash was identical to hard with autosquash.

Only if you undo and then redo and don't touch your working copy in between, because then the two soft resets cancel each other out. My use case was:

• make a commit
• undo it, see the changes in the working copy (good in case I want to commit them again)
• discard the changes, because I really wanted the commit gone for good
• nah, changed my mind, I do want the commit back after all, so redo
• now the changes of that commit are in my working copy, reversed

[jesseduffield]

Yeah, that makes sense

[gabriellanata]

Updated the PR with the latest comments

[stefanhaller]

This looks pretty good now. I took the liberty of pushing two commits, one that fixes formatting (it's a good idea to configure your editor to format automatically on save), and one that slightly simplifies the code in a minor way. It would be good if you could squash all commits down into one, with a commit message that describes the latest approach.

One last thing though: we are still doing the auto-stash thing even for the soft reset. That's unnecessary, since soft-reset already preserves any modifications in the working copy (whether staged or unstaged). An easy fix could be to add && options.Mode == "hard" to the condition if dirtyWorkingTree in resetWithAutoStash, but maybe there are cleaner ways to say that.

[jesseduffield]

I have joined the party with my own commit. Let me know your thoughts

[stefanhaller]

Looks great. I added one more commit to revert an unnecessary earlier renaming, so that we get a smaller overall diff.

[jesseduffield]

LGTM

[jesseduffield]

Something that I'm now wondering: does the auto-stash even need user confirmation? It seems silly to prompt when redoing a commit given that you're guaranteed to have changes in your working tree so you couldn't perform the redo without an auto-stash. We also already mention the auto-stash in the initial prompt: hitting the user with another prompt feels excessive.

[stefanhaller]

Something that I'm now wondering: does the auto-stash even need user confirmation?

I have wondered the same for other situations where we offer to auto-stash, e.g. when checking out a different branch. No strong opinion either way though. I guess the prompt is nice in the case that popping the stash conflicts, because then it's a bit clearer what's going on (i.e. "you asked for it" :)

If anything, what bothers me is that we mention auto-stashing twice, in the original prompt and then again in the auto-stash prompt. So if we decide we want to keep the second prompt, we should at least remove the "An auto-stash will be performed if necessary" from the first one.

It seems silly to prompt when redoing a commit given that you're guaranteed to have changes in your working tree so you couldn't perform the redo without an auto-stash.

That's not true though, see above.

[jesseduffield]

I've added a commit which removes the second prompt and adds a mention of a potential auto-stash to the first prompt for checkout actions (which was not previously present)

That's not true though, #4167 (comment).

Ah, yes.

[stefanhaller]

Works for me.

Note that when I mentioned checking out a branch in my previous comment, I didn't mean undoing a checkout, but actually checking out by pressing space. But there the situation is different because there is no initial prompt for that operation, so I guess the extra auto-stash confirmation is justified in that case.

Another thing that I just noticed is that reverting a commit can't be undone; it will reset to the state before whatever other action was done before that. Would be nice to support this, but it's unrelated to this PR.

[jesseduffield]

Note that when I mentioned checking out a branch in my previous comment, I didn't mean undoing a checkout, but actually checking out by pressing space. But there the situation is different because there is no initial prompt for that operation, so I guess the extra auto-stash confirmation is justified in that case.

Agreed.

Another thing that I just noticed is that reverting a commit can't be undone; it will reset to the state before whatever other action was done before that. Would be nice to support this, but it's unrelated to this PR.

Also agreed.

I'm gonna go ahead and squash these commits and merge.