Add merge menu with conflict resolver

[lmnek]

Implements #2026. I also tried to address issues mentioned in the #3477 PR.

Previously, pressing M opened an external merge tool. Now it opens a merge options menu that allows selecting all conflicts in chosen files as ours (HEAD), theirs (incoming), or union (both), while still providing access to the external merge tool.

This uses git-merge-file for a 3-way merge with the --ours, --theirs, and --union flags. This approach avoids the issue mentioned in #1608 (reply in thread), and correctly applies the chosen conflict resolutions while preserving changes from other branches. The command is executed with --object-id, which requires object IDs obtained via rev-parse, instead of relying on the standard version that works with full saved files.

Disclaimer: On my machine, the tests pass inconsistently. Sometimes they succeed, sometimes fail with errors such as POTENTIAL DEADLOCK: Recursive locking or Inconsistent locking. I haven’t yet identified the root cause of this issue.

PR Description

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

[stefanhaller]

O wow, this looks really nice at first glance. It might take me a while to review this, as I'm rather busy right now. Feel free to ping me if you haven't heard back after a week or two.

Disclaimer: On my machine, the tests pass inconsistently. Sometimes they succeed, sometimes fail with errors such as POTENTIAL DEADLOCK: Recursive locking or Inconsistent locking.

My guess is that you have go 1.25 installed locally. This is a known problem, I didn't have time to look into this yet. If you can use 1.24 to run the tests, it should be more reliable.

[bllendev]

yoo thanks for doing this i just ran into this prob, had a big rebase and i was really thinking this should exist lol, thanks again

[stefanhaller]

This is really great work. Don't let the large number of review comments discourage you, many of them are only nitpicks.

[lmnek]

Thank you for the feedback! I addressed all of the comments and responded when I wasn't 100% sure about my solution.

[stefanhaller]

Excellent work! All my complaints were addressed to my satisfaction. 😄

I pushed three more fixup commits, the first two for very minor things, the third is a bit more important though: it makes it so that the command to open the menu is disabled when there are no files in the selection that have merge conflicts. This is important because otherwise the command would always show up in the status bar (even when you don't have any conflicts), which is confusing. Feel free to squash all fixups if you are happy with them.

However, while working on this I noticed a problem that I didn't realize before: using normalisedSelectedNodes is wrong here. What this does is that when you have a multiselection of a directory and a file inside, it removes the file from the selection, leaving only the directory. (Except that the function is buggy right now, see #4920.) But this is not useful, because the new merge conflicts menu can't deal with directories. What we need is the opposite: when a directory is selected, it should remove the directory from the selection and instead add all files inside of it (no matter whether they are selected or not).

I guess I would also be content with a simpler version that requires you to select the files to operate on; in that case I would propose to disable the command unless all selected items are files with merge conflicts.

Let me know if you have any thoughts on this.

[lmnek]

I've squashed all previous commits and fixed the linter warning.

Thank you for pointing out the incorrect behavior of the node selection. I would prefer this to be solved completely rather than the simple restricting version. Therefore, I added one more fixup commit with a scratch implementation of a BFS algorithm that's somewhat a reverse of normalisedSelectedNodes. Let me know what you think 😄.

I also just noticed that the CI tests are failing on older Git versions, since the --object-id flag was only introduced in Git v2.43. I’m not sure what the preferred way to handle version compatibility is in this codebase, so I’d appreciate your input.

One possible fallback (without --object-id) would be to use git merge-file with three temporary files, one for each of the previously referenced objects; similar to this script: https://github.com/jakub-g/git-resolve-conflict/blob/base/lib/git-resolve-conflict.sh. That was actually my initial implementation, but I replaced it with the object ID approach since it seemed more elegant and lightweight.

[stefanhaller]

Therefore, I added one more fixup commit with a scratch implementation of a BFS algorithm that's somewhat a reverse of normalisedSelectedNodes.

This is very nice, I like the name too. I just wonder if it needs to be breadth first; wouldn't depth first be sufficient, and be easier? (No need for a queue.) I'm not much of an algorithm guru, so maybe I'm missing something.

I also just noticed that the CI tests are failing on older Git versions, since the --object-id flag was only introduced in Git v2.43. I’m not sure what the preferred way to handle version compatibility is in this codebase, so I’d appreciate your input.

Ah damn. Normally I'd be tempted to say "disable the feature on older versions", but 2.43 isn't that old yet (Apple still ships 2.39 with their devtools it seems), so this might be annoying for too many people. But I'm also not excited about maintaining an alternative code path (especially when it's more complex) for older versions.

So: I'm unsure too. Another option could be to do the simpler "checkout --theirs"/"checkout --ours", but the behavior would be slightly different; not sure that's good.

That was actually my initial implementation,

Ok, if you already had that, then maybe you are the best one to judge how bad it would be to maintain the extra code path?

[lmnek]

I just wonder if it needs to be breadth first; wouldn't depth first be sufficient, and be easier? (No need for a queue.)

Switching to DFS would require either changing the queue to a stack or rewriting the algorithm recursively. I don’t see a clear advantage in doing that as the current BFS implementation seems equally readable and comparable in terms of memory usage and performance.

But I'm also not excited about maintaining an alternative code path (especially when it's more complex) for older versions.

I implemented both code paths in the latest fixup commit. This included adding a git show command, a helper function to create temporary files for the older merge-file approach, and splitting behavior based on the detected Git version. The legacy path doesn’t feel particularly complex, so I don’t expect it to be difficult to maintain. Let me know what you think! :))

[stefanhaller]

Switching to DFS would require either changing the queue to a stack or rewriting the algorithm recursively.

Yeah, I would have expected a recursive implementation, but I'm also happy to keep the current one. You are right, it is straightforward enough.

I implemented both code paths in the latest fixup commit. This included adding a git show command, a helper function to create temporary files for the older merge-file approach, and splitting behavior based on the detected Git version. The legacy path doesn’t feel particularly complex, so I don’t expect it to be difficult to maintain. Let me know what you think! :))

Happy to keep this! You are right, it's not so much code, and the integration tests cover both implementations.

I pushed a few more fixups, let me know if you have questions about any of those.

I'm happy with the current state, and would be ready to merge it as is.

I did notice one cosmetic problem where after invoking the menu you get the "All conflicts resolved, continue the merge?" popup, but behind it you still see the conflict markers. I find this rather ugly, but it's not a new problem, so I decided to open a separate PR to address it. If you want to have a look: #4945.

[lmnek]

Thank you for the review and all the helpful comments throughout! I'm quite glad that I chose this issue as my first open-source contribution, as it was a very smooth and fun experience. I hope to contribute some more in the future :))

(I have no problem with any of the latest changes, so I went ahead and squashed all the commits.)

[stefanhaller]

Excellent. Looking forward to your next contribution!