From 7e46cc13dc63a3d45341b5635aabdca3b1607d99 Mon Sep 17 00:00:00 2001 From: Benjamin Tan Date: Thu, 28 Mar 2024 21:39:55 +0800 Subject: [PATCH] cli: print conflicted paths whenever the working copy is changed This is disabled when the global `--quiet` flag is used. --- CHANGELOG.md | 3 + cli/src/cli_util.rs | 105 ++++++++++++++++++++++- cli/src/commands/resolve.rs | 121 ++++----------------------- cli/src/commands/status.rs | 5 +- cli/tests/test_chmod_command.rs | 2 + cli/tests/test_diffedit_command.rs | 2 + cli/tests/test_repo_change_report.rs | 10 +++ cli/tests/test_resolve_command.rs | 4 +- cli/tests/test_restore_command.rs | 6 ++ 9 files changed, 145 insertions(+), 113 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d4c28ebcf..2badbd593 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -78,6 +78,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * new function `working_copies()` for revsets to show the working copy commits of all workspaces. +* The list of conflicted paths is printed whenever the working copy changes. + This can be disabled with the `--quiet` option. + ### Fixed bugs None. diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index 7b55b2206..620878232 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -14,7 +14,7 @@ use core::fmt; use std::borrow::Cow; -use std::collections::{HashMap, HashSet}; +use std::collections::{BTreeMap, HashMap, HashSet}; use std::env::{self, ArgsOs, VarError}; use std::ffi::OsString; use std::fmt::Debug; @@ -34,13 +34,14 @@ use clap::error::{ContextKind, ContextValue}; use clap::{ArgAction, ArgMatches, Command, FromArgMatches}; use indexmap::{IndexMap, IndexSet}; use itertools::Itertools; -use jj_lib::backend::{ChangeId, CommitId, MergedTreeId}; +use jj_lib::backend::{ChangeId, CommitId, MergedTreeId, TreeValue}; use jj_lib::commit::Commit; use jj_lib::git_backend::GitBackend; use jj_lib::gitignore::{GitIgnoreError, GitIgnoreFile}; use jj_lib::hex_util::to_reverse_hex; use jj_lib::id_prefix::IdPrefixContext; use jj_lib::matchers::{EverythingMatcher, Matcher, PrefixMatcher}; +use jj_lib::merge::MergedTreeValue; use jj_lib::merged_tree::MergedTree; use jj_lib::object_id::ObjectId; use jj_lib::op_store::{OpStoreError, OperationId, WorkspaceId}; @@ -1127,6 +1128,15 @@ See https://github.com/martinvonz/jj/blob/main/docs/working-copy.md#stale-workin if let Some(stats) = stats { print_checkout_stats(ui, stats, new_commit)?; } + if Some(new_commit) != maybe_old_commit { + if let Some(mut formatter) = ui.status_formatter() { + let conflicts = new_commit.tree()?.conflicts().collect_vec(); + if !conflicts.is_empty() { + writeln!(formatter, "There are unresolved conflicts at these paths:")?; + print_conflicted_paths(&conflicts, formatter.as_mut(), self)?; + } + } + } Ok(()) } @@ -1557,6 +1567,97 @@ pub fn check_stale_working_copy( } } +#[instrument(skip_all)] +pub fn print_conflicted_paths( + conflicts: &[(RepoPathBuf, MergedTreeValue)], + formatter: &mut dyn Formatter, + workspace_command: &WorkspaceCommandHelper, +) -> Result<(), CommandError> { + let formatted_paths = conflicts + .iter() + .map(|(path, _conflict)| workspace_command.format_file_path(path)) + .collect_vec(); + let max_path_len = formatted_paths.iter().map(|p| p.len()).max().unwrap_or(0); + let formatted_paths = formatted_paths + .into_iter() + .map(|p| format!("{:width$}", p, width = max_path_len.min(32) + 3)); + + for ((_, conflict), formatted_path) in std::iter::zip(conflicts.iter(), formatted_paths) { + let sides = conflict.num_sides(); + let n_adds = conflict.adds().flatten().count(); + let deletions = sides - n_adds; + + let mut seen_objects = BTreeMap::new(); // Sort for consistency and easier testing + if deletions > 0 { + seen_objects.insert( + format!( + // Starting with a number sorts this first + "{deletions} deletion{}", + if deletions > 1 { "s" } else { "" } + ), + "normal", // Deletions don't interfere with `jj resolve` or diff display + ); + } + // TODO: We might decide it's OK for `jj resolve` to ignore special files in the + // `removes` of a conflict (see e.g. https://github.com/martinvonz/jj/pull/978). In + // that case, `conflict.removes` should be removed below. + for term in itertools::chain(conflict.removes(), conflict.adds()).flatten() { + seen_objects.insert( + match term { + TreeValue::File { + executable: false, .. + } => continue, + TreeValue::File { + executable: true, .. + } => "an executable", + TreeValue::Symlink(_) => "a symlink", + TreeValue::Tree(_) => "a directory", + TreeValue::GitSubmodule(_) => "a git submodule", + TreeValue::Conflict(_) => "another conflict (you found a bug!)", + } + .to_string(), + "difficult", + ); + } + + write!(formatter, "{formatted_path} ")?; + formatter.with_label("conflict_description", |formatter| { + let print_pair = |formatter: &mut dyn Formatter, (text, label): &(String, &str)| { + write!(formatter.labeled(label), "{text}") + }; + print_pair( + formatter, + &( + format!("{sides}-sided"), + if sides > 2 { "difficult" } else { "normal" }, + ), + )?; + write!(formatter, " conflict")?; + + if !seen_objects.is_empty() { + write!(formatter, " including ")?; + let seen_objects = seen_objects.into_iter().collect_vec(); + match &seen_objects[..] { + [] => unreachable!(), + [only] => print_pair(formatter, only)?, + [first, middle @ .., last] => { + print_pair(formatter, first)?; + for pair in middle { + write!(formatter, ", ")?; + print_pair(formatter, pair)?; + } + write!(formatter, " and ")?; + print_pair(formatter, last)?; + } + }; + } + Ok(()) + })?; + writeln!(formatter)?; + } + Ok(()) +} + pub fn print_checkout_stats( ui: &mut Ui, stats: CheckoutStats, diff --git a/cli/src/commands/resolve.rs b/cli/src/commands/resolve.rs index fcae53b61..ada794d82 100644 --- a/cli/src/commands/resolve.rs +++ b/cli/src/commands/resolve.rs @@ -12,19 +12,14 @@ // See the License for the specific language governing permissions and // limitations under the License. -use std::collections::BTreeMap; use std::io::Write; use itertools::Itertools; -use jj_lib::backend::TreeValue; -use jj_lib::merge::MergedTreeValue; use jj_lib::object_id::ObjectId; -use jj_lib::repo_path::RepoPathBuf; use tracing::instrument; -use crate::cli_util::{CommandHelper, RevisionArg, WorkspaceCommandHelper}; +use crate::cli_util::{print_conflicted_paths, CommandHelper, RevisionArg}; use crate::command_error::{cli_error, CommandError}; -use crate::formatter::Formatter; use crate::ui::Ui; /// Resolve a conflicted file with an external merge tool @@ -111,107 +106,21 @@ pub(crate) fn cmd_resolve( format!("Resolve conflicts in commit {}", commit.id().hex()), )?; - if let Some(mut formatter) = ui.status_formatter() { - let new_tree = new_commit.tree()?; - let new_conflicts = new_tree.conflicts().collect_vec(); - if !new_conflicts.is_empty() { - writeln!( - formatter, - "After this operation, some files at this revision still have conflicts:" - )?; - print_conflicted_paths(&new_conflicts, formatter.as_mut(), &workspace_command)?; - } - } - Ok(()) -} - -#[instrument(skip_all)] -pub(crate) fn print_conflicted_paths( - conflicts: &[(RepoPathBuf, MergedTreeValue)], - formatter: &mut dyn Formatter, - workspace_command: &WorkspaceCommandHelper, -) -> Result<(), CommandError> { - let formatted_paths = conflicts - .iter() - .map(|(path, _conflict)| workspace_command.format_file_path(path)) - .collect_vec(); - let max_path_len = formatted_paths.iter().map(|p| p.len()).max().unwrap_or(0); - let formatted_paths = formatted_paths - .into_iter() - .map(|p| format!("{:width$}", p, width = max_path_len.min(32) + 3)); - - for ((_, conflict), formatted_path) in std::iter::zip(conflicts.iter(), formatted_paths) { - let sides = conflict.num_sides(); - let n_adds = conflict.adds().flatten().count(); - let deletions = sides - n_adds; - - let mut seen_objects = BTreeMap::new(); // Sort for consistency and easier testing - if deletions > 0 { - seen_objects.insert( - format!( - // Starting with a number sorts this first - "{deletions} deletion{}", - if deletions > 1 { "s" } else { "" } - ), - "normal", // Deletions don't interfere with `jj resolve` or diff display - ); - } - // TODO: We might decide it's OK for `jj resolve` to ignore special files in the - // `removes` of a conflict (see e.g. https://github.com/martinvonz/jj/pull/978). In - // that case, `conflict.removes` should be removed below. - for term in itertools::chain(conflict.removes(), conflict.adds()).flatten() { - seen_objects.insert( - match term { - TreeValue::File { - executable: false, .. - } => continue, - TreeValue::File { - executable: true, .. - } => "an executable", - TreeValue::Symlink(_) => "a symlink", - TreeValue::Tree(_) => "a directory", - TreeValue::GitSubmodule(_) => "a git submodule", - TreeValue::Conflict(_) => "another conflict (you found a bug!)", - } - .to_string(), - "difficult", - ); - } - - write!(formatter, "{formatted_path} ",)?; - formatter.with_label("conflict_description", |formatter| { - let print_pair = |formatter: &mut dyn Formatter, (text, label): &(String, &str)| { - write!(formatter.labeled(label), "{text}") - }; - print_pair( - formatter, - &( - format!("{sides}-sided"), - if sides > 2 { "difficult" } else { "normal" }, - ), - )?; - write!(formatter, " conflict")?; - - if !seen_objects.is_empty() { - write!(formatter, " including ")?; - let seen_objects = seen_objects.into_iter().collect_vec(); - match &seen_objects[..] { - [] => unreachable!(), - [only] => print_pair(formatter, only)?, - [first, middle @ .., last] => { - print_pair(formatter, first)?; - for pair in middle { - write!(formatter, ", ")?; - print_pair(formatter, pair)?; - } - write!(formatter, " and ")?; - print_pair(formatter, last)?; - } - }; + // Print conflicts that are still present after resolution if the workspace + // working copy is not at the commit. Otherwise, the conflicting paths will + // be printed by the `tx.finish()` instead. + if workspace_command.get_wc_commit_id() != Some(new_commit.id()) { + if let Some(mut formatter) = ui.status_formatter() { + let new_tree = new_commit.tree()?; + let new_conflicts = new_tree.conflicts().collect_vec(); + if !new_conflicts.is_empty() { + writeln!( + formatter, + "After this operation, some files at this revision still have conflicts:" + )?; + print_conflicted_paths(&new_conflicts, formatter.as_mut(), &workspace_command)?; } - Ok(()) - })?; - writeln!(formatter)?; + } } Ok(()) } diff --git a/cli/src/commands/status.rs b/cli/src/commands/status.rs index 982570843..5c190e6a6 100644 --- a/cli/src/commands/status.rs +++ b/cli/src/commands/status.rs @@ -18,8 +18,7 @@ use jj_lib::repo::Repo; use jj_lib::rewrite::merge_commit_trees; use tracing::instrument; -use super::resolve; -use crate::cli_util::CommandHelper; +use crate::cli_util::{print_conflicted_paths, CommandHelper}; use crate::command_error::CommandError; use crate::diff_util; use crate::ui::Ui; @@ -72,7 +71,7 @@ pub(crate) fn cmd_status( formatter.labeled("conflict"), "There are unresolved conflicts at these paths:" )?; - resolve::print_conflicted_paths(&conflicts, formatter, &workspace_command)? + print_conflicted_paths(&conflicts, formatter, &workspace_command)? } let template = workspace_command.commit_summary_template(); diff --git a/cli/tests/test_chmod_command.rs b/cli/tests/test_chmod_command.rs index 3da40b7bb..a43fa15fc 100644 --- a/cli/tests/test_chmod_command.rs +++ b/cli/tests/test_chmod_command.rs @@ -234,6 +234,8 @@ fn test_chmod_file_dir_deletion_conflicts() { Parent commit : zsuskuln c51c9c55 file | file Parent commit : royxmykx 6b18b3c1 deletion | deletion Added 0 files, modified 1 files, removed 0 files + There are unresolved conflicts at these paths: + file 2-sided conflict including 1 deletion and an executable "###); let stdout = test_env.jj_cmd_success(&repo_path, &["debug", "tree", "-r=file_deletion"]); insta::assert_snapshot!(stdout, diff --git a/cli/tests/test_diffedit_command.rs b/cli/tests/test_diffedit_command.rs index 4d3cc9694..b1ea6f1f9 100644 --- a/cli/tests/test_diffedit_command.rs +++ b/cli/tests/test_diffedit_command.rs @@ -384,6 +384,8 @@ fn test_diffedit_merge() { Working copy now at: yqosqzyt 1de824f2 (conflict) (empty) (no description set) Parent commit : royxmykx b90654a0 (conflict) merge Added 0 files, modified 0 files, removed 1 files + There are unresolved conflicts at these paths: + file2 2-sided conflict "###); let stdout = test_env.jj_cmd_success(&repo_path, &["diff", "-s", "-r", "@-"]); insta::assert_snapshot!(stdout, @r###" diff --git a/cli/tests/test_repo_change_report.rs b/cli/tests/test_repo_change_report.rs index 54a566e74..333b76932 100644 --- a/cli/tests/test_repo_change_report.rs +++ b/cli/tests/test_repo_change_report.rs @@ -43,6 +43,8 @@ fn test_report_conflicts() { Working copy now at: zsuskuln 7dc9bf15 (conflict) (empty) (no description set) Parent commit : kkmpptxz 9baab11e (conflict) C Added 0 files, modified 1 files, removed 0 files + There are unresolved conflicts at these paths: + file 2-sided conflict including 1 deletion "###); let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["rebase", "-d=description(A)"]); @@ -75,6 +77,8 @@ fn test_report_conflicts() { Working copy now at: zsuskuln 83074dac (conflict) (empty) (no description set) Parent commit : kkmpptxz 4f0eeaa6 (conflict) C Added 0 files, modified 1 files, removed 0 files + There are unresolved conflicts at these paths: + file 2-sided conflict "###); // Resolve one of the conflicts by (mostly) following the instructions @@ -84,6 +88,8 @@ fn test_report_conflicts() { Working copy now at: vruxwmqv 2ec0b4c3 (conflict) (empty) (no description set) Parent commit : rlvkpnrz e93270ab (conflict) B Added 0 files, modified 1 files, removed 0 files + There are unresolved conflicts at these paths: + file 2-sided conflict including 1 deletion "###); std::fs::write(repo_path.join("file"), "resolved\n").unwrap(); let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["squash"]); @@ -129,6 +135,8 @@ fn test_report_conflicts_with_divergent_commits() { Working copy now at: zsuskuln?? cdae4322 (conflict) C2 Parent commit : kkmpptxz b76d6a88 (conflict) B Added 0 files, modified 1 files, removed 0 files + There are unresolved conflicts at these paths: + file 2-sided conflict including 1 deletion "###); let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["rebase", "-d=description(A)"]); @@ -160,6 +168,8 @@ fn test_report_conflicts_with_divergent_commits() { Working copy now at: zsuskuln?? 33752e7e (conflict) C2 Parent commit : zzzzzzzz 00000000 (empty) (no description set) Added 0 files, modified 1 files, removed 0 files + There are unresolved conflicts at these paths: + file 2-sided conflict including 1 deletion "###); let (stdout, stderr) = diff --git a/cli/tests/test_resolve_command.rs b/cli/tests/test_resolve_command.rs index 133813810..94b01d728 100644 --- a/cli/tests/test_resolve_command.rs +++ b/cli/tests/test_resolve_command.rs @@ -233,7 +233,7 @@ fn test_resolution() { Parent commit : zsuskuln aa493daf a | a Parent commit : royxmykx db6a4daf b | b Added 0 files, modified 1 files, removed 0 files - After this operation, some files at this revision still have conflicts: + There are unresolved conflicts at these paths: file 2-sided conflict "###); insta::assert_snapshot!( @@ -704,7 +704,7 @@ fn test_multiple_conflicts() { Parent commit : zsuskuln de7553ef a | a Parent commit : royxmykx f68bc2f0 b | b Added 0 files, modified 1 files, removed 0 files - After this operation, some files at this revision still have conflicts: + There are unresolved conflicts at these paths: this_file_has_a_very_long_name_to_test_padding 2-sided conflict "###); insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff"]), diff --git a/cli/tests/test_restore_command.rs b/cli/tests/test_restore_command.rs index 0c8f1a2e3..6bc86a5b6 100644 --- a/cli/tests/test_restore_command.rs +++ b/cli/tests/test_restore_command.rs @@ -71,6 +71,8 @@ fn test_restore() { Working copy now at: kkmpptxz 761deaef (conflict) (no description set) Parent commit : rlvkpnrz e25100af (empty) (no description set) Added 0 files, modified 1 files, removed 0 files + There are unresolved conflicts at these paths: + file2 2-sided conflict including 1 deletion "###); let stdout = test_env.jj_cmd_success(&repo_path, &["diff", "-s", "-r=@-"]); insta::assert_snapshot!(stdout, @""); @@ -202,6 +204,8 @@ fn test_restore_conflicted_merge() { Parent commit : zsuskuln aa493daf a | a Parent commit : royxmykx db6a4daf b | b Added 0 files, modified 1 files, removed 0 files + There are unresolved conflicts at these paths: + file 2-sided conflict "###); insta::assert_snapshot!( std::fs::read_to_string(repo_path.join("file")).unwrap() @@ -241,6 +245,8 @@ fn test_restore_conflicted_merge() { Parent commit : zsuskuln aa493daf a | a Parent commit : royxmykx db6a4daf b | b Added 0 files, modified 1 files, removed 0 files + There are unresolved conflicts at these paths: + file 2-sided conflict "###); insta::assert_snapshot!( std::fs::read_to_string(repo_path.join("file")).unwrap()