Skip to content

Commit

Permalink
conflicts: add "ui.conflict-marker-style" config
Browse files Browse the repository at this point in the history
Adds a new "ui.conflict-marker-style" config option. The "diff" option
is the default jj-style conflict markers with a snapshot and a series of
diffs to apply to the snapshot. New conflict marker style options will
be added in later commits.

The majority of the changes in this commit are from passing the config
option down to the code that materializes the conflicts.

Example of "diff" conflict markers:

```
<<<<<<< Conflict 1 of 1
+++++++ Contents of side #1
fn example(word: String) {
    println!("word is {word}");
%%%%%%% Changes from base to side #2
-fn example(w: String) {
+fn example(w: &str) {
     println!("word is {w}");
>>>>>>> Conflict 1 of 1 ends
}
```
  • Loading branch information
scott2000 committed Nov 23, 2024
1 parent d409a31 commit e5cb9f9
Show file tree
Hide file tree
Showing 30 changed files with 797 additions and 172 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,10 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
- `op diff`
- `restore`

* New `ui.conflict-marker-style` config option to change how conflicts are
materialized in the working copy. The default option ("diff") renders
conflicts as a snapshot with a list of diffs to apply to the snapshot.

### Fixed bugs

* `jj config unset <TABLE-NAME>` no longer removes a table (such as `[ui]`.)
Expand Down
12 changes: 9 additions & 3 deletions cli/examples/custom-working-copy/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ use jj_lib::settings::UserSettings;
use jj_lib::signing::Signer;
use jj_lib::store::Store;
use jj_lib::working_copy::CheckoutError;
use jj_lib::working_copy::CheckoutOptions;
use jj_lib::working_copy::CheckoutStats;
use jj_lib::working_copy::LockedWorkingCopy;
use jj_lib::working_copy::ResetError;
Expand Down Expand Up @@ -240,14 +241,18 @@ impl LockedWorkingCopy for LockedConflictsWorkingCopy {
self.inner.snapshot(&options)
}

fn check_out(&mut self, commit: &Commit) -> Result<CheckoutStats, CheckoutError> {
fn check_out(
&mut self,
commit: &Commit,
options: &CheckoutOptions,
) -> Result<CheckoutStats, CheckoutError> {
let conflicts = commit
.tree()?
.conflicts()
.map(|(path, _value)| format!("{}\n", path.as_internal_file_string()))
.join("");
std::fs::write(self.wc_path.join(".conflicts"), conflicts).unwrap();
self.inner.check_out(commit)
self.inner.check_out(commit, options)
}

fn rename_workspace(&mut self, new_workspace_id: WorkspaceId) {
Expand All @@ -269,8 +274,9 @@ impl LockedWorkingCopy for LockedConflictsWorkingCopy {
fn set_sparse_patterns(
&mut self,
new_sparse_patterns: Vec<RepoPathBuf>,
options: &CheckoutOptions,
) -> Result<CheckoutStats, CheckoutError> {
self.inner.set_sparse_patterns(new_sparse_patterns)
self.inner.set_sparse_patterns(new_sparse_patterns, options)
}

fn finish(
Expand Down
69 changes: 57 additions & 12 deletions cli/src/cli_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ use jj_lib::backend::TreeValue;
use jj_lib::commit::Commit;
use jj_lib::config::ConfigError;
use jj_lib::config::ConfigNamePathBuf;
use jj_lib::conflicts::ConflictMarkerStyle;
use jj_lib::file_util;
use jj_lib::fileset;
use jj_lib::fileset::FilesetDiagnostics;
Expand Down Expand Up @@ -113,6 +114,7 @@ use jj_lib::str_util::StringPattern;
use jj_lib::transaction::Transaction;
use jj_lib::view::View;
use jj_lib::working_copy;
use jj_lib::working_copy::CheckoutOptions;
use jj_lib::working_copy::CheckoutStats;
use jj_lib::working_copy::SnapshotOptions;
use jj_lib::working_copy::WorkingCopy;
Expand Down Expand Up @@ -457,6 +459,7 @@ impl CommandHelper {
let stale_wc_commit = repo.store().get_commit(wc_commit_id)?;

let mut workspace_command = self.workspace_helper_no_snapshot(ui)?;
let checkout_options = workspace_command.checkout_options();

let repo = workspace_command.repo().clone();
let (mut locked_ws, desired_wc_commit) =
Expand All @@ -479,6 +482,7 @@ impl CommandHelper {
repo.op_id().clone(),
&stale_wc_commit,
&desired_wc_commit,
&checkout_options,
)?;

// TODO: Share this code with new/checkout somehow.
Expand Down Expand Up @@ -710,6 +714,7 @@ pub struct WorkspaceCommandEnvironment {
workspace_id: WorkspaceId,
immutable_heads_expression: Rc<UserRevsetExpression>,
short_prefixes_expression: Option<Rc<UserRevsetExpression>>,
conflict_marker_style: ConflictMarkerStyle,
}

impl WorkspaceCommandEnvironment {
Expand All @@ -730,6 +735,7 @@ impl WorkspaceCommandEnvironment {
workspace_id: workspace.workspace_id().to_owned(),
immutable_heads_expression: RevsetExpression::root(),
short_prefixes_expression: None,
conflict_marker_style: command.settings().conflict_marker_style()?,
};
env.immutable_heads_expression = env.load_immutable_heads_expression(ui)?;
env.short_prefixes_expression = env.load_short_prefixes_expression(ui)?;
Expand Down Expand Up @@ -791,6 +797,11 @@ impl WorkspaceCommandEnvironment {
&self.immutable_heads_expression
}

/// User-configured conflict marker style for materializing conflicts
pub fn conflict_marker_style(&self) -> ConflictMarkerStyle {
self.conflict_marker_style
}

fn load_immutable_heads_expression(
&self,
ui: &Ui,
Expand Down Expand Up @@ -896,6 +907,7 @@ impl WorkspaceCommandEnvironment {
self.revset_parse_context(),
id_prefix_context,
self.immutable_expression(),
self.conflict_marker_style,
&self.command.data.commit_template_extensions,
)
}
Expand Down Expand Up @@ -1138,6 +1150,12 @@ impl WorkspaceCommandHelper {
&self.env
}

pub fn checkout_options(&self) -> CheckoutOptions {
CheckoutOptions {
conflict_marker_style: self.env.conflict_marker_style(),
}
}

pub fn unchecked_start_working_copy_mutation(
&mut self,
) -> Result<(LockedWorkspace, Commit), CommandError> {
Expand Down Expand Up @@ -1321,7 +1339,12 @@ to the current parents may contain changes from multiple commits.

/// Creates textual diff renderer of the specified `formats`.
pub fn diff_renderer(&self, formats: Vec<DiffFormat>) -> DiffRenderer<'_> {
DiffRenderer::new(self.repo().as_ref(), self.path_converter(), formats)
DiffRenderer::new(
self.repo().as_ref(),
self.path_converter(),
self.env.conflict_marker_style(),
formats,
)
}

/// Loads textual diff renderer from the settings and command arguments.
Expand Down Expand Up @@ -1354,13 +1377,20 @@ to the current parents may contain changes from multiple commits.
tool_name: Option<&str>,
) -> Result<DiffEditor, CommandError> {
let base_ignores = self.base_ignores()?;
let conflict_marker_style = self.env.conflict_marker_style();
if let Some(name) = tool_name {
Ok(DiffEditor::with_name(name, self.settings(), base_ignores)?)
Ok(DiffEditor::with_name(
name,
self.settings(),
base_ignores,
conflict_marker_style,
)?)
} else {
Ok(DiffEditor::from_settings(
ui,
self.settings(),
base_ignores,
conflict_marker_style,
)?)
}
}
Expand Down Expand Up @@ -1389,10 +1419,11 @@ to the current parents may contain changes from multiple commits.
ui: &Ui,
tool_name: Option<&str>,
) -> Result<MergeEditor, MergeToolConfigError> {
let conflict_marker_style = self.env.conflict_marker_style();
if let Some(name) = tool_name {
MergeEditor::with_name(name, self.settings())
MergeEditor::with_name(name, self.settings(), conflict_marker_style)
} else {
MergeEditor::from_settings(ui, self.settings())
MergeEditor::from_settings(ui, self.settings(), conflict_marker_style)
}
}

Expand Down Expand Up @@ -1720,6 +1751,7 @@ to the current parents may contain changes from multiple commits.
.settings()
.max_new_file_size()
.map_err(snapshot_command_error)?;
let conflict_marker_style = self.env.conflict_marker_style();
let command = self.env.command.clone();
let mut locked_ws = self
.workspace
Expand Down Expand Up @@ -1787,6 +1819,7 @@ See https://martinvonz.github.io/jj/latest/working-copy/#stale-working-copy \
progress: progress.as_ref().map(|x| x as _),
start_tracking_matcher: &auto_tracking_matcher,
max_new_file_size,
conflict_marker_style,
})
.map_err(snapshot_command_error)?;
drop(progress);
Expand Down Expand Up @@ -1842,11 +1875,13 @@ See https://martinvonz.github.io/jj/latest/working-copy/#stale-working-copy \
new_commit: &Commit,
) -> Result<(), CommandError> {
assert!(self.may_update_working_copy);
let checkout_options = self.checkout_options();
let stats = update_working_copy(
&self.user_repo.repo,
&mut self.workspace,
maybe_old_commit,
new_commit,
&checkout_options,
)?;
if Some(new_commit) != maybe_old_commit {
if let Some(mut formatter) = ui.status_formatter() {
Expand Down Expand Up @@ -2101,7 +2136,7 @@ See https://martinvonz.github.io/jj/latest/working-copy/#stale-working-copy \
.collect(),
)?;
}
revset_util::warn_unresolvable_trunk(ui, new_repo, &self.env().revset_parse_context())?;
revset_util::warn_unresolvable_trunk(ui, new_repo, &self.env.revset_parse_context())?;

Ok(())
}
Expand Down Expand Up @@ -2409,18 +2444,22 @@ fn update_stale_working_copy(
op_id: OperationId,
stale_commit: &Commit,
new_commit: &Commit,
options: &CheckoutOptions,
) -> Result<CheckoutStats, CommandError> {
// The same check as start_working_copy_mutation(), but with the stale
// working-copy commit.
if stale_commit.tree_id() != locked_ws.locked_wc().old_tree_id() {
return Err(user_error("Concurrent working copy operation. Try again."));
}
let stats = locked_ws.locked_wc().check_out(new_commit).map_err(|err| {
internal_error_with_message(
format!("Failed to check out commit {}", new_commit.id().hex()),
err,
)
})?;
let stats = locked_ws
.locked_wc()
.check_out(new_commit, options)
.map_err(|err| {
internal_error_with_message(
format!("Failed to check out commit {}", new_commit.id().hex()),
err,
)
})?;
locked_ws.finish(op_id)?;

Ok(stats)
Expand Down Expand Up @@ -2616,13 +2655,19 @@ pub fn update_working_copy(
workspace: &mut Workspace,
old_commit: Option<&Commit>,
new_commit: &Commit,
options: &CheckoutOptions,
) -> Result<Option<CheckoutStats>, CommandError> {
let old_tree_id = old_commit.map(|commit| commit.tree_id().clone());
let stats = if Some(new_commit.tree_id()) != old_tree_id.as_ref() {
// TODO: CheckoutError::ConcurrentCheckout should probably just result in a
// warning for most commands (but be an error for the checkout command)
let stats = workspace
.check_out(repo.op_id().clone(), old_tree_id.as_ref(), new_commit)
.check_out(
repo.op_id().clone(),
old_tree_id.as_ref(),
new_commit,
options,
)
.map_err(|err| {
internal_error_with_message(
format!("Failed to check out commit {}", new_commit.id().hex()),
Expand Down
6 changes: 5 additions & 1 deletion cli/src/commands/file/show.rs
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,11 @@ fn write_tree_entries<P: AsRef<RepoPath>>(
io::copy(&mut reader, &mut ui.stdout_formatter().as_mut())?;
}
MaterializedTreeValue::FileConflict { contents, .. } => {
materialize_merge_result(&contents, &mut ui.stdout_formatter())?;
materialize_merge_result(
&contents,
workspace_command.env().conflict_marker_style(),
&mut ui.stdout_formatter(),
)?;
}
MaterializedTreeValue::OtherConflict { id } => {
ui.stdout_formatter().write_all(id.describe().as_bytes())?;
Expand Down
2 changes: 2 additions & 0 deletions cli/src/commands/file/track.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ pub(crate) fn cmd_file_track(
args: &FileTrackArgs,
) -> Result<(), CommandError> {
let mut workspace_command = command.workspace_helper(ui)?;
let conflict_marker_style = workspace_command.env().conflict_marker_style();
let matcher = workspace_command
.parse_file_patterns(ui, &args.paths)?
.to_matcher();
Expand All @@ -57,6 +58,7 @@ pub(crate) fn cmd_file_track(
progress: None,
start_tracking_matcher: &matcher,
max_new_file_size: command.settings().max_new_file_size()?,
conflict_marker_style,
})?;
let num_rebased = tx.repo_mut().rebase_descendants(command.settings())?;
if num_rebased > 0 {
Expand Down
2 changes: 2 additions & 0 deletions cli/src/commands/file/untrack.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ pub(crate) fn cmd_file_untrack(
args: &FileUntrackArgs,
) -> Result<(), CommandError> {
let mut workspace_command = command.workspace_helper(ui)?;
let conflict_marker_style = workspace_command.env().conflict_marker_style();
let store = workspace_command.repo().store().clone();
let matcher = workspace_command
.parse_file_patterns(ui, &args.paths)?
Expand Down Expand Up @@ -75,6 +76,7 @@ pub(crate) fn cmd_file_untrack(
progress: None,
start_tracking_matcher: &auto_tracking_matcher,
max_new_file_size: command.settings().max_new_file_size()?,
conflict_marker_style,
})?;
if wc_tree_id != *new_commit.tree_id() {
let wc_tree = store.get_root_tree(&wc_tree_id)?;
Expand Down
4 changes: 3 additions & 1 deletion cli/src/commands/operation/diff.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,9 @@ pub fn cmd_op_diff(
let diff_renderer = {
let formats = diff_formats_for_log(command.settings(), &args.diff_format, args.patch)?;
let path_converter = workspace_env.path_converter();
(!formats.is_empty()).then(|| DiffRenderer::new(merged_repo, path_converter, formats))
let conflict_marker_style = workspace_env.conflict_marker_style();
(!formats.is_empty())
.then(|| DiffRenderer::new(merged_repo, path_converter, conflict_marker_style, formats))
};
let id_prefix_context = workspace_env.new_id_prefix_context();
let commit_summary_template = {
Expand Down
11 changes: 9 additions & 2 deletions cli/src/commands/operation/log.rs
Original file line number Diff line number Diff line change
Expand Up @@ -167,8 +167,15 @@ fn do_op_log(
)?
};
let path_converter = workspace_env.path_converter();
let diff_renderer = (!diff_formats.is_empty())
.then(|| DiffRenderer::new(repo.as_ref(), path_converter, diff_formats.clone()));
let conflict_marker_style = workspace_env.conflict_marker_style();
let diff_renderer = (!diff_formats.is_empty()).then(|| {
DiffRenderer::new(
repo.as_ref(),
path_converter,
conflict_marker_style,
diff_formats.clone(),
)
});

show_op_diff(
ui,
Expand Down
10 changes: 9 additions & 1 deletion cli/src/commands/operation/show.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,15 @@ pub fn cmd_op_show(
let diff_renderer = {
let formats = diff_formats_for_log(command.settings(), &args.diff_format, args.patch)?;
let path_converter = workspace_env.path_converter();
(!formats.is_empty()).then(|| DiffRenderer::new(repo.as_ref(), path_converter, formats))
let conflict_marker_style = workspace_env.conflict_marker_style();
(!formats.is_empty()).then(|| {
DiffRenderer::new(
repo.as_ref(),
path_converter,
conflict_marker_style,
formats,
)
})
};

// TODO: Should we make this customizable via clap arg?
Expand Down
3 changes: 2 additions & 1 deletion cli/src/commands/sparse.rs
Original file line number Diff line number Diff line change
Expand Up @@ -211,11 +211,12 @@ fn update_sparse_patterns_with(
workspace_command: &mut WorkspaceCommandHelper,
f: impl FnOnce(&mut Ui, &[RepoPathBuf]) -> Result<Vec<RepoPathBuf>, CommandError>,
) -> Result<(), CommandError> {
let checkout_options = workspace_command.checkout_options();
let (mut locked_ws, wc_commit) = workspace_command.start_working_copy_mutation()?;
let new_patterns = f(ui, locked_ws.locked_wc().sparse_patterns()?)?;
let stats = locked_ws
.locked_wc()
.set_sparse_patterns(new_patterns)
.set_sparse_patterns(new_patterns, &checkout_options)
.map_err(|err| internal_error_with_message("Failed to update working copy paths", err))?;
let operation_id = locked_ws.locked_wc().old_operation_id().clone();
locked_ws.finish(operation_id)?;
Expand Down
3 changes: 2 additions & 1 deletion cli/src/commands/workspace/add.rs
Original file line number Diff line number Diff line change
Expand Up @@ -146,10 +146,11 @@ pub fn cmd_workspace_add(
};

if let Some(sparse_patterns) = sparsity {
let checkout_options = new_workspace_command.checkout_options();
let (mut locked_ws, _wc_commit) = new_workspace_command.start_working_copy_mutation()?;
locked_ws
.locked_wc()
.set_sparse_patterns(sparse_patterns)
.set_sparse_patterns(sparse_patterns, &checkout_options)
.map_err(|err| internal_error_with_message("Failed to set sparse patterns", err))?;
let operation_id = locked_ws.locked_wc().old_operation_id().clone();
locked_ws.finish(operation_id)?;
Expand Down
Loading

0 comments on commit e5cb9f9

Please sign in to comment.