Unresolvable conflict when merging with empty file

[matts1]

Description

When one commit creates an empty file, and another commit creates a file of the same name, you get a conflict that can never be resolved.

I believe the core issue is that there are no conflict markers in such a file, and so jj says "file was unmodified" and hence thinks the resolution failed.

Steps to Reproduce the Problem

1. jj git init
2. touch foo
3. jj new @-
4. echo "abc" > foo
5. jj rebase -s @ <other commit>
6. Attempt to resolve the merge that jj new created

Expected Behavior

Either there is no conflict, or the conflict resolution works

Actual Behavior

Conflict can never be resolved

Specifications

• Platform: Linux
• Version: Internal unstable version

[ilyagr]

Interesting. It should be possible to work around the bug by doing jj restore --from some_commit file (probably from one of the parent commits).

[matts1]

Yeah, I worked around by renaming the file, running jj status, then renaming it back

[Kintaro]

Following git's behavior, there is no conflict. So it seems jj is doing the same behavior here, but erroneously marks it as a conflict in the transaction.

[Kintaro]

Should this be handled as a conflict or just merge the files as git does?

[Kintaro]

Still new to the code base, but I think the issue stems from somewhere in MergeTree::merge. Looks like as_resolved doesn't try to resolve trivial conflicts.

(edit: scratch that, a trivial resolution is attempted in merge_trees)

[martinvonz]

Copying my answer from Discord to here:

I think it ideally should be considered a conflict. The problem is that we use the empty string as a placeholder for a missing file when materializing/rendering the conflict markers. So the conflict becomes this:

<<<<<<<
+++++++
%%%%%%%
+abc
>>>>>>>

I.e. a conflict adding "abc\n" on one side, "" on the other side, and "" in the base. That gets automatically resolved to just "abc\n". (It doesn't actually work exactly this way.) I wonder how well it would work to replace the missing-file placeholder by e.g. "". I'm not sure that's a good idea, and i don't want to use english-specific user-visible strings in the library code anyway

[ilyagr]

This is a little confusing to me: it seems like we shouldn't need to change the conflict markers. As you said,

<<<<<<<
+++++++
%%%%%%%
+abc
>>>>>>>

should already be auto-resolving to abc. This should auto-resolve the conflict between an empty new file and another new file to just "another new file", and there wouldn't be a bug. It's unclear without looking at the code which part of this doesn't work or whether I'm missing something.

I'm not sure that's a good idea, and i don't want to use english-specific user-visible strings in the library code anyway

Oh, I was just planning to add some with #1176 (comment). The plan is to eventually make the strings configurable, though, and they could be provided with the CLI.

[ilyagr]

I'm guessing that the problem is that the conflict is generated like that after auto-resolution of conflicts happens.

We could still introduce different conflict markers for empty files, but in terms of user-visible effects with this bug, things would be the same or worse. Creating an empty file on both sides is an example of what we've been calling "A + A - B" conflicts, which are auto-resolved to "A" at some cost to consistency in our conflict theory. It would behoove jj to recognize the situation, but I'm not sure it can do much with it unless we get a better idea of how to deal with the "A + A - B = A" situation.

[Kintaro]

If it's considered a conflict or not, the situation should be handled in some way. Marking it as a conflict but not providing a conflict marker might be confusing for users.

[martinvonz]

Creating an empty file on both sides is an example of what we've been calling "A + A - B" conflicts

I actually think "empty file" and "missing file" are not the same state, which is why I feel like this case should be considered a conflict. But it's a rare case in practice so we might want to just mark it resolved anyway.

[ilyagr]

If it's considered a conflict or not, the situation should be handled in some way. Marking it as a conflict but not providing a conflict marker might be confusing for users.

+1

I actually think "empty file" and "missing file" are not the same state.

I agree. In our theory, creating an empty file on both sides is a conflict where the base is the "missing file" state and both sides are the "empty file" state. So, the situation of this bug (one side being an "empty file" and the other being a "nonempty file") would be a conflict.

In practice, however, I don't think jj's behavior would change if we treated it that way. As long as we follow "A + A - B = A", it does not matter whether "B" is "missing" or "empty" (in the latter case, we'd have B=A).

[ilyagr]

Putting things another way, perhaps jj should simplify the conflict after reading it from a file? This would also mean that if the user edits a conflict to something trivially resolvable, jj would resolve the conflict upon reading the file.

What I said before is true if the answer to this question is "yes". If it's "no", we'd have to simplify the (emptyfile + file - missingfile) conflicts before materializing them, probably in the same place we simplify the "A + A - B" conflicts.

[martinvonz]

Putting things another way, perhaps jj should simplify the conflict after reading it from a file?

I agree, but I thought we already do. I suspect we're hitting this check:

jj/lib/src/conflicts.rs

Lines 370 to 380 in 2dc95bb

	// First check if the new content is unchanged compared to the old content. If
	// it is, we don't need parse the content or write any new objects to the
	// store. This is also a way of making sure that unchanged tree/file
	// conflicts (for example) are not converted to regular files in the working
	// copy.
	let mut old_content = Vec::with_capacity(content.len());
	let merge_hunk = extract_as_single_hunk(file_ids, store, path).await;
	materialize_merge_result(&merge_hunk, &mut old_content).unwrap();
	if content == old_content {
	return Ok(file_ids.clone());
	}

[ilyagr]

Good find! I think that's likely it.

[Kintaro]

Just tested. In the scenario of this issue, update_from_content is not called.

[martinvonz]

Just tested. In the scenario of this issue, update_from_content is not called.

Weird. I just checked as well and it is called for me, and it returns early because of that check I mentioned above.

[Kintaro]

Really weird on my end. It sometimes triggers, sometimes doesn't. Sometimes only when I run jj resolve. I'll wipe the repo and try again.

[ilyagr]

I'm currently thinking of resolving this by keeping this conflicted and materializing the file as a conflict. However, it turns out we currently wouldn't materialize a conflict involving an empty file, or any file not ending in a newline, correctly: #3968. If acceptable, I'd materialize it as though the empty file was a file with just a newline (see that bug).

Another option would be to not consider this a conflict. I'm going back and forth on whether this would be "more correct" in theory (considered as a composition of a conflict (empty file) - (no file) + (empty file) which we'd resolve via A - B + A = A rule and a normal edit), but it would require a special case deeper in the guts of jj's tree resolution, so I'm leaning against it.