(debug) assertion failure at MergedTree::merge() by rebasing resolvable commit

[yuja]

Description

Maybe this is another case of 7fda80f? Without debug assertion, we'll get a conflicted file with no conflict markers.

Steps to Reproduce the Problem

rm -Rf repo
jj init --git repo
cd repo

# create conflict in subtree 'x'
jj new -mR 'root()'
mkdir x
echo R > x/file

jj new -mA 'description(R)'
echo A >> x/file

jj new -mB 'description(R)'
echo B >> x/file

jj rebase -r 'description(A)' -d 'description(B)'
jj log --no-pager
jj resolve -lr 'description(A)'

# add deletion to conflict (which I don't know if differs from adding more conflicts)
jj rebase -r 'description(A)' -d 'root()'
jj log --no-pager
jj resolve -lr 'description(A)'

# new base to which the conflicted commit can be merged cleanly
jj new -mC 'root()'
mkdir x
cat <<'EOF' > x/file
C
R
EOF

# rebase onto it
jj rebase -r 'description(A)' -d 'description(C)'
jj log --no-pager

Expected Behavior

The last jj rebase succeeds.

Actual Behavior

+ jj rebase -r description(A) -d description(C)
thread 'main' panicked at lib/src/merged_tree.rs:401:17:
assertion `left == right` failed
  left: Resolved(Tree { dir: "", id: TreeId("1ad9dd045be836edfd3844740ce51c475cd39ef6") })
 right: Conflicted([Tree { dir: "", id: TreeId("a0a6a5e7cc7ecd3a80c5331268d057c4b69dc390") }, Tree { dir: "", id: TreeId("e54e2235e3ca7ad9718d23af1d4e7cc663f626a8") }, Tree { dir: "", id: TreeId("0995303dc5e81edeba1c9e48b305d22219b925f0") }])
stack backtrace:
   0: rust_begin_unwind
             at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/std/src/panicking.rs:597:5
   1: core::panicking::panic_fmt
             at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/core/src/panicking.rs:72:14
   2: core::panicking::assert_failed_inner
   3: core::panicking::assert_failed
             at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/core/src/panicking.rs:270:5
   4: jj_lib::merged_tree::MergedTree::merge
             at /home/yuya/work/2023/jj/lib/src/merged_tree.rs:401:17
   5: jj_lib::rewrite::rebase_commit_with_options
             at /home/yuya/work/2023/jj/lib/src/rewrite.rs:147:13
   6: jj_lib::rewrite::rebase_commit
             at /home/yuya/work/2023/jj/lib/src/rewrite.rs:107:5
   7: jj_cli::commands::rebase::rebase_revision
             at /home/yuya/work/2023/jj/cli/src/commands/rebase.rs:423:5
   8: jj_cli::commands::rebase::cmd_rebase
             at /home/yuya/work/2023/jj/cli/src/commands/rebase.rs:197:9
   9: jj_cli::commands::run_command
             at /home/yuya/work/2023/jj/cli/src/commands/mod.rs:187:39
  10: core::ops::function::FnOnce::call_once
             at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/core/src/ops/function.rs:250:5
  11: core::ops::function::FnOnce::call_once{{vtable.shim}}
             at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/core/src/ops/function.rs:250:5
  12: <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once
             at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/alloc/src/boxed.rs:2007:9
  13: jj_cli::cli_util::CliRunner::run_internal
             at /home/yuya/work/2023/jj/cli/src/cli_util.rs:2876:9
  14: jj_cli::cli_util::CliRunner::run
             at /home/yuya/work/2023/jj/cli/src/cli_util.rs:2893:22
  15: jj::main
             at /home/yuya/work/2023/jj/cli/src/main.rs:18:5
  16: core::ops::function::FnOnce::call_once
             at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/core/src/ops/function.rs:250:5

Specifications

• Platform: Linux
• Version: 0.11.0-ec7f9e79f67d9a5a12d221c60a91d59b5eb6b925 (debug assertion enabled)

[martinvonz]

Thanks for reporting. I don't know what's going on there yet. It bothers me, but the bug is quite harmless, so I'll probably prioritize more impactful changes in the near future.

[yuja]

I think the problem is that the conflicted tree may be padded with absent trees, whereas file-level conflict resolution requires that all tree values are files.

jj/lib/src/tree.rs

Lines 399 to 460 in 48d586c

	pub fn try_resolve_file_conflict(
	store: &Store,
	filename: &RepoPath,
	conflict: &MergedTreeValue,
	) -> Result<Option<TreeValue>, TreeMergeError> {
	// If there are any non-file or any missing parts in the conflict, we can't
	// merge it. We check early so we don't waste time reading file contents if
	// we can't merge them anyway. At the same time we determine whether the
	// resulting file should be executable.
	let Some(file_id_conflict) = conflict.maybe_map(|term| match term {
	Some(TreeValue::File { id, executable: _ }) => Some(id),
	_ => None,
	}) else {
	return Ok(None);
	};
	let Some(executable_conflict) = conflict.maybe_map(|term| match term {
	Some(TreeValue::File { id: _, executable }) => Some(executable),
	_ => None,
	}) else {
	return Ok(None);
	};
	let Some(&&executable) = executable_conflict.resolve_trivial() else {
	// We're unable to determine whether the result should be executable
	return Ok(None);
	};
	if let Some(&resolved_file_id) = file_id_conflict.resolve_trivial() {
	// Don't bother reading the file contents if the conflict can be trivially
	// resolved.
	return Ok(Some(TreeValue::File {
	id: resolved_file_id.clone(),
	executable,
	}));
	}
	// Simplify the conflict for two reasons:
	// 1. Avoid reading unchanged file contents
	// 2. The simplified conflict can sometimes be resolved when the unsimplfied one
	// cannot
	let file_id_conflict = file_id_conflict.simplify();
	let contents: Merge<Vec<u8>> =
	file_id_conflict.try_map(|&file_id| -> Result<Vec<u8>, TreeMergeError> {
	let mut content = vec![];
	store
	.read_file(filename, file_id)?
	.read_to_end(&mut content)
	.map_err(|err| TreeMergeError::ReadError {
	source: err,
	file_id: file_id.clone(),
	})?;
	Ok(content)
	})?;
	let slices = contents.map(|content| content.as_slice());
	let merge_result = files::merge(&slices);
	match merge_result {
	MergeResult::Resolved(merged_content) => {
	let id = store.write_file(filename, &mut merged_content.0.as_slice())?;
	Ok(Some(TreeValue::File { id, executable }))
	}
	MergeResult::Conflict(_) => Ok(None),
	}
	}

Inserting .simplify() somewhere before the file_id_conflict extraction appears to be fixed the problem.

[martinvonz]

Oh, that makes sense!

[yuja]

Hmm, it might be because an empty subtree is added to the parent while propagating conflicts up, creating non-empty parent tree containing empty trees. I'll take a look.

[UPDATED] or maybe there are two separate problems. Adding empty subtree to parent is probably wrong, but merge_tree_values() should be able to handle absent value represented by None.