feat(mergiraf solve): improve error output for jj users #318

As said in #317 (comment), by the time we reach this, it might already be too late -- Mergiraf would think that a file with jj-style conflicts doesn't have conflicts at all (since they won't match the conflict regex), and happily return it in the Ok variant. So all of this should be happening sooner.

Even if you don't want to go with the regex approach, I think the best option would be to modify this part of ParsedMerge::parse (the method that parses files with conflicts):

Namely, I think it would make sense to move the error handling added by this PR (probably packed in a helper function) to after we try to match for diff2.

That will of course make the no-conflict case slower, but it's a very-happy path anyway (meaning it's very fast elsewhere), so slowing it down a little here shouldn't be too bad

Here's the output I get from running mergiraf solve Example.java in the example-repo:

WARN Error while resolving conflicts: parse error at 0..21, starting with: <<<<<<< Conflict 1 of
WARN Mergiraf: Could not generate any solution
INFO You seem to be using Jujutsu instead of Git. Consider using `jj resolve --tool mergiraf <file>` instead.

In what situation would Mergiraf think there are no conflicts at all?

In what situation would Mergiraf think there are no conflicts at all?

TLDR: it actually does in this one, and it's only by luck (and robust logic I guess) that everything doesn't blow up violently.

in this exact one -- see the verbose output:

DEBUG re-constructing revisions from parsed merge took 20.814µs
DEBUG initializing the parser took 13.849µs
WARN Error while resolving conflicts: parse error at 0..21, starting with: <<<<<<< Conflict 1 of
error while retrieving Base revision for Example.java:
git checkout-index: example-repo/Example.java is not in the cache

WARN Full structured merge failed: Could not retrieve conflict sides from Git.
DEBUG ~~~ Solve statistics ~~~
DEBUG from_parsed_original: 0 conflict(s), 0 mass, has_additional_issues: false
WARN Mergiraf: Could not generate any solution
INFO You seem to be using Jujutsu instead of Git. Consider using `jj resolve --tool mergiraf <file>` instead.

I guess it might be a bit convoluted, so here's the important bit:

DEBUG from_parsed_original: 0 conflict(s), 0 mass, has_additional_issues: false` means

from_parsed_original refers to what have parsed (with ParsedMerge::parse) from the input file. As you can see, Mergiraf reports no problems at all with this "solution", and that's precisely because it didn't recognize any of the conflicts.

The only reason we don't happily return this clearly wrong "solution" is that we have a logic to look for a better one. So normally, we:

1. get a from_parsed_original
2. reconstruct the three revisions from the conflict, so this:

A
<<<<<<< LEFT
B
||||||| BASE
C
=======
D
>>>>>>> RIGHT
E

becomes this:

# left
A
B
E

# base
A
C
E

# right
A
D
E

then parse those sides using tree-sitter and do a structured merge
3. if that fails, we resort to trying to get the original revisions (so before the merge was even attempted) from Git
4. And we return the best of those two solution only if it's better that what we started with.

But here, both of those fail:

1. since we fail to recognize the conflict, each of the "reconstructed" revisions still has the conflict in them. This then trips up tree-sitter, which returns an Err and so we abort structured resolution. The output above does mention this:

WARN Error while resolving conflicts: parse error at 0..21, starting with: <<<<<<< Conflict 1 of
error while retrieving Base revision for Example.java:

2. we try to extract the revisions from Git, but fail, since there is no top-level .git directory¹. Therefore, this strategy fails as well. Again, see the logs:

error while retrieving Base revision for Example.java:
git checkout-index: example-repo/Example.java is not in the cache

WARN Full structured merge failed: Could not retrieve conflict sides from Git.

So it's but a pure coincidence that everything works out relatively peacefully after all.

That's why I propose handling the error as early as possible. If we get a "jujutsu detected" error from ParsedMerge::parse, we can avoid calling both resolve_merge and structured_merge_from_git_revisions (the first and second strategy, respectively). That'd not only be faster, but would also avoid the confusing warnings before the final, actually helpful, suggestion

Now that I think of it, you could even just move the check to the very_beginning_ of mergiraf solve logic, so line 300 (in mergiraf.rs), and hard-error-out right away

Okay, that makes sense. So, you propose one more regex in ParsedMerge::parse? Why not shell out to jj earlier? Maybe the performance impact isn't so bad. And even git users are not expected to use mergiraf solve regularly, right? The recommended way is to let git call mergiraf merge automatically.

TBH my regex experience is very limited, I'd prefer to use a parser combinator if I had to do something like detect conflict markers.

we try to extract the revisions from Git, but fail, since there is no top-level .git directory [...] it's surprising to me that Jujutsu is bold enough to already do that by default, I thought it'd try to maintain compatibility for somewhat longer

It's always been this way I think. The important piece of git compatibility is that you can push to a git remote, so you can choose to use jj while your coworkers stay on git. So it's about overcoming the network effects of a VCS. Existing local tooling working is also very useful, but colocating a repo also has a few downsides noted here, so it's not done by default.

Okay, that makes sense. So, you propose one more regex in ParsedMerge::parse? Why not shell out to jj earlier?

Yeah I initially thought we'd need the former, but now I think we could actually do the latter, see my last comment (the small one)

And even git users are not expected to use mergiraf solve regularly, right? The recommended way is to let git call mergiraf merge automatically.

Well it's recommended if you prefer doing everything manually. But yeah, I'd expect most users to have it done automatically. Though this is the other way around with Jujutsu, funnily enough.

TBH my regex experience is very limited, I'd prefer to use a parser combinator if I had to do something like detect conflict markers.

I wanted to do that, too, but unfortunately failed.

It's always been this way I think. The important piece of git compatibility is that you can push to a git remote, so you can choose to use jj while your coworkers stay on git. So it's about overcoming the network effects of a VCS. Existing local tooling working is also very useful, but colocating a repo also has a few downsides noted here, so it's not done by default.

I see, thanks for the explanation! And that link is exactly what I would've liked to see after "colocated" in the explanation comment you added (but we decided to ditch it so it's not important)

@ada4a wrote in #318 (comment):

Now that I think of it, you could even just move the check to the very_beginning_ of mergiraf solve logic, so line 300 (in mergiraf.rs), and hard-error-out right away

done!

colocated?

A Jujutsu repository can either be a .jj directory with the backing git repo stored in .jj/repo/store/git, or a .jj and .git directory side-by-side. The latter is what's usually recommended, because many git-related tools keep working.

Yeah I think that comment would only be clear to someone who is already moderatly knowledgeable about Jujutsu. I'd replace "may not be colocated" with "may be configured to store .git as a subdirectory of .jj instead of side-by-side (<link to a Jujutsu documentation article that talks about this possibility)".

because many git-related tools keep working

Does this imply that some git-related tools are in fact able to handle this? How? Now that I think of it, they probably read GIT_DIR instead of hard-coding it as .git?

Well, to my knowledge, we in Mergiraf don't hardcode it either. In fact I don't think we even look for it anywhere – on the rare occasions that we do need to interact with the repo, we just shell out to Git.

I felt like the comment was getting a bit wordy and confusing. The point about the .git directory isn't even that important, the conflict marker style is the main issue. I removed that part.

Does this imply that some git-related tools are in fact able to handle this? How? Now that I think of it, they probably read GIT_DIR instead of hard-coding it as .git?

Well, to my knowledge, we in Mergiraf don't hardcode it either. In fact I don't think we even look for it anywhere – on the rare occasions that we do need to interact with the repo, we just shell out to Git.

Well, no. If a Jujutsu repo is not colocated, meaning the .git directory is stored within the .jj repo, pretty much all tooling breaks because they don't recognize that you're in a repository in the first place. Reading GIT_DIR wouldn't help much, because Jujutsu doesn't set that for you. Shelling out to Git is perfectly fine and the right way to go in my view. Jujutsu actually used to use libgit2 and has moved to shelling out for some things because its more reliable.

I.. don't get it. If neither GIT_DIR is set, nor is .git in its correct location, how is Git-that-we-shell-out-to even able to do anything?

It isn't. In what situation does mergiraf shell out to git? It must not do that for mergiraf merge, which is how jj calls it. Otherwise git would certainly fail.

There's actually one at the very beginning! You can pass an envvar to disable Mergiraf – you can want to do that when running git {merge,rebase}. In that case, Mergiraf falls back to git merge-file, the lower-level command that takes three files and, well, merges them – the function's called fallback_to_git_merge_file.

So I guess that's quite an important case indeed, and what we'd like to do there is to check for Jujutsu and call some lower-level analogue of jj resolve instead of git merge-file.

It might be that there are more shell-outs, but I think they're mostly in testing, and one in mergiraf solve which is not relevant here as you say

I see. For that particular case, I don't think it's relevant to jujutsu. Since every call to mergiraf is made intentionally with jj resolve --tool mergiraf, there would be no point in disabling mergiraf, right?

Hmm, I guess so... I want to ask whether there's a way set up a default --tool – but even if there were one, and you were to set Mergiraf as default, but wanted to use something different temporarily, you'd probably just specify it with --tool, so no need for the envvar either.

I'll try to take a look at the code tomorrow to check that there aren't any more shell-outs I forgot

Right. The closest thing to a default merge tool would be to add an alias for it, e.g. the config

[aliases]
rmg = ["resolve","--tool","mergiraf"]

will allow you to run jj rmg [file] instead, but that's still very intentionally using mergiraf.

Marking this as resolved because we now error out before getting any opportunity to shell out to Git

• you could use Result::is_ok_and to collapse these two ifs
• you should silent the output of jj root (if any) by piping stdout/err to /dev/null:

.stdout(Stdio::null()).stderr(Stdio::null())

Applied the first suggestion 👍 capturing stdout and stderr is already the default for .output(), .spawn() would be the alternative way to start a process that inherits stdout and stderr from its parent.

Ah, didn't know the latter, thanks for the info!

I think the detailed explanation is nice, but it's a shame that the user doesn't see any of it. I think it'd be good to argue why we think they should run jj resolve, and showing (a part of) this comment might be the way to do that. Here's roughly what I have in mind:

@@ -297,18 +297,20 @@ fn real_main(args: CliArgs) -> Result<i32, String> {
                 // update its value with what of `--keep`
                 stdout = keep;
             }
-            // Check if user is using Jujutsu instead of Git, which can lead
-            // to issues. Jujutsu has its own style of conflict markers,
-            // which Mergiraf doesn't understand. Jujutsu users shouldn't call
-            // `mergiraf solve` directly, because it has a builtin configuration
-            // to resolve conflicts manually using `mergiraf merge`.
+
+            // Check if user is using Jujutsu instead of Git, which can lead to issues.
             if Command::new("jj")
                 .arg("root")
                 .output()
                 .is_ok_and(|o| o.status.success())
             {
-                return Err("You seem to be using Jujutsu instead of Git. \
-                    Please use `jj resolve --tool mergiraf [file]`."
+                return Err("\
+You seem to be using Jujutsu instead of Git.
+Please use `jj resolve --tool mergiraf [file]`.
+
+Jujutsu has its own style of conflict markers, which Mergiraf doesn't understand.
+Jujutsu users shouldn't call `mergiraf solve` directly, because it has
+a builtin configuration to resolve conflicts manually using `mergiraf merge`."
                     .into());
             }
 

The alignment at the right is kind of wonky, but I'm not sure we should hand-wrap the text to any particular width, or just leave everything on one line and have the terminal do the rest

updated 👍 It's a wordy error message, but users should only see it once, so it's fine. If you

I used \n\ for indentation, another option would be to use the indoc crate.

TIL about indoc – looks useful! Also didn't know about \n\-s – I was trying to use , but it would eat the entire empty line after the first paragraph. Actually, isn't a \n\ only really needed on that empty line? I'm pretty sure on the other lines just \ should be enough

There is a difference in behavior. \ eats all whitespace following it, including any newlines. So without the \n before it, the text would be on a single line, soft-wrapped by the terminal. With \n\ on the other hand, lines are hard-wrapped at the places we defined.

With \, except for the empty line:

Mergiraf: You seem to be using Jujutsu instead of Git. Please use `jj resolve --tool mergiraf [file]`.
Jujutsu has its own style of conflict markers, which Mergiraf doesn't understand. Jujutsu users shouldn't call `mergiraf solve` directly, because Jujutsu has a builtin configuration to resolve conflicts manually using `mergiraf merge`.

With \n\ on every line:

Mergiraf: You seem to be using Jujutsu instead of Git.
Please use `jj resolve --tool mergiraf [file]`.

Jujutsu has its own style of conflict markers, which Mergiraf doesn't understand.
Jujutsu users shouldn't call `mergiraf solve` directly, because Jujutsu has
a builtin configuration to resolve conflicts manually using `mergiraf merge`.

It just depends on what we want.

I see, thanks! I think I'd have the first two lines as separate lines, then an empty line, then the rest soft-wrapped. What do you think?

I like that! updated 🙂