Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automatic staging of files during a merge conflict / interactive rebase #3111

Closed
randoragon opened this issue Nov 8, 2023 · 16 comments · Fixed by #3870
Closed

Automatic staging of files during a merge conflict / interactive rebase #3111

randoragon opened this issue Nov 8, 2023 · 16 comments · Fixed by #3870
Labels
bug Something isn't working

Comments

@randoragon
Copy link

Technically this is not a bug, but an inconvenient behavior that should be possible to disable, in my opinion.

During a merge or interactive rebase, when conflicts are encountered, lazygit attempts to resolve them automatically by staging all files, and, if it's possible to proceed, prompts the user with "All merge conflicts resolved. Continue?". I find this behavior highly disruptive, because there are plenty of scenarios in which resolving a merge conflict is not as simple as git add ., even if there are no conflict markers present in files.
In general, this makes me not want to rely on lazygit during some of these merges, because I'm afraid of it ruining my staging area while I want to carefully review the unstaged changes and stage the relevant bits little by little manually.

To Reproduce

  1. Cause a merge conflict that can be resolved with git add ..
  2. Open lazygit.
  3. Within a few seconds, lazygit stages all files and prompts the user with "All merge conflicts resolved. Continue?".

Expected behavior

Lazygit should not stage any files and should not prompt the user in any way, giving them manual control over the rebase.
I would like to see a config option to turn off this behavior entirely.

Version info:
commit=v0.40.2, build date=2023-08-12T17:47:33Z, build source=binaryRelease, version=0.40.2, os=linux, arch=amd64, git version=2.42.1
git version 2.42.1

Additional context
I run into this problem the most frequently when I'm working on projects with git submodules. What tends to happen:

  1. I have 2 matching feature branches in both the parent repo and its submodule. They are being developed in parallel, i.e. I may have commits [A, B, C] on the submodule feature branch, and corresponding commits [A, B, C] on the parent repo feature branch, which bump the submodule.
  2. I want to rebase both repos' feature branches on top of their masters. The way to do this in git is to first rebase the submodule branch, and then rebase the parent repo branch. But during the parent repo rebase, it is necessary to stop at each of the A, B and C commits, go to the submodule, check out the correct commit, go back to the main repo, git add, and only then the conflict is properly resolved. git add . is a legal way to resolve the conflict, but it leads to "squashing" all [A, B, C] commits of the submodule repo into a single commit on the main repo, which is undesired.
@randoragon randoragon added the bug Something isn't working label Nov 8, 2023
@stefanhaller
Copy link
Collaborator

stefanhaller commented Nov 8, 2023

[side note/rant]: Submodules suck. They may be ok for pulling in third-party dependencies that only change every so often, but for stuff that people are actively developing, they are a PITA. I'm so glad that we switched to a monorepo at work.

On to the real issue: yes, I can see how lazygit's behavior can be annoying, and I agree it would be good to have an option to disable it. Personally I happen to like it, most of the time, so I would keep it enabled. In fact, I often have the opposite problem, which is that lazygit doesn't stage changes aggressively enough. The use case is that you fix the conflicts in your editor (I rarely use lazygit's merge editor for that); once you saved the last one, lazygit stages things and brings up the "Continue?" prompt, but in the background where you don't see it. Meanwhile, I find that things don't compile yet, so I continue to fix some compiler errors; now, when I return to lazygit, answering "yes" to the continue prompt doesn't work because not all files are staged.

@randoragon
Copy link
Author

Personally I happen to like it, most of the time, so I would keep it enabled. In fact, I often have the opposite problem, which is that lazygit doesn't stage changes aggressively enough.

That's a good counterexample, I have that happen to me sometimes as well. Part of the reason I dislike the "Continue?" prompt is because it's not always clear what has been staged and what hasn't. Generally in such cases I'd rather be forced to double-check than trust a tool that only does what I want 99% of the time (especially given how annoying correcting a rebase/merge is in git).

I totally agree that the current behavior should be opt-out, not opt-in. New users might get surprised by it, but find it useful, or disable it if they don't. On the contrary, if it was opt-out, it could go over many potential users's heads, since it's not an obvious feature.

@BrdgYin
Copy link

BrdgYin commented Nov 9, 2023

I couldn't agree more

@turion
Copy link

turion commented Apr 22, 2024

I've run into this problem often enough now when doing lazygit interactive rebase + VSCodium merge conflict editor. I'm not using submodules at all, so the problem exists without them. Also, it occurs when I'm not using lazygits merge editor. It just randomly happens in the background while I'm trying to resolve conflicts in VSCodium. I had to abort some big rebases because I couldn't salvage my worktree anymore. An option to disable this behaviour would be greatly appreciated! (IMHO it should be the default, then maybe add a tooltip to raise awareness for the option)

@samaursa
Copy link

This caught me off guard where it was happening in the background and I did not know what happened to the files in conflict. With Git LFS enabled, merge conflict resolution by Lazygit is plain wrong with this behavior. Unreal is capable of handling binary merge conflicts of its assets, but with Lazygit running, I cannot resolve such conflicts.

I would prefer this behaviour to be OFF by default, but what matters more is that it is driven by a config.

@stefanhaller
Copy link
Collaborator

Here's a PR: #3870.

Could you please test it and see if this addresses the problem for you?

@jesseduffield
Copy link
Owner

Meanwhile, I find that things don't compile yet, so I continue to fix some compiler errors; now, when I return to lazygit, answering "yes" to the continue prompt doesn't work because not all files are staged.

For the record this annoys me too: I'd like it to prompt me to stage all and continue

@samaursa
Copy link

samaursa commented Aug 31, 2024

@stefanhaller I'd be happy to test it, however I have been relying on the binary builds. Are there a set of binary builds of the PR? If not, I'll try to build/run it from repo following the contribution guide.

@stefanhaller
Copy link
Collaborator

Meanwhile, I find that things don't compile yet, so I continue to fix some compiler errors; now, when I return to lazygit, answering "yes" to the continue prompt doesn't work because not all files are staged.

For the record this annoys me too: I'd like it to prompt me to stage all and continue

Are you thinking about changing the confirmation to a different one while it is showing if it notices that there are unstaged changes now? I'd have to look into how we might do this, but I'm also worried that the change might be too easy to miss. Or do you mean an additional prompt after you confirmed the first one? That would be much easier.

@stefanhaller
Copy link
Collaborator

@stefanhaller I'd be happy to test it, however I have been relying on the binary builds. Are there a set of binary builds of the PR? If not, I'll try to build/run it from repo following the contribution guide.

We don't have builds for PRs, sorry. But lazygit is very easy to build from source, all you need is install go for your platform. It has no other dependencies.

I'm very interested in your feedback on this before I merge. The other people who requested this too! @randoragon @turion @BrdgYin

@stefanhaller
Copy link
Collaborator

Are you thinking about changing the confirmation to a different one while it is showing if it notices that there are unstaged changes now? I'd have to look into how we might do this, but I'm also worried that the change might be too easy to miss. Or do you mean an additional prompt after you confirmed the first one? That would be much easier.

Assuming you mean the latter, I opened a PR to do that: #3879

@randoragon
Copy link
Author

randoragon commented Aug 31, 2024

I'm very interested in your feedback on this before I merge. The other people who requested this too! @randoragon @turion @BrdgYin

Thanks for your hard work, I should be able to check today or tomorrow, hope that's okay.

@randoragon
Copy link
Author

I cloned and built the 93f2961 commit, placed

git:
  autoStageResolvedConflicts: false

in ~/.config/lazygit/config.ymland ran a very simple experiment on a fresh git repo:

  1. Init commit a file with any contents.
  2. Create a feature branch.
  3. Commit a modification to the file on the feature branch, and a different modification on the main branch.
  4. Merge/rebase one branch onto the other (tried both) to cause a conflict.
  5. Open lazygit, edit the conflicting file with an external editor and get rid of the conflict markers.
  6. After the above step, upstream lazygit auto-stages the file and asks "All merge conflicts resolved. Continue?". PR lazygit does not do that, the edited file remains unstaged and no prompts show up.

Seems good, this is exactly the behavior I was hoping for @stefanhaller!

@stefanhaller
Copy link
Collaborator

@randoragon Thanks for testing. I'll consider this enough confirmation, and am going to merge.

stefanhaller added a commit that referenced this issue Sep 2, 2024
- **PR Description**

Add user config `git.autoStageResolvedConflicts` (default true). When
set to false, users need to stage their conflicted files manually after
resolving conflicts, and also continue a merge/rebase manually when all
conflicted files are resolved.

Fixes #3111.
stefanhaller added a commit that referenced this issue Sep 2, 2024
…ving conflicts (#3879)

- **PR Description**

When lazygit sees that all conflicts were resolved, it auto-stages all
previously conflicted files and asks to continue the rebase. (There is
[a PR](#3870) open to make
this optional.)

It is a common situation for this popup to be opened in the background,
while the user is still busy fixing build errors in their editor. In
this case, coming back to lazygit and confirming the continue prompt
would result in an error because not all files are staged.

Improve this by opening another popup in this case, asking to stage the
newly modified files too and continue.

See
#3111 (comment)
and the following discussion further down in that issue.
@samaursa
Copy link

samaursa commented Sep 3, 2024

Was able to confirm that the above fix works as intended. LFS file conflicts are no longer auto-merged.

@stefanhaller
Copy link
Collaborator

@samaursa Thanks for testing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants