Skip to content

Commit

Permalink
cli: log conflicted paths whenever the working copy is changed
Browse files Browse the repository at this point in the history 
This is disabled when the global `--quiet` flag is used.
  • Loading branch information
@bnjmnt4n
bnjmnt4n committed Mar 28, 2024
1 parent 24bcce5 commit 88f429c
Show file tree
Hide file tree
Showing 10 changed files with 133 additions and 130 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

* `jj branch list` now supports a `--conflicted/-c` option to show only conflicted branches.

* Commands which update the working copy now print the list of conflicted paths.

### Fixed bugs

## [0.15.1] - 2024-03-06
Expand Down
107 changes: 105 additions & 2 deletions cli/src/cli_util.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
// limitations under the License.

use core::fmt;
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;
Expand All @@ -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};
Expand Down Expand Up @@ -1160,6 +1161,17 @@ 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 !self.global_args.quiet && Some(new_commit) != maybe_old_commit {
let mut formatter = ui.stderr_formatter();
let conflicts = new_commit.tree()?.conflicts().collect_vec();
if !conflicts.is_empty() {
writeln!(
formatter.labeled("conflict"),
"There are unresolved conflicts at these paths:"
)?;
print_conflicted_paths(&conflicts, formatter.as_mut(), &self)?;
}
}
Ok(())
}

Expand Down Expand Up @@ -1588,6 +1600,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,
Expand Down
121 changes: 2 additions & 119 deletions cli/src/commands/resolve.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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, WorkspaceCommandHelper};
use crate::cli_util::{print_conflicted_paths, CommandHelper};
use crate::command_error::{cli_error, CommandError};
use crate::formatter::Formatter;
use crate::ui::Ui;

/// Resolve a conflicted file with an external merge tool
Expand All @@ -51,10 +46,6 @@ pub(crate) struct ResolveArgs {
// `diff --summary`, but should be more verbose.
#[arg(long, short)]
list: bool,
/// Do not print the list of remaining conflicts (if any) after resolving a
/// conflict
#[arg(long, short, conflicts_with = "list")]
quiet: bool,
/// Specify 3-way merge tool to be used
#[arg(long, conflicts_with = "list", value_name = "NAME")]
tool: Option<String>,
Expand Down Expand Up @@ -105,121 +96,13 @@ pub(crate) fn cmd_resolve(
)?;
let mut tx = workspace_command.start_transaction();
let new_tree_id = merge_editor.edit_file(&tree, repo_path)?;
let new_commit = tx
.mut_repo()
tx.mut_repo()
.rewrite_commit(command.settings(), &commit)
.set_tree_id(new_tree_id)
.write()?;
tx.finish(
ui,
format!("Resolve conflicts in commit {}", commit.id().hex()),
)?;

if !args.quiet {
let new_tree = new_commit.tree()?;
let new_conflicts = new_tree.conflicts().collect_vec();
if !new_conflicts.is_empty() {
writeln!(
ui.stderr(),
"After this operation, some files at this revision still have conflicts:"
)?;
print_conflicted_paths(
&new_conflicts,
ui.stderr_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)?;
}
};
}
Ok(())
})?;
writeln!(formatter)?;
}
Ok(())
}
5 changes: 2 additions & 3 deletions cli/src/commands/status.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand Down
4 changes: 0 additions & 4 deletions cli/tests/[email protected]
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1530,10 +1530,6 @@ Note that conflicts can also be resolved without using this command. You may edi
Possible values: `true`, `false`
* `-q`, `--quiet` — Do not print the list of remaining conflicts (if any) after resolving a conflict
Possible values: `true`, `false`
* `--tool <NAME>` — Specify 3-way merge tool to be used
Expand Down
2 changes: 2 additions & 0 deletions cli/tests/test_chmod_command.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 2 additions & 0 deletions cli/tests/test_diffedit_command.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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###"
Expand Down
10 changes: 10 additions & 0 deletions cli/tests/test_repo_change_report.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)"]);
Expand Down Expand Up @@ -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
Expand All @@ -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"]);
Expand Down Expand Up @@ -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)"]);
Expand Down Expand Up @@ -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) =
Expand Down
Loading

0 comments on commit 88f429c

Please sign in to comment.