Add support for external merge tool #18
Comments
I initially made the working copy materialize conflicts in its `check_out()` method. Then I changed it later (exactly a year ago, on Halloween of 2020, actually) so that the working copy expected conflicts to already have been materalized, which happens in `MutableRepo::check_out`(). I think my reasoning then was that the file system cannot represent a conflict. While it's true that the file system itself doesn't have information to know whether a file represents a conflict, we can record that ourselves. We already record whether a file is executable or not and then preserve that if we're on a file system that isn't able to record it. It's not that different to do the same for conflicts if we're on a file system that doesn't understand conflicts (i.e. all file systems). The plan is to have the working copy remember whether a file represents a conflict. When we check if it has changed, we parse the file, including conflict markers, and recreate the conflict from it. We should be able to do that losslessly (and we should adjust formats to make it possible if we find cases where it's not). Having the working copy preserve conflict states has several advantages: * Because conflicts are not materialized in the working copy, you can rebase the conflicted commit and the working copy without causing more conflicts (that's currently a UX bug I run into every now and then). * If you don't change anything in the working copy, it will be unchanged compared to its parent, which means we'll automatically abandon it if you update away from it. * The user can choose to resolve only some of the conflicts in a file and squash those in, and it'll work they way you'd hope. * It should make it easier to implement support for external merge tools (#18) without having them treat the working copy differently. This patch prepares for that work by adding support for parsing materialized conflicts.
I initially made the working copy materialize conflicts in its `check_out()` method. Then I changed it later (exactly a year ago, on Halloween of 2020, actually) so that the working copy expected conflicts to already have been materalized, which happens in `MutableRepo::check_out`(). I think my reasoning then was that the file system cannot represent a conflict. While it's true that the file system itself doesn't have information to know whether a file represents a conflict, we can record that ourselves. We already record whether a file is executable or not and then preserve that if we're on a file system that isn't able to record it. It's not that different to do the same for conflicts if we're on a file system that doesn't understand conflicts (i.e. all file systems). The plan is to have the working copy remember whether a file represents a conflict. When we check if it has changed, we parse the file, including conflict markers, and recreate the conflict from it. We should be able to do that losslessly (and we should adjust formats to make it possible if we find cases where it's not). Having the working copy preserve conflict states has several advantages: * Because conflicts are not materialized in the working copy, you can rebase the conflicted commit and the working copy without causing more conflicts (that's currently a UX bug I run into every now and then). * If you don't change anything in the working copy, it will be unchanged compared to its parent, which means we'll automatically abandon it if you update away from it. * The user can choose to resolve only some of the conflicts in a file and squash those in, and it'll work they way you'd hope. * It should make it easier to implement support for external merge tools (#18) without having them treat the working copy differently. This patch prepares for that work by adding support for parsing materialized conflicts.
I initially made the working copy materialize conflicts in its `check_out()` method. Then I changed it later (exactly a year ago, on Halloween of 2020, actually) so that the working copy expected conflicts to already have been materalized, which happens in `MutableRepo::check_out`(). I think my reasoning then was that the file system cannot represent a conflict. While it's true that the file system itself doesn't have information to know whether a file represents a conflict, we can record that ourselves. We already record whether a file is executable or not and then preserve that if we're on a file system that isn't able to record it. It's not that different to do the same for conflicts if we're on a file system that doesn't understand conflicts (i.e. all file systems). The plan is to have the working copy remember whether a file represents a conflict. When we check if it has changed, we parse the file, including conflict markers, and recreate the conflict from it. We should be able to do that losslessly (and we should adjust formats to make it possible if we find cases where it's not). Having the working copy preserve conflict states has several advantages: * Because conflicts are not materialized in the working copy, you can rebase the conflicted commit and the working copy without causing more conflicts (that's currently a UX bug I run into every now and then). * If you don't change anything in the working copy, it will be unchanged compared to its parent, which means we'll automatically abandon it if you update away from it. * The user can choose to resolve only some of the conflicts in a file and squash those in, and it'll work they way you'd hope. * It should make it easier to implement support for external merge tools (#18) without having them treat the working copy differently. This patch prepares for that work by adding support for parsing materialized conflicts.
|
I don't know if this is related, but setting the |
|
I'd like to add basic mergetool support. I think the best that can be done is to use a mergetool for simple conflicts, merge-tools.meld.merge-args = ["$left", "$base", "$right", "-o", "$output_empty"]
merge-tools.vimdiff.merge-args = ["-f", "-d", "$output_workspace", "-M", "$left", "$base", "$right",
"-c", "wincmd J", "-c", "set modifiable", "-c", "set write"]
How does this sound? |
|
Sounds good! Thank you!
Yep, that's definitely good enough to start with. Once that's done, it's probably relatively simple to add support for the the more complex cases by doing one 3-way merge at a time. For example, if the conflict is For non-file states, we'll probably just want ask the user to choose interactively with a prompt. Feel free to leave that, too, for later.
That matches what git calls it. I like hg's choice better:
That configuration matches what @yuja suggested in f6acee4, so good job either digging that up, or extrapolating from current config options :)
Git and hg seem to have been okay with having a single
Here's what git does: https://git-scm.com/docs/git-mergetool#Documentation/git-mergetool.txt-mergetoollttoolgttrustExitCode FYI, a related feature is "partial merge tools", which I added support for to hg in https://www.mercurial-scm.org/repo/hg/rev/f3aafd785e65. It's good to keep that feature in mind, but I don't think it would impact anything. They're invoked quite differently (as a series of filters, not just a single tool), and the same tool is unlikely to work both as a regular merge tool and a partial tool (unlike how difftools and mergetools are often the same binary), which means we probably don't want to share even the configuration. |
|
I now have a working but very unfinished version of this at https://github.com/ilyagr/jj/tree/mergetool. For now, you have to recompile to use a different mergetool than Vim. Also, it doesn't function correctly if the
That's certainly an option. Another option is to guide the user to the appropriate revisions to solve a conflict -- take them to a revision where the conflict is simple enough, or help them split an octopus merge into a sequence of merges. (This is more difficult for rebases, but perhaps possible with the obslog). Anyway, I'm not planning to address this at the moment.
I agree,
:). yes, I extrapolated this from the docs for the current configuration. I haven't looked carefully at the current implementation yet, though.
Yes, let's start with this and see if we ever need to make it more complicated.
For now, the merge fails if there is an exit code, if the output file is unchanged, or if it is empty. If we need more logic, we can add it later.
I haven't looked into this in detail, but it looks like, at least for merge tools that let you edit the output file directly, it should be quite simple to add to One question that bothers me is what to do if the conflicts cannot be parsed, not to lose the work the user put in. My best idea is to create an |
|
Regarding partial merge tools: I'm not sure, but it sounds like you misunderstood what they're about. They're meant to be completely automatic tools for resolving conflicts, even if they can't resolve all conflicts. The builtin resolution of trivial conflicts (where one side is unchanged or both sides changed the same way) can be viewed as a first step in a chain of partial merge tools.
I think this comment is unrelated to partial merge tools and related to partial conflict resolutions done by the user. Are you thinking of Let's say the conflict looked like this (when rendered as conflict markers): Maybe the user removed the By the way, should we use the usual 3-way marker style when we pre-populate the output file? I don't know what |
|
Indeed, I was talking solely about partial conflict resolution. The initial version of my code did exactly what you suggest: it took whatever the merge tool output was, and put it into the repo as a regular file. It was not very pleasant -- jj has trouble with such files. For instance, I now have two approaches: abort if there seem to be any conflicts in the file or use existing functions to parse the file as a conflict. I haven't had time to write any tests or figure out what the latter versions would even do in your example.
This could be an option, if we find any tools that benefit from that. It would be very nice to support conflict resolution in VS Code, but I'm not sure if it is possible to use VS Code as a mergetool without writing a VS Code extension. It would certainly be possible for repos with |
Ah, you mean if the file gets replaced by a regular file with conflict markers in? FYI, what I agree that that seems confusing. One option is to render conflicts as empty strings in the pre-populated output file. I just tried FYI, another option for making the That makes logical sense, but I think the diff against rendered conflict markers are generally easier to read. We could still add support for this "combined diff" as an option. If we do, we could hint about it when it looks like there are still conflict markers in the resolved side of diff with a conflict.
That would mean that you cannot use the merge tool to resolve a conflict if the result is actually supposed to have conflict markers in it (like some of our test files).
I think we should trust the merge tool instead. Hopefully the idea about render conflict hunks as empty strings will work. |
|
Oh, another option for diffs of conflicts would be to show a header near each resolved conflict, maybe right after where we currently show |
I find it very helpful simply to see the three versions of the file in three separate panes. The difftool functionality I use in
I don't like this idea. If I had to choose, I like the idea of empty strings better than using the base file (which I can get from the base file window anyway). I think A more minor reason to dislike these two options is that I like the possibility of partial conflict resolution. I haven't yet had a conflict complicated enough to make that necessary, but it seems likely to happen eventually. None of the graphical (AKA non-primitive) merge tools I tried so far seem to care about what's in the output file initially.
I think that the simplest behavior that makes sense is to default to starting with an empty file for For now, #689 only implements the latter behavior. For me, at least, it seems to work well. However, if that continues to be the default behavior, there is potential for some merge tool I don't know or somebody's custom script to get confused.
That's right, and the diff you described is what I saw.
I don't understand the various diff formats git uses for conflicts well enough to comment about that. I would not be surprised if they are very useful in some cases.
I like this idea. Currently, I resolve a conflict in a merge commit (for example), I find the diff However, there are some difficulties. We run into the problem of what to do when a file or a conflict includes text that looks like a header. I guess we don't absolutely have to require the normal diff output to be able to represent everything. We could instead have an option to output Git-style conflict resolution diffs as you discussed. I assume those are designed to handle all cases. Aside: I also find it weirdly difficult to see what the conflict is. This is the only case in which I need to switch to the commit and use |
|
BTW, I tried out the case of the user messing up the conflict markers you mentioned in #18 (comment). Arguably, that works out OK -- the messed up conflict is considered "resolved" text, and no information is lost. I found one problem: if the messed up conflict in the only conflict in the file, the entire file is considered resolved. I think this can only be undone with |
Sorry, I understand that
Makes sense.
We may want a per-tool option for that, but that can come later.
Yes. Now that I understand better what
FWIW, Git has only very recently gained an option to show a diff very similar to ours.
I doubt I'll prioritize implementing that format soon, but it's probably not very hard.
Since the diff contents in our
Good point, we may want to split it up like that.
You shouldn't need |
|
I'll close this one. Some of the TODOs in the code may or may not become new bugs. |
No description provided.
The text was updated successfully, but these errors were encountered: