Support range select for reverting commits

jesseduffield commented 9 months ago

Is your feature request related to a problem? Please describe. Sometimes it's useful to revert a range of commits.

Describe the solution you'd like In git you can revert commits A-B via git revert ^A..B. Now that we support range select, we should use it for handling reverts.

Additional context Currently if you try to revert an individual merge commit, you're asked to select a parent commit for the revert. When we're dealing with a range, the range could include multiple merge commits it's not clear to me what we should do in that situation. As such, for now let's just not support reverting a range of commits which includes merge commits (via GetDisabledReason).

ankit-pn commented 9 months ago

Hey i would love to work on this. it would be great if you assign this to me! thanks

jesseduffield commented 9 months ago

@ankit-pn done :)

uberrice commented 7 months ago

Hi @jesseduffield ,

As nothing was happening here for a while and I was looking for something to contribute, I took a crack at it.

See over at https://github.com/uberrice/lazygit/tree/3272---range-revert-commit

I haven't gone over the internationalization yet - I have put a TODO wherever that would be a thing to do; if my approach is seen as sane, I'd be happy to put that in as well.

I am still unsure about a few things:

I tried working with startIdx and endIdx rather than just the first and last list element - I ran into problems there, where for example for a list of 2 things, if I addressed with startIdx and endIdx-1, I got the same commit twice, but startIdx was already 0, and endIdx 2. So I went with just the first and last element of the selected commits - is that fine?
Visibility: To not clutter up the menu, it makes sense that 'revert' is not in the bottom bar. However, if we select a range, the tips at the bottom change. Is it possible to use some variation of DisplayOnScreen to only display it if a range is selected? -> Currently, it's set to be visible
I implemented the range support by basically just checking whether one or more commits are selected, and running the old revert code if that is the case. Would it be nicer to somehow integrate this all into a 'one-in-all' handler?
Is my canBeReverted correct? Not sure of all the edge cases
Git command: I'm not a full on git pro that reverts ranges often - to get an 'inclusive selection' I select the parent commit as well. Should that maybe be solved in another way?

stefanhaller commented 7 months ago

@uberrice Thanks for working on this!

One general remark first: I think we shouldn't add this feature before we did #1807. For a range of reverts it's easy to run into the situation where one of them conflicts, but lazygit currently has no way of detecting that state, and it doesn't let you abort or continue the revert operation. We need to fix this first. #1807 is pretty high up on my list of things to work on next, but I don't have a huge amount of time right now, so it might take a bit. This shouldn't stop you from continuing to work on this, we just wouldn't merge it yet.

On to your questions:

I wouldn't bother trying to use git's .. feature for specifying a range. You can pass a list of commits to git revert, so that's what I would do. Simply pass all the commits that get passed into the handler as selectedCommits (in reverse order, they are passed in top to bottom). This way the handler works for a single commit or for a range of commits; also, should we ever support non-contiguous selections in the future, it will just work for them too.
I see no reason why we need to disallow reverting merge commits. I would put up the same prompt as we now do for a single commit when it is a merge, asking for which parent you want to use. If there are several merge commits contained in the range, we'd use the same parent choice for all of them, which I think is fine. (Command-line git doesn't let you specify it more fine-grained either.)

jesseduffield commented 7 months ago

Sorry for the late reply: I agree with @stefanhaller .

With regards to visibility, I'm fine for the keybinding to not be on the bottom bar regardless of whether multiple commits are selected: single-commit revert is a common use case, but not common enough to be included in the already over-crowded keybinding suggestions, and multi-commit revert is not common enough to warrant inclusion.

In general your branch isn't doing anything crazy so if you implement @stefanhaller 's feedback it'll be in good shape.

uberrice commented 6 months ago

Thanks for the replies. I'll apply your suggestions and get everything cleaned up when I get the time and then submit a pull request (to be blocked til #1807 is dealt with, of course - makes sense)

uberrice commented 5 months ago

Updated my branch with the following:

Removed visibility of 'revert' when range selected
Changed implementation to pass all individual SHAs rather than use git CLI's .. operator

I'm currently working on the merge commit menu for range reverts with a merge commit - I'll have to rework createRevertMergeCommit a bit for that (or overload the function to accept a range of commits?), and that wasn't immediately obvious to me. When that's done, the feature should be good to go.

-> I could also change my implementation that it would call git revert for every single reverted commit - this would remove complexity (no RevertRange function to call) at the cost of potentially doing a lot of git operations. Would that maybe be preferred?

Other option: when reverting a range with multiple merge commits, revert 'range' up to merge commit, revert that one individually with the menu, and so on. That would add granularity, and you could choose which parent to use on every merge commit included in the range.

stefanhaller commented 5 months ago

I'll have to rework createRevertMergeCommit a bit for that

Ah, this is a bit trickier than I thought. I didn't realize that the menu actually shows the commit subjects of the merge commits's parents to choose from. My suggestion would be to keep this behavior only if a single merge commit is selected; if a range of commits is selected, and it contains one or more merge commits, put up a similar menu but with generic entries like "first parent" and "second parent". (Technically it's possible to have merge commits with more than two parents, but I've never seen them in real life, so I wouldn't worry about that case.)

-> I could also change my implementation that it would call git revert for every single reverted commit

No, definitely not. It would be way too slow, and it would also make it very difficult to handle the case where one commit in the middle has conflicts.

Other option: when reverting a range with multiple merge commits, revert 'range' up to merge commit, revert that one individually with the menu, and so on.

This would have the same problem as the one above (what if there are conflicts), but it's also unnecessary complexity. As I said earlier, command-line git doesn't let you specify the merge parents separately for each merge commit either. If someone really has the need to pick a different parent for two merge commits, they can just do the revert in multiple steps, starting with the newest one.

jesseduffield / lazygit

Support range select for reverting commits #3272