From 2dae5f58dd921b6eb4590789c04f32a9e78fc1c5 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Sun, 19 Nov 2023 18:10:39 -0800 Subject: [PATCH] cli: print a message about new and resolved conflicts When e.g. `jj rebase` results in new conflicts, it's useful for the user to learn about that without having to run `jj log` right after. This patch adds reporting of new conflicts created by an operation. It also add reporting of conflicts that were resolved or abandoned by the operation. There was no measurable performance impact when rebasing a single commit in the Linux kernel repo. --- CHANGELOG.md | 2 + cli/src/cli_util.rs | 109 +++++++++++++++++++- cli/tests/test_chmod_command.rs | 2 + cli/tests/test_repo_change_report.rs | 147 +++++++++++++++++++++++++++ cli/tests/test_resolve_command.rs | 6 ++ cli/tests/test_restore_command.rs | 2 + 6 files changed, 263 insertions(+), 5 deletions(-) create mode 100644 cli/tests/test_repo_change_report.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index cb870e71e0..6b48d6c378 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### New features +* Information about new and resolved conflicts is now printed by every command. + ### Fixed bugs diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index 5c95fb78b0..221c90e0de 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -28,7 +28,7 @@ use std::{iter, str}; use clap::builder::{NonEmptyStringValueParser, TypedValueParser, ValueParserFactory}; use clap::{Arg, ArgAction, ArgMatches, Command, FromArgMatches}; -use indexmap::IndexSet; +use indexmap::{IndexMap, IndexSet}; use itertools::Itertools; use jj_lib::backend::{BackendError, ChangeId, CommitId, MergedTreeId, ObjectId}; use jj_lib::commit::Commit; @@ -52,8 +52,8 @@ use jj_lib::repo::{ use jj_lib::repo_path::{FsPathParseError, RepoPath, RepoPathBuf}; use jj_lib::revset::{ DefaultSymbolResolver, Revset, RevsetAliasesMap, RevsetCommitRef, RevsetEvaluationError, - RevsetExpression, RevsetIteratorExt, RevsetParseContext, RevsetParseError, - RevsetParseErrorKind, RevsetResolutionError, RevsetWorkspaceContext, + RevsetExpression, RevsetFilterPredicate, RevsetIteratorExt, RevsetParseContext, + RevsetParseError, RevsetParseErrorKind, RevsetResolutionError, RevsetWorkspaceContext, }; use jj_lib::rewrite::restore_tree; use jj_lib::settings::{ConfigResultExt as _, UserSettings}; @@ -1444,8 +1444,9 @@ See https://github.com/martinvonz/jj/blob/main/docs/working-copy.md#stale-workin writeln!(ui.stderr(), "Rebased {num_rebased} descendant commits")?; } - let maybe_old_wc_commit = tx - .base_repo() + let old_repo = tx.base_repo().clone(); + + let maybe_old_wc_commit = old_repo .view() .get_wc_commit_id(self.workspace_id()) .map(|commit_id| tx.base_repo().store().get_commit(commit_id)) @@ -1465,6 +1466,8 @@ See https://github.com/martinvonz/jj/blob/main/docs/working-copy.md#stale-workin print_failed_git_export(ui, &failed_branches)?; } self.user_repo = ReadonlyUserRepo::new(tx.commit()); + self.report_repo_changes(ui, &old_repo)?; + if self.may_update_working_copy { if let Some(new_commit) = &maybe_new_wc_commit { self.update_working_copy(ui, maybe_old_wc_commit.as_ref(), new_commit)?; @@ -1484,6 +1487,102 @@ See https://github.com/martinvonz/jj/blob/main/docs/working-copy.md#stale-workin } Ok(()) } + + /// Inform the user about important changes to the repo since the previous + /// operation (when `old_repo` was loaded). + fn report_repo_changes( + &self, + ui: &mut Ui, + old_repo: &Arc, + ) -> Result<(), CommandError> { + let old_view = old_repo.view(); + let new_repo = self.repo().as_ref(); + let new_view = new_repo.view(); + let added_heads = RevsetExpression::commits( + new_view + .heads() + .iter() + .filter(|id| !old_view.heads().contains(id)) + .cloned() + .collect(), + ); + let removed_heads = RevsetExpression::commits( + old_view + .heads() + .iter() + .filter(|id| !new_view.heads().contains(id)) + .cloned() + .collect(), + ); + // Filter the revsets by conflicts instead of reading all commits and doing the + // filtering here. That way, we can afford to evaluate the revset even if there + // are millions of commits added to the repo, assuming the revset engine can + // efficiently skip non-conflicting commits. Filter out empty commits mostly so + // `jj new ` doesn't result in a message about new conflicts. + let conflicts = RevsetExpression::filter(RevsetFilterPredicate::HasConflict) + .intersection(&RevsetExpression::filter(RevsetFilterPredicate::File(None))); + let removed_conflicts_expr = added_heads.range(&removed_heads).intersection(&conflicts); + let added_conflicts_expr = removed_heads.range(&added_heads).intersection(&conflicts); + + let get_commits = |expr: Rc| -> Result, CommandError> { + let commits = expr + .evaluate_programmatic(new_repo)? + .iter() + .commits(new_repo.store()) + .try_collect()?; + Ok(commits) + }; + let removed_conflict_commits = get_commits(removed_conflicts_expr)?; + let added_conflict_commits = get_commits(added_conflicts_expr)?; + + fn commits_by_change_id(commits: &[Commit]) -> IndexMap<&ChangeId, Vec<&Commit>> { + let mut result: IndexMap<&ChangeId, Vec<&Commit>> = IndexMap::new(); + for commit in commits { + result.entry(commit.change_id()).or_default().push(commit); + } + result + } + let removed_conflicts_by_change_id = commits_by_change_id(&removed_conflict_commits); + let added_conflicts_by_change_id = commits_by_change_id(&added_conflict_commits); + let mut resolved_conflicts_by_change_id = removed_conflicts_by_change_id.clone(); + resolved_conflicts_by_change_id + .retain(|change_id, _commits| !added_conflicts_by_change_id.contains_key(change_id)); + let mut new_conflicts_by_change_id = added_conflicts_by_change_id.clone(); + new_conflicts_by_change_id + .retain(|change_id, _commits| !removed_conflicts_by_change_id.contains_key(change_id)); + + // TODO: Also report new divergence and maybe resolved divergence + let mut fmt = ui.stderr_formatter(); + if !resolved_conflicts_by_change_id.is_empty() { + writeln!( + fmt, + "Existing conflicts were resolved or abandoned from these commits:" + )?; + for (_, old_commits) in &resolved_conflicts_by_change_id { + // TODO: Report which ones were resolved and which ones were abandoned. However, + // that involves resolving the change_id among the visible commits in the new + // repo, which isn't currently supported by Google's revset engine. + for commit in old_commits { + write!(fmt, " ")?; + self.write_commit_summary(fmt.as_mut(), commit)?; + writeln!(fmt)?; + } + } + } + if !new_conflicts_by_change_id.is_empty() { + writeln!(fmt, "New conflicts appeared in these commits:")?; + for (_, new_commits) in &new_conflicts_by_change_id { + for commit in new_commits { + write!(fmt, " ")?; + self.write_commit_summary(fmt.as_mut(), commit)?; + writeln!(fmt)?; + } + } + } + + // TODO: Hint about doing `jj new `. + Ok(()) + } } #[must_use] diff --git a/cli/tests/test_chmod_command.rs b/cli/tests/test_chmod_command.rs index 2d523142a8..64875b6e92 100644 --- a/cli/tests/test_chmod_command.rs +++ b/cli/tests/test_chmod_command.rs @@ -225,6 +225,8 @@ fn test_chmod_file_dir_deletion_conflicts() { test_env.jj_cmd_ok(&repo_path, &["chmod", "x", "file", "-r=file_deletion"]); insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(stderr, @r###" + New conflicts appeared in these commits: + kmkuslsw 4cc432b5 file_deletion | (conflict) file_deletion Working copy now at: kmkuslsw 4cc432b5 file_deletion | (conflict) file_deletion Parent commit : zsuskuln c51c9c55 file | file Parent commit : royxmykx 6b18b3c1 deletion | deletion diff --git a/cli/tests/test_repo_change_report.rs b/cli/tests/test_repo_change_report.rs new file mode 100644 index 0000000000..e6f7b32e3e --- /dev/null +++ b/cli/tests/test_repo_change_report.rs @@ -0,0 +1,147 @@ +// Copyright 2023 The Jujutsu Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use crate::common::TestEnvironment; + +pub mod common; + +#[test] +fn test_report_conflicts() { + let test_env = TestEnvironment::default(); + test_env.jj_cmd_ok(test_env.env_root(), &["init", "repo", "--git"]); + let repo_path = test_env.env_root().join("repo"); + + std::fs::write(repo_path.join("file"), "A\n").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["commit", "-m=A"]); + std::fs::write(repo_path.join("file"), "B\n").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["commit", "-m=B"]); + std::fs::write(repo_path.join("file"), "C\n").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["commit", "-m=C"]); + + let (stdout, stderr) = + test_env.jj_cmd_ok(&repo_path, &["rebase", "-s=description(B)", "-d=root()"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Rebased 3 commits + New conflicts appeared in these commits: + kkmpptxz a2593769 (conflict) C + rlvkpnrz 727244df (conflict) B + Working copy now at: zsuskuln 30928080 (conflict) (empty) (no description set) + Parent commit : kkmpptxz a2593769 (conflict) C + Added 0 files, modified 1 files, removed 0 files + "###); + + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["rebase", "-d=description(A)"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Rebased 3 commits + Existing conflicts were resolved or abandoned from these commits: + kkmpptxz hidden a2593769 (conflict) C + rlvkpnrz hidden 727244df (conflict) B + Working copy now at: zsuskuln 355a2e34 (empty) (no description set) + Parent commit : kkmpptxz ed071401 C + Added 0 files, modified 1 files, removed 0 files + "###); +} + +#[test] +fn test_report_conflicts_with_divergent_commits() { + let test_env = TestEnvironment::default(); + test_env.jj_cmd_ok(test_env.env_root(), &["init", "repo", "--git"]); + let repo_path = test_env.env_root().join("repo"); + + test_env.jj_cmd_ok(&repo_path, &["describe", "-m=A"]); + std::fs::write(repo_path.join("file"), "A\n").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["new", "-m=B"]); + std::fs::write(repo_path.join("file"), "B\n").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["new", "-m=C"]); + std::fs::write(repo_path.join("file"), "C\n").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["describe", "-m=C2"]); + test_env.jj_cmd_ok(&repo_path, &["describe", "-m=C3", "--at-op=@-"]); + + let (stdout, stderr) = + test_env.jj_cmd_ok(&repo_path, &["rebase", "-s=description(B)", "-d=root()"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Concurrent modification detected, resolving automatically. + Rebased 3 commits + New conflicts appeared in these commits: + zsuskuln?? 76c40a95 (conflict) C3 + zsuskuln?? e92329f2 (conflict) C2 + kkmpptxz aed319ec (conflict) B + Working copy now at: zsuskuln?? e92329f2 (conflict) C2 + Parent commit : kkmpptxz aed319ec (conflict) B + Added 0 files, modified 1 files, removed 0 files + "###); + + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["rebase", "-d=description(A)"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Rebased 3 commits + Existing conflicts were resolved or abandoned from these commits: + zsuskuln hidden 76c40a95 (conflict) C3 + zsuskuln hidden e92329f2 (conflict) C2 + kkmpptxz hidden aed319ec (conflict) B + Working copy now at: zsuskuln?? 9c33e9a9 C2 + Parent commit : kkmpptxz 9ce42c2a B + Added 0 files, modified 1 files, removed 0 files + "###); + + // Same thing when rebasing the divergent commits one at a time + let (stdout, stderr) = + test_env.jj_cmd_ok(&repo_path, &["rebase", "-s=description(C2)", "-d=root()"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Rebased 1 commits + New conflicts appeared in these commits: + zsuskuln?? 0d6cb6b7 (conflict) C2 + Working copy now at: zsuskuln?? 0d6cb6b7 (conflict) C2 + Parent commit : zzzzzzzz 00000000 (empty) (no description set) + Added 0 files, modified 1 files, removed 0 files + "###); + + let (stdout, stderr) = + test_env.jj_cmd_ok(&repo_path, &["rebase", "-s=description(C3)", "-d=root()"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Rebased 1 commits + New conflicts appeared in these commits: + zsuskuln?? 9652a362 (conflict) C3 + "###); + + let (stdout, stderr) = test_env.jj_cmd_ok( + &repo_path, + &["rebase", "-s=description(C2)", "-d=description(B)"], + ); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Rebased 1 commits + Existing conflicts were resolved or abandoned from these commits: + zsuskuln hidden 0d6cb6b7 (conflict) C2 + Working copy now at: zsuskuln?? 24f79296 C2 + Parent commit : kkmpptxz 9ce42c2a B + Added 0 files, modified 1 files, removed 0 files + "###); + + let (stdout, stderr) = test_env.jj_cmd_ok( + &repo_path, + &["rebase", "-s=description(C3)", "-d=description(B)"], + ); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Rebased 1 commits + Existing conflicts were resolved or abandoned from these commits: + zsuskuln hidden 9652a362 (conflict) C3 + "###); +} diff --git a/cli/tests/test_resolve_command.rs b/cli/tests/test_resolve_command.rs index 42c44f5a5d..069d7c035e 100644 --- a/cli/tests/test_resolve_command.rs +++ b/cli/tests/test_resolve_command.rs @@ -184,6 +184,8 @@ conflict ); insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(stderr, @r###" + New conflicts appeared in these commits: + vruxwmqv ff4e8c6b conflict | (conflict) conflict Working copy now at: vruxwmqv ff4e8c6b conflict | (conflict) conflict Parent commit : zsuskuln aa493daf a | a Parent commit : royxmykx db6a4daf b | b @@ -635,6 +637,8 @@ fn test_multiple_conflicts() { let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["resolve", "another_file"]); insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(stderr, @r###" + New conflicts appeared in these commits: + vruxwmqv c3c25bce conflict | (conflict) conflict Working copy now at: vruxwmqv c3c25bce conflict | (conflict) conflict Parent commit : zsuskuln de7553ef a | a Parent commit : royxmykx f68bc2f0 b | b @@ -664,6 +668,8 @@ fn test_multiple_conflicts() { let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["resolve", "--quiet", "another_file"]); insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(stderr, @r###" + New conflicts appeared in these commits: + vruxwmqv fd3874cd conflict | (conflict) conflict Working copy now at: vruxwmqv fd3874cd conflict | (conflict) conflict Parent commit : zsuskuln de7553ef a | a Parent commit : royxmykx f68bc2f0 b | b diff --git a/cli/tests/test_restore_command.rs b/cli/tests/test_restore_command.rs index aafff60697..335ca8213e 100644 --- a/cli/tests/test_restore_command.rs +++ b/cli/tests/test_restore_command.rs @@ -63,6 +63,8 @@ fn test_restore() { insta::assert_snapshot!(stderr, @r###" Created rlvkpnrz e25100af (empty) (no description set) Rebased 1 descendant commits + New conflicts appeared in these commits: + kkmpptxz e301deb3 (conflict) (no description set) Working copy now at: kkmpptxz e301deb3 (conflict) (no description set) Parent commit : rlvkpnrz e25100af (empty) (no description set) Added 0 files, modified 1 files, removed 0 files