Closed martinvonz closed 1 year ago
I'd like to add basic mergetool support. I think the best that can be done is to use a mergetool for simple conflicts, A + B - C
in jj notation. The command could be jj mergetool FILENAME
. It would be configured as follows in config.toml
:
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"]
jj
will replace arguments that start with $
as appropriate. Exactly one of $output_empty
or $output_workspace
is required. The first of these will ignore the contents of FILENAME
in the current commit, and consider the merge successful if anything is written to the file (I think this is the best meld
can do). The second will put the current (partially merged) contents of FILENAME
at $output_workspace
and will consider the merge successful if the file is modified. If there are conflict markers remaining in FILENAME
, that is fine as long as they can be parsed.
How does this sound?
Sounds good! Thank you!
I think the best that can be done is to use a mergetool for simple conflicts,
A + B - C
in jj notation.
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 A + B + C - D - E
, we can call the external tool with e.g. $base=D, $left=A, $right=B
. If that gets resolved successfully, we replace those three terms with the resolved state.
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.
The command could be
jj mergetool FILENAME
That matches what git calls it. I like hg's choice better: resolve
. What do you think about that?
It would be configured as follows
That configuration matches what @yuja suggested in https://github.com/martinvonz/jj/commit/f6acee41dfa209262b6506fe52c87ee3de7b0777, so good job either digging that up, or extrapolating from current config options :)
Exactly one of
$output_empty
or$output_workspace
is required.
Git and hg seem to have been okay with having a single $output
variable. I just tried it with meld and it seems to give you the option to overwrite the output file or leave it unchanged, so it seems it would be fine to pass the workspace file to meld?
consider the merge successful if anything is written to the file
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 output
file has any conflict markers in it after the mergetool.
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.
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 like hg's choice better: resolve. What do you think about that?
I agree, resolve
is perfect for this.
good job either digging that up, or extrapolating from current config options :)
:). yes, I extrapolated this from the docs for the current configuration. I haven't looked carefully at the current implementation yet, though.
Git and hg seem to have been okay with having a single $output variable.
Yes, let's start with this and see if we ever need to make it more complicated.
consider the merge successful if anything is written to the file.
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.
a related feature is "partial merge tools",...
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 jj
. The output file starts with jj
's conflict markers, and after the edit, it can read whatever markers are left. I guess I'm biased since I'm used to vim, which does this, but I'm not sure how many popular merge tools work this way. I'm pretty sure meld
doesn't and I am not sure about kdiff3
. I can also try Beyond Compare
, since their free trial is so generous.
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 original_filename.FAILED_RESOLVE_DELETE_ME
file in the revision next to the original file. There's some chance the user will accidentally push it, but it's mitigated by the fact that jj
will refuse to push it until the conflict is fixed. Ideally, the following jj resolve
command will notice the file, and ask the user whether to delete it or to use it.
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.
One question that bothers me is what to do if the conflicts cannot be parsed, not to lose the work the user put in.
I think this comment is unrelated to partial merge tools and related to partial conflict resolutions done by the user. Are you thinking of vimdiff
-like tools that expect an output file that has been pre-populated with conflict markers?
Let's say the conflict looked like this (when rendered as conflict markers):
line 1
line 2
<<<<<<<
+++++++
line 3a
%%%%%%%
-line 3
+line 3b
>>>>>>>
line 4
line 5
Maybe the user removed the <<<<<<<
and left the file like that. If the user made their merge tool exit with status 0, I think we should just trust that and not try to parse any conflict markers. We could print a warning if we find any <<<<<<<
, %%%%%%%
, etc., however.
By the way, should we use the usual 3-way marker style when we pre-populate the output file? I don't know what vimdiff
and similar tools use the pre-populated contents for. If they interpret them in some way, they're surely more likely to be able to interpret the usual 3-way markers.
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, jj diff
had trouble displaying anything reasonable for them. It should be easy to revert my code to this behavior, but it seems barely usable. It would make more sense if the output
file started out empty, which seems to not make any difference to tools other than vimdiff
, but I like being able to resolve conflicts in a terminal...
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.
By the way, should we use the usual 3-way marker style when we pre-populate the output file?
This could be an option, if we find any tools that benefit from that. vimdiff
certainly works with jj's style, though I haven't checked whether it works better with git's style. meld
or kdiff3
ignore the output file contents entirely.
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 --git-dir .
if jj
simulated git's merge states, but that's a whole different problem. I only know that merge states use git's staging area in some bizarre fashion.
It was not very pleasant -- jj has trouble with such files. For instance,
jj diff
had trouble displaying anything reasonable for them.
Ah, you mean if the file gets replaced by a regular file with conflict markers in? FYI, what jj diff
does when one side is a conflict is to render it as conflict markers, so if you show a diff where the left side was a conflict and the right side is a regular file with the same conflict markers (because the user accidentally marked it as resolved), then it's going to be an empty diff, except for the type transition, so it's going to look like this:
Resolved conflict in some/path:
...
I agree that that seems confusing. One option is to render conflicts as empty strings in the pre-populated output file. I just tried vimdiff
to get a better understanding for how it works. It seems it doesn't actually help you merge at all. So I guess that's why it's called vimdiff
and not vimmerge
:) For primitive tools like that, empty strings does seem to make sense (and actual merge tools probably don't care about the initial contents, so they're also fine with it). What do you think? Another option is to simply use the base file as initial output.
FYI, another option for making the jj diff
output less confusing in these cases would be to use a format closer to Git's "combined diff" format (git diff -c/--cc
). Something like this for conflict B + C - A
resolved as D
, i.e. diff D - (B + C - A) = D - B - C + A
:
- b
+ d
-c
+a
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.
abort if there seem to be any conflicts in the file
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).
use existing functions to parse the file as a conflict
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 ...
for skipped context lines. Then incorrectly resolved hunks would appear as removing a conflict marker and adding the same (or similar) conflict marker.
One option is to render conflicts as empty strings in the pre-populated output file. I just tried vimdiff to get a better understanding for how it works. It seems it doesn't actually help you merge at all. .....
I find it very helpful simply to see the three versions of the file in three separate panes. The difftool functionality I use in vimdiff
is the fact that all the panes scroll simultaneously, the folding, and the ]c
and [c
commands to navigate between changes. It is primitive, but I often find it much nicer than editing the conflict markers directly, and these are the only two options I know for conflict resolution in the terminal.
For primitive tools like that, empty strings does seem to make sense (....). What do you think? Another option is to simply use the base file as initial output.
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 vimdiff
would still be hard to use with that. It would make more sense if vimdiff
was a more feature-full merge tool and highlighted the differences between the output window and the left/right version better than it currently does.
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 we should trust the merge tool instead. Hopefully the idea about render conflict hunks as empty strings will work.
I think that the simplest behavior that makes sense is to default to starting with an empty file for $output
by default, but having an option to switch to outputting the file with conflict markers and then parsing the conflict markers.
For now, https://github.com/martinvonz/jj/pull/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.
Ah, you mean if the file gets replaced by a regular file with conflict markers in? .... then it's going to be an empty diff, except for the type transition
That's right, and the diff you described is what I saw.
FYI, another option for making the jj diff output less confusing in these cases would be to use a format closer to Git's "combined diff" format
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.
another option for diffs of conflicts would be to show a header near each resolved conflict
I like this idea. Currently, I resolve a conflict in a merge commit (for example), I find the diff jj
shows for that merge commit quite difficult to understand (the coloring helps a lot, I'm completely lost without it).
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 jj status
. In all other cases, I can find out everything important about a commit with jj log -r
and jj diff -r
commands. I'm not sure what a good solution to this would be, though. Perhaps there could be a jj resolve list
command, or the commands could be jj conflict resolve
and jj conflict list
.
BTW, I tried out the case of the user messing up the conflict markers you mentioned in https://github.com/martinvonz/jj/issues/18#issuecomment-1296526577. 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 jj obslog
+ jj restore
or jj op restore
. I'm not sure if there's an easy fix, but perhaps this is not a huge problem.
One option is to render conflicts as empty strings in the pre-populated output file. I just tried vimdiff to get a better understanding for how it works. It seems it doesn't actually help you merge at all. .....
I find it very helpful simply to see the three versions of the file in three separate panes. The difftool functionality I use in
vimdiff
is the fact that all the panes scroll simultaneously, the folding, and the]c
and[c
commands to navigate between changes. It is primitive, but I often find it much nicer than editing the conflict markers directly, and these are the only two options I know for conflict resolution in the terminal.
Sorry, I understand that vimdiff
helps you merge by presenting you with the diffs. What I meant is that it doesn't actually do any merging - it doesn't even resolve trivial conflicts. So even if a hunk changed only on one side, vimdiff
will not help you by putting the changed side in the output file (because it's a diff tool, not a merge tool). That's why the VCS needs to help it by merging as much as possible before handing control to vimdiff
.
For primitive tools like that, empty strings does seem to make sense (....). What do you think? Another option is to simply use the base file as initial output.
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
vimdiff
would still be hard to use with that. It would make more sense ifvimdiff
was a more feature-full merge tool and highlighted the differences between the output window and the left/right version better than it currently does.
Makes sense.
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.
We may want a per-tool option for that, but that can come later.
I think we should trust the merge tool instead. Hopefully the idea about render conflict hunks as empty strings will work.
I think that the simplest behavior that makes sense is to default to starting with an empty file for
$output
by default, but having an option to switch to outputting the file with conflict markers and then parsing the conflict markers.
Yes. Now that I understand better what vimdiff
is, the "premerge" config in Mercurial finally makes more sense. That's exactly what you're suggesting, I think. I agree that we should copy that feature.
FYI, another option for making the jj diff output less confusing in these cases would be to use a format closer to Git's "combined diff" format
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.
FWIW, Git has only very recently gained an option to show a diff very similar to ours.
another option for diffs of conflicts would be to show a header near each resolved conflict
I like this idea. Currently, I resolve a conflict in a merge commit (for example), I find the diff
jj
shows for that merge commit quite difficult to understand (the coloring helps a lot, I'm completely lost without it).
I doubt I'll prioritize implementing that format soon, but it's probably not very hard.
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.
Since the diff contents in our jj diff
format are indented, I don't think that will be a problem.
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
jj status
. In all other cases, I can find out everything important about a commit withjj log -r
andjj diff -r
commands. I'm not sure what a good solution to this would be, though. Perhaps there could be ajj resolve list
command, or the commands could bejj conflict resolve
andjj conflict list
.
Good point, we may want to split it up like that.
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
jj obslog
+jj restore
orjj op restore
. I'm not sure if there's an easy fix, but perhaps this is not a huge problem.
You shouldn't need jj op restore
. If the conflict was also in the parent commit, you can restore it from there (jj restore --from @- some/path
). Otherwise you can restore it using the obslog, as you found.
@ilyagr, do you think we should close this now? I think what you implemented in #689 is enough to let us say that we support external merge tools. I know you're already adding more features, so you decide if you want to finish those first.
I'll close this one. Some of the TODOs in the code may or may not become new bugs.
I don't know if this is related, but setting the
diff_editor
under[ui]
(in my case to"vimdiff"
) as instructed in the tutorial doesn't seem to work: