Add merge menu with conflict resolver #4889
Conversation
|
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.
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. |
|
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 |
Replace merge-tool with merge options menu that allows resolving all conflicts for selected files as ours, theirs, or union, while still providing access to the merge tool.
| fetch: f | ||
| toggleTreeView: '`' | ||
| openMergeTool: M | ||
| openMergeOptions: M |
There was a problem hiding this comment.
This is a breaking change (users might have changed the keybinding in their config file). We have the technology to migrate users's config files to account for this, and we should do that by adding an entry to this array.
| Description: self.c.Tr.OpenMergeTool, | ||
| Tooltip: self.c.Tr.OpenMergeToolTooltip, | ||
| Key: opts.GetKey(opts.Config.Files.OpenMergeOptions), | ||
| Handler: self.withItems(self.createMergeConflictMenu), |
There was a problem hiding this comment.
Instead of implementing the menu here, we should implement it in WorkingTreeHelper.OpenMergeTool (and rename it accordingly). That way, it will also be used when you are in the merge conflict editor.
Edit: hm, when you are in the merge conflict editor you probably expect to work only on the current file you are in. So we'd have to pass a list of file paths to that function, and pass the selected ones from here, and the current one from merge_conflicts_controller.
| fileNodes := lo.Filter(normalizedNodes, func(node *filetree.FileNode, _ int) bool { | ||
| return node.File != nil | ||
| }) | ||
| filenames := lo.Map(fileNodes, func(node *filetree.FileNode, _ int) string { |
There was a problem hiding this comment.
Nitpick: would call the variable filepaths.
| cmdArgs := NewGitCmd("merge-file"). | ||
| Arg("--object-id"). | ||
| Arg(strategy). | ||
| Arg("-p", oursID, baseID, theirsID). |
There was a problem hiding this comment.
Nitpicks: would use --stdout instead of -p for better readability, and put it on a line by itself; otherwise it looks like it is an option that takes the object ids as arguments.
| Arg("-p", oursID, baseID, theirsID). | |
| Arg("--stdout"). | |
| Arg(oursID, baseID, theirsID). |
|
|
||
| func (self *FilesController) createMergeConflictMenu(nodes []*filetree.FileNode) error { | ||
| onMergeStrategySelected := func(strategy string) error { | ||
| normalizedNodes := normalisedSelectedNodes(nodes) |
There was a problem hiding this comment.
This relies on the assumption that all currently shown files have conflicts. This doesn't have to be true; while we automatically filter for only conflicted files when there are any, the user can open the filter menu (ctrl-b) and choose "no filter" to show all files.
Which means we should probably filter on the status again here.
Also, for files that have non-textual conflicts (see #4431), I guess the git rev-parse commands will probably error? So we should filter those out, too.
| ViewMergeConflictOptions: "View merge conflict options", | ||
| ViewMergeConflictOptionsTooltip: "View options for resolving merge conflicts.", | ||
| MergeConflictOptionsTitle: "Resolve merge conflicts", | ||
| UseHead: "Use HEAD", |
There was a problem hiding this comment.
For non-textual conflicts we settled on the terms "current changes" (for head) and "incoming changes"; see #4431, so I'd suggest to use those here, too.
Also, we don't use Title Case in menu items, so it should be "Use current changes" and "Use both".
| ` | ||
|
|
||
| var MergeFileTheirs = NewIntegrationTest(NewIntegrationTestArgs{ | ||
| Description: "Conflicting file can be resolved to 'their' version via merge-file", |
There was a problem hiding this comment.
Isn't "ours" the more interesting case? I'd prefer a test that has both conflicts and non-conflicting changes, and tests that when you choose "ours" you get ours for the conflicts, but theirs for the non-conflicts. I pushed a commit that does this (aeeab45), also using test data that I find a bit easier to grok.
Actually, it might be worth testing all three cases; I find them all interesting.
stefanhaller
force-pushed
the
merge-file-options
branch
from
b6c68fc to
aeeab45
Compare
Implements #2026. I also tried to address issues mentioned in the #3477 PR.
Previously, pressing
Mopened 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--unionflags. 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 viarev-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 lockingorInconsistent locking. I haven’t yet identified the root cause of this issue.PR Description
Please check if the PR fulfills these requirements
go generate ./...)