feat: mergetool support

mitchellwrosen commented 1 month ago

Overview

This PR adds a primitive mergetool support to ucm.

If a project/main> merge /topic fails due to conflicts, and you have a UCM_MERGETOOL env var set, we will first write four files:

<tmpdir>/main.u
<tmpdir>/topic.u
<tmpdir>/main-topic-base.u
main-topic-merged.u

The first three files contain the conflicted definitions on the respective branches and the LCA (the "base"), followed by a fold line, followed by all dependents of the merge (from both main and topic), all rendered in a PPE that favors the target branch (main). The fourth file contains a two-way diff between main and topic with git-like merge markers.

Then we spawn the process specified in UCM_MERGETOOL, with the sentinel strings $BASE, $LOCAL, $REMOTE, and $MERGED substituted for <tmpdir>/main-topic-base.u.tmp, <tmpdir>/main.u.tmp, <tmpdir>/topic.u.tmp and main-topic-merged.u, respectively. It is the user's responsibility to provide a mergetool that actually spawns in a new tab/window/etc.

To get started with a dummy "no-op" mergetool that just prints the files, you could use

UCM_MERGETOOL='echo us=$LOCAL them=$REMOTE lca=$BASE merged=$MERGED'

and then fiddle with the files afterwards. The existence of the UCM_MERGETOOL environment variable is what triggers the new behavior of separating the conflicted definitions into separate files (and additionally providing the LCA definitions), versus the old behavior of putting everything in one file.

Test coverage

I've tested this manually. It'd be easy enough to get under test in transcripts with a new debug command like debug.merge-with-mergetool that accepts a string argument for UCM_MERGETOOL, but we decided that manual testing + dogfooding is sufficient for now.

Sample merge tool commands:

tool	sample command-line	notes
vscode (mac)	`UCM_MERGETOOL='/Applications/Visual\ Studio\ Code.app/Contents/Resources/app/bin/code --merge "$REMOTE" "$LOCAL" "$BASE" "$MERGED"' ucm`	supports LSP, but the merge is a bit confusing
intellij idea (mac)	`UCM_MERGETOOL='/Applications/IntelliJ\ IDEA\ CE.app/Contents/MacOS/idea merge "$LOCAL" "$REMOTE" "$BASE" "$MERGED"' ucm`	intellij idea produces a lot of output, and ucm seems to be put into the background somehow? fix with `fg`
intellij idea launched in new terminal (mac)	`UCM_MERGETOOL='osascript -e '"'"'tell app "Terminal" to do script "/Applications/IntelliJ\\ IDEA\\ CE.app/Contents/MacOS/idea merge \"$LOCAL\" \"$REMOTE\" \"$BASE\" \"$MERGED\""'"'" ucm`
filemerge (mac)	`UCM_MERGETOOL='opendiff "$LOCAL" "$REMOTE" -ancestor "$BASE" -merge "$MERGED"' ucm`
p4merge (mac)	`UCM_MERGETOOL='/Applications/p4merge.app/Contents/MacOS/p4merge "$BASE" "$LOCAL" "$REMOTE" "$MERGED" 2>&1 >/dev/null &' ucm`	solid merge tool

aryairani commented 1 month ago

Just to document some of the pieces we specifically want to evaluate and/or change before merging:

currently , temp files are placed in the current directory, with *.u.tmp extensions to avoid being caught by the typechecker. We could still put them in /tmp/<tmpdir>/*.u since they are temp files and we may not want to clutter the project directory either, while also enabling syntax highlighting for unison editors. (There may not be a lot of unison editors yet, but the people doing a unison merge are more likely to have one.)
upon more digging, I'm not sure if MERGETOOL as an environment variable is a standard thing? (It is a standard thing as a git config variable.) If it's not, we should probably just switch to UCM_MERGETOOL to match our other variables.

Also want to put out a note at release time about our longer-term plans for merges in Unison.

Side note: apparently git allows configuring a separate environment variable for a gui mergetool than for the tui one. (--gui option). I wonder what the point of that is.

mitchellwrosen commented 1 month ago

Ok, putting files in a temp dir and switching to UCM_MERGETOOL sounds good to me.

aryairani commented 1 month ago

I'm about to test out vscode as a mergetool (supposedly the magic string is

code --wait --merge $REMOTE $LOCAL $BASE $MERGED

), and maybe a few others, to make sure we've made the right assumptions about $MERGED.

aryairani commented 1 month ago

Looking pretty great:

In initial testing, a couple things jumped out:

we want some feedback if the command fails; I think these 1 2 worked out well, though I'm not sure what kind of info we get back from the "shell" call?
it looks like tools (vscode at least) expect merged.u to exist. I think we want it to be another copy of the lca file.
the alice and bob temp file names become part of the interface:
could we put any randomness just in the directory part of the name and not in the filename, to clean this up? Maybe with withTempDirectory

aryairani commented 3 weeks ago

@mitchellwrosen I wanna try to get this merged for use in the knn demo work—

Besides the merge conflict, where did we land on merged.u, I think we were deciding between "a copy of lca" vs "has git diff markers for 2-3 versions"?

aryairani commented 3 weeks ago

Decent workaround is vscode's "compare new untitled text files" command, and then copying in the two versions from the scratch file

vs

mitchellwrosen commented 3 weeks ago

@aryairani I believe we landed on the latter, that merged.u is supposed to contain git merge markers

mitchellwrosen commented 3 weeks ago

@aryairani Ok, I think this is ready for review again

aryairani commented 2 weeks ago

Merging for now to be able to test it with other new features already in trunk, even though I think we should tweak the auto-load behavior after writing the files, and also we want to make a pass to limit the amount of time merge will work without a progress indicator, before doing a new release.

unisonweb / unison