From 8fac33afdb7ebd10d40b1196349d0cd8bea0da7d Mon Sep 17 00:00:00 2001 From: Jesse Somerville <jssomerville2@gmail.com> Date: Tue, 21 Nov 2023 18:54:47 -0800 Subject: [PATCH 01/13] install-and-setup.md: `--strategy` -> `--strategies` for `cargo binstall` --- docs/install-and-setup.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/install-and-setup.md b/docs/install-and-setup.md index 883f187a26..9374e911a4 100644 --- a/docs/install-and-setup.md +++ b/docs/install-and-setup.md @@ -19,11 +19,11 @@ can install the same binaries of the last `jj` release from GitHub as follows: ```shell # Will put the jj binary for the latest release in ~/.cargo/bin by default -cargo binstall --strategy crate-meta-data jj-cli +cargo binstall --strategies crate-meta-data jj-cli ``` -Without the `--strategy` option, you may get equivalent binaries that should be -compiled from the same source code. +Without the `--strategies` option, you may get equivalent binaries that should +be compiled from the same source code. ### Linux From 49170b7c759a153e526ce259b49d20b6336085b2 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Wed, 22 Nov 2023 15:17:49 +0000 Subject: [PATCH 02/13] cargo: bump the cargo-dependencies group with 1 update Bumps the cargo-dependencies group with 1 update: [config](https://github.com/mehcode/config-rs). - [Changelog](https://github.com/mehcode/config-rs/blob/v0.13.4/CHANGELOG.md) - [Commits](https://github.com/mehcode/config-rs/compare/0.13.3...v0.13.4) --- updated-dependencies: - dependency-name: config dependency-type: direct:production update-type: version-update:semver-patch dependency-group: cargo-dependencies ... Signed-off-by: dependabot[bot] <support@github.com> --- Cargo.lock | 4 ++-- Cargo.toml | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 66cb8a57cb..c4c533784a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -431,9 +431,9 @@ checksum = "acbf1af155f9b9ef647e42cdc158db4b64a1b61f743629225fde6f3e0be2a7c7" [[package]] name = "config" -version = "0.13.3" +version = "0.13.4" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d379af7f68bfc21714c6c7dea883544201741d2ce8274bb12fa54f89507f52a7" +checksum = "23738e11972c7643e4ec947840fc463b6a571afcd3e735bdfce7d03c7a784aca" dependencies = [ "async-trait", "lazy_static", diff --git a/Cargo.toml b/Cargo.toml index cd7b246c8e..871eb7a3b3 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -33,7 +33,7 @@ chrono = { version = "0.4.31", default-features = false, features = [ "std", "clock", ] } -config = { version = "0.13.2", default-features = false, features = ["toml"] } +config = { version = "0.13.4", default-features = false, features = ["toml"] } criterion = "0.5.1" crossterm = { version = "0.26", default-features = false } digest = "0.10.7" From 5642b437c56719ba6e824a37a4f21c2239b6222b Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Wed, 22 Nov 2023 15:58:51 +0000 Subject: [PATCH 03/13] github: bump the github-dependencies group with 1 update Bumps the github-dependencies group with 1 update: [DeterminateSystems/nix-installer-action](https://github.com/determinatesystems/nix-installer-action). - [Release notes](https://github.com/determinatesystems/nix-installer-action/releases) - [Commits](https://github.com/determinatesystems/nix-installer-action/compare/5620eb4af6b562c53e4d4628c0b6e4f9d9ae8612...07b8bcba1b22d847db7ee507180c33e115499665) --- updated-dependencies: - dependency-name: DeterminateSystems/nix-installer-action dependency-type: direct:production update-type: version-update:semver-major dependency-group: github-dependencies ... Signed-off-by: dependabot[bot] <support@github.com> --- .github/workflows/nix-linux.yml | 2 +- .github/workflows/nix-update-flake.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/nix-linux.yml b/.github/workflows/nix-linux.yml index 746930362f..de134d3125 100644 --- a/.github/workflows/nix-linux.yml +++ b/.github/workflows/nix-linux.yml @@ -17,6 +17,6 @@ jobs: - uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 with: fetch-depth: 0 - - uses: DeterminateSystems/nix-installer-action@5620eb4af6b562c53e4d4628c0b6e4f9d9ae8612 + - uses: DeterminateSystems/nix-installer-action@07b8bcba1b22d847db7ee507180c33e115499665 - uses: DeterminateSystems/magic-nix-cache-action@a04e6275a6bea232cd04fc6f3cbf20d4cb02a3e1 - run: nix build --print-build-logs --show-trace diff --git a/.github/workflows/nix-update-flake.yml b/.github/workflows/nix-update-flake.yml index 8cd5b1e1b0..756c7b5c11 100644 --- a/.github/workflows/nix-update-flake.yml +++ b/.github/workflows/nix-update-flake.yml @@ -11,7 +11,7 @@ jobs: - name: Checkout repository uses: actions/checkout@v4 - name: Install Nix - uses: DeterminateSystems/nix-installer-action@5620eb4af6b562c53e4d4628c0b6e4f9d9ae8612 + uses: DeterminateSystems/nix-installer-action@07b8bcba1b22d847db7ee507180c33e115499665 - name: Update flake.lock uses: DeterminateSystems/update-flake-lock@da2fd6f2563fe3e4f2af8be73b864088564e263d with: From 5568ca81c6e499d3704a0b6b5b0842ec9e1bdc05 Mon Sep 17 00:00:00 2001 From: Carlos Precioso <github@precioso.design> Date: Wed, 22 Nov 2023 16:10:40 +0100 Subject: [PATCH 04/13] docs: add mermaid plugin for diagrams --- mkdocs.yml | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/mkdocs.yml b/mkdocs.yml index 1c70d853c8..281ff9ca16 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -39,7 +39,11 @@ markdown_extensions: - mdx_breakless_lists - pymdownx.tabbed: alternate_style: true - - pymdownx.superfences + - pymdownx.superfences: + custom_fences: + - name: mermaid + class: mermaid + format: !!python/name:pymdownx.superfences.fence_code_format - pymdownx.details - pymdownx.snippets - pymdownx.emoji: From 31def4b13157ac2a125afb13a3bd873b35cbb4fa Mon Sep 17 00:00:00 2001 From: Yuya Nishihara <yuya@tcha.org> Date: Thu, 23 Nov 2023 09:26:46 +0900 Subject: [PATCH 05/13] cleanup: don't use debug format to print source errors --- cli/src/cli_util.rs | 8 ++++---- cli/src/merge_tools/builtin.rs | 2 +- cli/src/ui.rs | 4 ++-- lib/src/default_index_store.rs | 4 ++-- lib/src/working_copy.rs | 8 ++++---- 5 files changed, 13 insertions(+), 13 deletions(-) diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index 261fc430bd..155d6e5572 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -480,7 +480,7 @@ impl TracingSubscription { .from_env_lossy() }) .map_err(|err| { - CommandError::InternalError(format!("failed to enable verbose logging: {err:?}")) + CommandError::InternalError(format!("failed to enable verbose logging: {err}")) })?; tracing::info!("verbose logging enabled"); Ok(()) @@ -2215,14 +2215,14 @@ pub fn write_config_value_to_file( // If config doesn't exist yet, read as empty and we'll write one. std::io::ErrorKind::NotFound => Ok("".to_string()), _ => Err(user_error(format!( - "Failed to read file {path}: {err:?}", + "Failed to read file {path}: {err}", path = path.display() ))), } })?; let mut doc = toml_edit::Document::from_str(&config_toml).map_err(|err| { user_error(format!( - "Failed to parse file {path}: {err:?}", + "Failed to parse file {path}: {err}", path = path.display() )) })?; @@ -2264,7 +2264,7 @@ pub fn write_config_value_to_file( // Write config back std::fs::write(path, doc.to_string()).map_err(|err| { user_error(format!( - "Failed to write file {path}: {err:?}", + "Failed to write file {path}: {err}", path = path.display() )) }) diff --git a/cli/src/merge_tools/builtin.rs b/cli/src/merge_tools/builtin.rs index f78761a40e..ad0564623f 100644 --- a/cli/src/merge_tools/builtin.rs +++ b/cli/src/merge_tools/builtin.rs @@ -36,7 +36,7 @@ pub enum BuiltinToolError { }, #[error("Rendering {item} {id} is unimplemented for the builtin difftool/mergetool")] Unimplemented { item: &'static str, id: String }, - #[error("Backend error: {0:?}")] + #[error("Backend error: {0}")] BackendError(#[from] jj_lib::backend::BackendError), } diff --git a/cli/src/ui.rs b/cli/src/ui.rs index ab5744df0a..a94c125eff 100644 --- a/cli/src/ui.rs +++ b/cli/src/ui.rs @@ -170,13 +170,13 @@ pub enum PaginationChoice { fn pagination_setting(config: &config::Config) -> Result<PaginationChoice, CommandError> { config .get::<PaginationChoice>("ui.paginate") - .map_err(|err| CommandError::ConfigError(format!("Invalid `ui.paginate`: {err:?}"))) + .map_err(|err| CommandError::ConfigError(format!("Invalid `ui.paginate`: {err}"))) } fn pager_setting(config: &config::Config) -> Result<CommandNameAndArgs, CommandError> { config .get::<CommandNameAndArgs>("ui.pager") - .map_err(|err| CommandError::ConfigError(format!("Invalid `ui.pager`: {err:?}"))) + .map_err(|err| CommandError::ConfigError(format!("Invalid `ui.pager`: {err}"))) } impl Ui { diff --git a/lib/src/default_index_store.rs b/lib/src/default_index_store.rs index 2b6a4de205..4eed2fd676 100644 --- a/lib/src/default_index_store.rs +++ b/lib/src/default_index_store.rs @@ -251,12 +251,12 @@ impl IndexStore for DefaultIndexStore { .downcast::<MutableIndexImpl>() .expect("index to merge in must be a MutableIndexImpl"); let index = index.save_in(self.dir.clone()).map_err(|err| { - IndexWriteError::Other(format!("Failed to write commit index file: {err:?}")) + IndexWriteError::Other(format!("Failed to write commit index file: {err}")) })?; self.associate_file_with_operation(&index, op_id) .map_err(|err| { IndexWriteError::Other(format!( - "Failed to associate commit index file with a operation {op_id:?}: {err:?}" + "Failed to associate commit index file with a operation {op_id:?}: {err}" )) })?; Ok(Box::new(ReadonlyIndexWrapper(index))) diff --git a/lib/src/working_copy.rs b/lib/src/working_copy.rs index ae9cc637b1..56c74b96a7 100644 --- a/lib/src/working_copy.rs +++ b/lib/src/working_copy.rs @@ -141,7 +141,7 @@ pub enum SnapshotError { max_size: HumanByteSize, }, /// Some other error happened while snapshotting the working copy. - #[error("{message}: {err:?}")] + #[error("{message}: {err}")] Other { /// Error message. message: String, @@ -224,7 +224,7 @@ pub enum CheckoutError { #[error("Internal backend error: {0}")] InternalBackendError(#[from] BackendError), /// Some other error happened while checking out the working copy. - #[error("{message}: {err:?}")] + #[error("{message}: {err}")] Other { /// Error message. message: String, @@ -248,7 +248,7 @@ pub enum ResetError { #[error("Internal error: {0}")] InternalBackendError(#[from] BackendError), /// Some other error happened while checking out the working copy. - #[error("{message}: {err:?}")] + #[error("{message}: {err}")] Other { /// Error message. message: String, @@ -260,7 +260,7 @@ pub enum ResetError { /// An error while reading the working copy state. #[derive(Debug, Error)] -#[error("{message}: {err:?}")] +#[error("{message}: {err}")] pub struct WorkingCopyStateError { /// Error message. pub message: String, From 13c93d5270581e0867256c547c4770aafeb26615 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara <yuya@tcha.org> Date: Thu, 23 Nov 2023 09:10:13 +0900 Subject: [PATCH 06/13] cli: show executable name in error message, include underlying error details Since this is the error to spawn (or wait) process, command arguments aren't important. Let's make that clear by not showing full command string. #2614 --- cli/src/cli_util.rs | 11 ++++++----- cli/src/config.rs | 6 ++++++ cli/src/ui.rs | 4 ++-- 3 files changed, 14 insertions(+), 7 deletions(-) diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index 155d6e5572..d8d875e2ed 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -2294,11 +2294,12 @@ pub fn run_ui_editor(settings: &UserSettings, edit_path: &PathBuf) -> Result<(), .config() .get("ui.editor") .map_err(|err| CommandError::ConfigError(format!("ui.editor: {err}")))?; - let exit_status = editor - .to_command() - .arg(edit_path) - .status() - .map_err(|_| user_error(format!("Failed to run editor '{editor}'")))?; + let exit_status = editor.to_command().arg(edit_path).status().map_err(|err| { + user_error(format!( + "Failed to run editor '{name}': {err}", + name = editor.split_name(), + )) + })?; if !exit_status.success() { return Err(user_error(format!( "Editor '{editor}' exited with an error" diff --git a/cli/src/config.rs b/cli/src/config.rs index 2060605f99..453d7dabed 100644 --- a/cli/src/config.rs +++ b/cli/src/config.rs @@ -447,6 +447,12 @@ pub enum CommandNameAndArgs { } impl CommandNameAndArgs { + /// Returns command name without arguments. + pub fn split_name(&self) -> Cow<str> { + let (name, _) = self.split_name_and_args(); + name + } + /// Returns command name and arguments. /// /// The command name may be an empty string (as well as each argument.) diff --git a/cli/src/ui.rs b/cli/src/ui.rs index a94c125eff..0479aac4ca 100644 --- a/cli/src/ui.rs +++ b/cli/src/ui.rs @@ -224,8 +224,8 @@ impl Ui { Err(e) => { writeln!( self.warning(), - "Failed to spawn pager '{cmd}': {e}", - cmd = self.pager_cmd, + "Failed to spawn pager '{name}': {e}", + name = self.pager_cmd.split_name(), ) .ok(); } From a4f6e0de0b13562ae1061cf378245c88b5bfbcac Mon Sep 17 00:00:00 2001 From: Yuya Nishihara <yuya@tcha.org> Date: Sat, 18 Nov 2023 15:15:07 +0900 Subject: [PATCH 07/13] repo_path: extract helper that converts Path to RepoPath literally --- lib/src/repo_path.rs | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/lib/src/repo_path.rs b/lib/src/repo_path.rs index d3ae6addaa..bd0d8cf345 100644 --- a/lib/src/repo_path.rs +++ b/lib/src/repo_path.rs @@ -83,6 +83,22 @@ impl RepoPath { } } + /// Converts repo-relative `Path` to `RepoPath`. + /// + /// The input path should not contain `.` or `..`. + pub fn from_relative_path(relative_path: impl AsRef<Path>) -> Option<Self> { + let relative_path = relative_path.as_ref(); + let components = relative_path + .components() + .map(|c| match c { + Component::Normal(a) => Some(RepoPathComponent::from(a.to_str().unwrap())), + // TODO: better to return Err instead of None? + _ => None, + }) + .collect::<Option<_>>()?; + Some(RepoPath::from_components(components)) + } + pub fn from_components(components: Vec<RepoPathComponent>) -> Self { RepoPath { components } } @@ -103,14 +119,8 @@ impl RepoPath { if repo_relative_path == Path::new(".") { return Ok(RepoPath::root()); } - let components = repo_relative_path - .components() - .map(|c| match c { - Component::Normal(a) => Ok(RepoPathComponent::from(a.to_str().unwrap())), - _ => Err(FsPathParseError::InputNotInRepo(input.to_owned())), - }) - .try_collect()?; - Ok(RepoPath::from_components(components)) + Self::from_relative_path(repo_relative_path) + .ok_or_else(|| FsPathParseError::InputNotInRepo(input.to_owned())) } /// The full string form used internally, not for presenting to users (where From c16c89bc2758e680bed87d9ea716ae4e1a827b02 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara <yuya@tcha.org> Date: Sat, 18 Nov 2023 15:54:16 +0900 Subject: [PATCH 08/13] fsmonitor: keep paths relative to the workspace root Since the caller wants repo-relative paths, it doesn't make sense to convert them back and forth. --- lib/src/fsmonitor.rs | 10 ++++------ lib/src/local_working_copy.rs | 13 ++----------- lib/tests/test_local_working_copy.rs | 6 ++---- 3 files changed, 8 insertions(+), 21 deletions(-) diff --git a/lib/src/fsmonitor.rs b/lib/src/fsmonitor.rs index 75f91ab630..c75b392277 100644 --- a/lib/src/fsmonitor.rs +++ b/lib/src/fsmonitor.rs @@ -164,8 +164,9 @@ pub mod watchman { /// Query for changed files since the previous point in time. /// - /// The returned list of paths is absolute. If it is `None`, then the - /// caller must crawl the entire working copy themselves. + /// The returned list of paths is relative to the `working_copy_path`. + /// If it is `None`, then the caller must crawl the entire working copy + /// themselves. #[instrument(skip(self))] pub async fn query_changed_files( &self, @@ -205,10 +206,7 @@ pub mod watchman { let paths = files .unwrap_or_default() .into_iter() - .map(|file_info| { - let NameOnly { name } = file_info; - self.resolved_root.path().join(name.into_inner()) - }) + .map(|NameOnly { name }| name.into_inner()) .collect_vec(); Ok((clock, Some(paths))) } diff --git a/lib/src/local_working_copy.rs b/lib/src/local_working_copy.rs index c864000fe6..3d80850ce6 100644 --- a/lib/src/local_working_copy.rs +++ b/lib/src/local_working_copy.rs @@ -57,7 +57,7 @@ use crate::matchers::{ use crate::merge::{Merge, MergeBuilder, MergedTreeValue}; use crate::merged_tree::{MergedTree, MergedTreeBuilder}; use crate::op_store::{OperationId, WorkspaceId}; -use crate::repo_path::{FsPathParseError, RepoPath, RepoPathComponent, RepoPathJoin}; +use crate::repo_path::{RepoPath, RepoPathComponent, RepoPathJoin}; use crate::settings::HumanByteSize; use crate::store::Store; use crate::tree::Tree; @@ -866,16 +866,7 @@ impl TreeState { let repo_paths = trace_span!("processing fsmonitor paths").in_scope(|| { changed_files .into_iter() - .filter_map(|path| { - match RepoPath::parse_fs_path( - &self.working_copy_path, - &self.working_copy_path, - path, - ) { - Ok(repo_path) => Some(repo_path), - Err(FsPathParseError::InputNotInRepo(_)) => None, - } - }) + .filter_map(RepoPath::from_relative_path) .collect_vec() }); diff --git a/lib/tests/test_local_working_copy.rs b/lib/tests/test_local_working_copy.rs index 9e66e3caff..6bb9f3b62b 100644 --- a/lib/tests/test_local_working_copy.rs +++ b/lib/tests/test_local_working_copy.rs @@ -22,6 +22,7 @@ use std::io::Write; use std::os::unix::fs::PermissionsExt; #[cfg(unix)] use std::os::unix::net::UnixListener; +use std::path::Path; use std::sync::Arc; use itertools::Itertools; @@ -927,10 +928,7 @@ fn test_fsmonitor() { testutils::write_working_copy_file(&workspace_root, &gitignore_path, "to/ignored\n"); let snapshot = |locked_ws: &mut LockedWorkspace, paths: &[&RepoPath]| { - let fs_paths = paths - .iter() - .map(|p| p.to_fs_path(&workspace_root)) - .collect(); + let fs_paths = paths.iter().map(|p| p.to_fs_path(Path::new(""))).collect(); locked_ws .locked_wc() .snapshot(SnapshotOptions { From 767e94f5affa84b1439bdf8b7dc1776aea8486bc Mon Sep 17 00:00:00 2001 From: Yuya Nishihara <yuya@tcha.org> Date: Sun, 19 Nov 2023 09:57:28 +0900 Subject: [PATCH 09/13] fsmonitor: drop unneeded mut from make_fsmonitor_matcher() We only need &self.working_copy_path here. --- lib/src/local_working_copy.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/src/local_working_copy.rs b/lib/src/local_working_copy.rs index 3d80850ce6..55a66f7be7 100644 --- a/lib/src/local_working_copy.rs +++ b/lib/src/local_working_copy.rs @@ -836,7 +836,7 @@ impl TreeState { #[instrument(skip_all)] fn make_fsmonitor_matcher( - &mut self, + &self, fsmonitor_kind: Option<FsmonitorKind>, ) -> Result<FsmonitorMatcher, SnapshotError> { let (watchman_clock, changed_files) = match fsmonitor_kind { From 1ddcaa43b3646dcc76ae42033ba43a0c81c1075f Mon Sep 17 00:00:00 2001 From: Yuya Nishihara <yuya@tcha.org> Date: Sat, 18 Nov 2023 17:10:50 +0900 Subject: [PATCH 10/13] fsmonitor: don't apply prefix matching to paths obtained from watchman If I understand it, watchman returns changed files and directories, and a directory change doesn't mean we need to scan all files under the directory. --- lib/src/local_working_copy.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/src/local_working_copy.rs b/lib/src/local_working_copy.rs index 55a66f7be7..db181e7f23 100644 --- a/lib/src/local_working_copy.rs +++ b/lib/src/local_working_copy.rs @@ -52,7 +52,7 @@ use crate::fsmonitor::FsmonitorKind; use crate::gitignore::GitIgnoreFile; use crate::lock::FileLock; use crate::matchers::{ - DifferenceMatcher, EverythingMatcher, IntersectionMatcher, Matcher, PrefixMatcher, + DifferenceMatcher, EverythingMatcher, FilesMatcher, IntersectionMatcher, Matcher, PrefixMatcher, }; use crate::merge::{Merge, MergeBuilder, MergedTreeValue}; use crate::merged_tree::{MergedTree, MergedTreeBuilder}; @@ -870,7 +870,7 @@ impl TreeState { .collect_vec() }); - Some(Box::new(PrefixMatcher::new(&repo_paths))) + Some(Box::new(FilesMatcher::new(&repo_paths))) } }; Ok(FsmonitorMatcher { From 5c737bdeca80f64bca0d7d3389743960275bcda7 Mon Sep 17 00:00:00 2001 From: Matt Stark <msta@google.com> Date: Tue, 21 Nov 2023 16:48:53 +1100 Subject: [PATCH 11/13] lib: Create struct RebaseOptions --- lib/src/rewrite.rs | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs index bd97856afb..c68dd2329f 100644 --- a/lib/src/rewrite.rs +++ b/lib/src/rewrite.rs @@ -171,10 +171,33 @@ pub fn back_out_commit( .write()?) } +#[derive(Clone, Default, PartialEq)] +pub enum EmptyBehaviour { + /// Always keep empty commits + #[default] + Keep, + /// Skips commits that would be empty after the rebase, but that were not + /// originally empty. + /// Will never skip merge commits with multiple non-empty parents. + AbandonNewlyEmpty, + /// Skips all empty commits, including ones that were empty before the + /// rebase. + /// Will never skip merge commits with multiple non-empty parents. + AbandonAllEmpty, +} + +/// Controls the configuration of a rebase. +// If we wanted to add a flag similar to `git rebase --ignore-date`, then this +// makes it much easier by ensuring that the only changes required are to +// change the RebaseOptions construction in the CLI, and changing the +// rebase_commit function to actually use the flag, and ensure we don't need to +// plumb it in. +#[derive(Clone, Default)] +pub struct RebaseOptions { + pub empty: EmptyBehaviour, +} + /// Rebases descendants of a commit onto a new commit (or several). -// TODO: Should there be an option to drop empty commits (and/or an option to -// drop empty commits only if they weren't already empty)? Or maybe that -// shouldn't be this type's job. pub struct DescendantRebaser<'settings, 'repo> { settings: &'settings UserSettings, mut_repo: &'repo mut MutableRepo, From 34a34c5211c47422a43c7e61b9653afd3d4843af Mon Sep 17 00:00:00 2001 From: Matt Stark <msta@google.com> Date: Tue, 21 Nov 2023 16:48:53 +1100 Subject: [PATCH 12/13] lib: Implement skipping of empty commits --- cli/src/commands/rebase.rs | 5 +- lib/src/repo.rs | 19 ++++- lib/src/rewrite.rs | 89 +++++++++++++++++--- lib/tests/test_rewrite.rs | 161 ++++++++++++++++++++++++++++++++++++- 4 files changed, 257 insertions(+), 17 deletions(-) diff --git a/cli/src/commands/rebase.rs b/cli/src/commands/rebase.rs index 6cfbfe8beb..e0613b26e8 100644 --- a/cli/src/commands/rebase.rs +++ b/cli/src/commands/rebase.rs @@ -334,7 +334,10 @@ fn rebase_revision( ); } // Now, rebase the descendants of the children. - rebased_commit_ids.extend(tx.mut_repo().rebase_descendants_return_map(settings)?); + rebased_commit_ids.extend( + tx.mut_repo() + .rebase_descendants_return_map(settings, Default::default())?, + ); let num_rebased_descendants = rebased_commit_ids.len(); // We now update `new_parents` to account for the rebase of all of diff --git a/lib/src/repo.rs b/lib/src/repo.rs index 5a14378e3d..1c15f05d29 100644 --- a/lib/src/repo.rs +++ b/lib/src/repo.rs @@ -50,7 +50,7 @@ use crate::refs::{ diff_named_ref_targets, diff_named_remote_refs, merge_ref_targets, merge_remote_refs, }; use crate::revset::{self, ChangeIdIndex, Revset, RevsetExpression}; -use crate::rewrite::DescendantRebaser; +use crate::rewrite::{DescendantRebaser, RebaseOptions}; use crate::settings::{RepoSettings, UserSettings}; use crate::simple_op_heads_store::SimpleOpHeadsStore; use crate::simple_op_store::SimpleOpStore; @@ -855,28 +855,39 @@ impl MutableRepo { fn rebase_descendants_return_rebaser<'settings, 'repo>( &'repo mut self, settings: &'settings UserSettings, + options: RebaseOptions, ) -> Result<Option<DescendantRebaser<'settings, 'repo>>, TreeMergeError> { if !self.has_rewrites() { // Optimization return Ok(None); } let mut rebaser = self.create_descendant_rebaser(settings); + *rebaser.mut_options() = options; rebaser.rebase_all()?; Ok(Some(rebaser)) } - pub fn rebase_descendants(&mut self, settings: &UserSettings) -> Result<usize, TreeMergeError> { + pub fn rebase_descendants_with_options( + &mut self, + settings: &UserSettings, + options: RebaseOptions, + ) -> Result<usize, TreeMergeError> { Ok(self - .rebase_descendants_return_rebaser(settings)? + .rebase_descendants_return_rebaser(settings, options)? .map_or(0, |rebaser| rebaser.rebased().len())) } + pub fn rebase_descendants(&mut self, settings: &UserSettings) -> Result<usize, TreeMergeError> { + self.rebase_descendants_with_options(settings, Default::default()) + } + pub fn rebase_descendants_return_map( &mut self, settings: &UserSettings, + options: RebaseOptions, ) -> Result<HashMap<CommitId, CommitId>, TreeMergeError> { Ok(self - .rebase_descendants_return_rebaser(settings)? + .rebase_descendants_return_rebaser(settings, options)? .map_or(HashMap::new(), |rebaser| rebaser.rebased().clone())) } diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs index c68dd2329f..bf6538dd2a 100644 --- a/lib/src/rewrite.rs +++ b/lib/src/rewrite.rs @@ -103,6 +103,22 @@ pub fn rebase_commit( mut_repo: &mut MutableRepo, old_commit: &Commit, new_parents: &[Commit], +) -> Result<Commit, TreeMergeError> { + rebase_commit_with_options( + settings, + mut_repo, + old_commit, + new_parents, + &Default::default(), + ) +} + +pub fn rebase_commit_with_options( + settings: &UserSettings, + mut_repo: &mut MutableRepo, + old_commit: &Commit, + new_parents: &[Commit], + options: &RebaseOptions, ) -> Result<Commit, TreeMergeError> { let old_parents = old_commit.parents(); let old_parent_trees = old_parents @@ -113,16 +129,44 @@ pub fn rebase_commit( .iter() .map(|parent| parent.store_commit().root_tree.clone()) .collect_vec(); - let new_tree_id = if new_parent_trees == old_parent_trees { - // Optimization - old_commit.tree_id().clone() + + let (old_base_tree_id, new_tree_id) = if new_parent_trees == old_parent_trees { + ( + // Optimization: old_base_tree_id is only used for newly empty, but when the parents + // haven't changed it can't be newly empty. + None, + // Optimization: Skip merging. + old_commit.tree_id().clone(), + ) } else { let old_base_tree = merge_commit_trees(mut_repo, &old_parents)?; let new_base_tree = merge_commit_trees(mut_repo, new_parents)?; let old_tree = old_commit.tree()?; - let merged_tree = new_base_tree.merge(&old_base_tree, &old_tree)?; - merged_tree.id() + ( + Some(old_base_tree.id()), + new_base_tree.merge(&old_base_tree, &old_tree)?.id(), + ) }; + // Ensure we don't abandon commits with multiple parents (merge commits), even + // if they're empty. + if let [parent] = new_parents { + match options.empty { + EmptyBehaviour::AbandonNewlyEmpty | EmptyBehaviour::AbandonAllEmpty => { + if *parent.tree_id() == new_tree_id + && (options.empty == EmptyBehaviour::AbandonAllEmpty + || old_base_tree_id != Some(old_commit.tree_id().clone())) + { + mut_repo.record_abandoned_commit(old_commit.id().clone()); + // Record old_commit as being succeeded by the parent for the purposes of + // the rebase. + // This ensures that when we stack commits, the second commit knows to + // rebase on top of the parent commit, rather than the abandoned commit. + return Ok(parent.clone()); + } + } + EmptyBehaviour::Keep => {} + } + } let new_parent_ids = new_parents .iter() .map(|commit| commit.id().clone()) @@ -221,6 +265,9 @@ pub struct DescendantRebaser<'settings, 'repo> { // have been rebased. heads_to_add: HashSet<CommitId>, heads_to_remove: Vec<CommitId>, + + // Options to apply during a rebase. + options: RebaseOptions, } impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { @@ -322,9 +369,15 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { branches, heads_to_add, heads_to_remove: Default::default(), + options: Default::default(), } } + /// Returns options that can be set. + pub fn mut_options(&mut self) -> &mut RebaseOptions { + &mut self.options + } + /// Returns a map from `CommitId` of old commit to new commit. Includes the /// commits rebase so far. Does not include the inputs passed to /// `rebase_descendants`. @@ -333,21 +386,30 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { } fn new_parents(&self, old_ids: &[CommitId]) -> Vec<CommitId> { + // This should be a set, but performance of a vec is much better since we expect + // 99% of commits to have <= 2 parents. let mut new_ids = vec![]; + let mut add_parent = |id: &CommitId| { + // This can trigger if we abandon an empty commit, as both the empty commit and + // its parent are succeeded by the same commit. + if !new_ids.contains(id) { + new_ids.push(id.clone()); + } + }; for old_id in old_ids { if let Some(new_parent_ids) = self.new_parents.get(old_id) { for new_parent_id in new_parent_ids { // The new parent may itself have been rebased earlier in the process if let Some(newer_parent_id) = self.rebased.get(new_parent_id) { - new_ids.push(newer_parent_id.clone()); + add_parent(newer_parent_id); } else { - new_ids.push(new_parent_id.clone()); + add_parent(new_parent_id); } } } else if let Some(new_parent_id) = self.rebased.get(old_id) { - new_ids.push(new_parent_id.clone()); + add_parent(new_parent_id); } else { - new_ids.push(old_id.clone()); + add_parent(old_id); }; } new_ids @@ -477,8 +539,13 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { .filter(|new_parent| head_set.contains(new_parent)) .map(|new_parent_id| self.mut_repo.store().get_commit(new_parent_id)) .try_collect()?; - let new_commit = - rebase_commit(self.settings, self.mut_repo, &old_commit, &new_parents)?; + let new_commit = rebase_commit_with_options( + self.settings, + self.mut_repo, + &old_commit, + &new_parents, + &self.options, + )?; self.rebased .insert(old_commit_id.clone(), new_commit.id().clone()); self.update_references(old_commit_id, vec![new_commit.id().clone()], true)?; diff --git a/lib/tests/test_rewrite.rs b/lib/tests/test_rewrite.rs index de5f445808..c0ec951d1d 100644 --- a/lib/tests/test_rewrite.rs +++ b/lib/tests/test_rewrite.rs @@ -13,12 +13,17 @@ // limitations under the License. use itertools::Itertools as _; +use jj_lib::commit::Commit; use jj_lib::matchers::{EverythingMatcher, FilesMatcher}; +use jj_lib::merged_tree::MergedTree; use jj_lib::op_store::{RefTarget, RemoteRef, RemoteRefState, WorkspaceId}; use jj_lib::repo::Repo; use jj_lib::repo_path::RepoPath; -use jj_lib::rewrite::{restore_tree, DescendantRebaser}; +use jj_lib::rewrite::{ + restore_tree, DescendantRebaser, EmptyBehaviour, RebaseOptions, RebasedDescendant, +}; use maplit::{hashmap, hashset}; +use test_case::test_case; use testutils::{ assert_rebased, create_random_commit, create_tree, write_random_commit, CommitGraphBuilder, TestRepo, @@ -1480,3 +1485,157 @@ fn test_rebase_descendants_update_checkout_abandoned_merge() { let checkout = repo.store().get_commit(new_checkout_id).unwrap(); assert_eq!(checkout.parent_ids(), vec![commit_b.id().clone()]); } + +fn assert_rebase_skipped( + rebased: Option<RebasedDescendant>, + expected_old_commit: &Commit, + expected_new_commit: &Commit, +) { + if let Some(RebasedDescendant { + old_commit, + new_commit, + }) = rebased + { + assert_eq!(old_commit, *expected_old_commit,); + assert_eq!(new_commit, *expected_new_commit); + // Since it was abandoned, the change ID should be different. + assert_ne!(old_commit.change_id(), new_commit.change_id()); + } else { + panic!("expected rebased commit: {rebased:?}"); + } +} + +#[test_case(EmptyBehaviour::Keep; "keep all commits")] +#[test_case(EmptyBehaviour::AbandonNewlyEmpty; "abandon newly empty commits")] +#[test_case(EmptyBehaviour::AbandonAllEmpty ; "abandon all empty commits")] +fn test_empty_commit_option(empty: EmptyBehaviour) { + let settings = testutils::user_settings(); + let test_repo = TestRepo::init(); + let repo = &test_repo.repo; + + // Rebase a previously empty commit, a newly empty commit, and a commit with + // actual changes. + // + // BD (commit B joined with commit D) + // | G + // | | + // | F (clean merge) + // | /|\ + // | C D E (empty) + // | \|/ + // | B + // A__/ + let mut tx = repo.start_transaction(&settings, "test"); + let mut_repo = tx.mut_repo(); + let create_fixed_tree = |paths: &[&str]| { + let content_map = paths + .iter() + .map(|&p| (RepoPath::from_internal_string(p), p)) + .collect_vec(); + let content_map_ref = content_map + .iter() + .map(|(path, content)| (path, *content)) + .collect_vec(); + create_tree(repo, &content_map_ref) + }; + + // The commit_with_parents function generates non-empty merge commits, so it + // isn't suitable for this test case. + let tree_b = create_fixed_tree(&["B"]); + let tree_c = create_fixed_tree(&["B", "C"]); + let tree_d = create_fixed_tree(&["B", "D"]); + let tree_f = create_fixed_tree(&["B", "C", "D"]); + let tree_g = create_fixed_tree(&["B", "C", "D", "G"]); + + let commit_a = create_random_commit(mut_repo, &settings).write().unwrap(); + + let mut create_commit = |parents: &[&Commit], tree: &MergedTree| { + create_random_commit(mut_repo, &settings) + .set_parents( + parents + .iter() + .map(|commit| commit.id().clone()) + .collect_vec(), + ) + .set_tree_id(tree.id()) + .write() + .unwrap() + }; + let commit_b = create_commit(&[&commit_a], &tree_b); + let commit_c = create_commit(&[&commit_b], &tree_c); + let commit_d = create_commit(&[&commit_b], &tree_d); + let commit_e = create_commit(&[&commit_b], &tree_b); + let commit_f = create_commit(&[&commit_c, &commit_d, &commit_e], &tree_f); + let commit_g = create_commit(&[&commit_f], &tree_g); + let commit_bd = create_commit(&[&commit_a], &tree_d); + + let mut rebaser = DescendantRebaser::new( + &settings, + tx.mut_repo(), + hashmap! { + commit_b.id().clone() => hashset!{commit_bd.id().clone()} + }, + hashset! {}, + ); + *rebaser.mut_options() = RebaseOptions { + empty: empty.clone(), + }; + + let new_head = match empty { + EmptyBehaviour::Keep => { + // The commit C isn't empty. + let new_commit_c = + assert_rebased(rebaser.rebase_next().unwrap(), &commit_c, &[&commit_bd]); + let new_commit_d = + assert_rebased(rebaser.rebase_next().unwrap(), &commit_d, &[&commit_bd]); + let new_commit_e = + assert_rebased(rebaser.rebase_next().unwrap(), &commit_e, &[&commit_bd]); + let new_commit_f = assert_rebased( + rebaser.rebase_next().unwrap(), + &commit_f, + &[&new_commit_c, &new_commit_d, &new_commit_e], + ); + assert_rebased(rebaser.rebase_next().unwrap(), &commit_g, &[&new_commit_f]) + } + EmptyBehaviour::AbandonAllEmpty => { + // The commit C isn't empty. + let new_commit_c = + assert_rebased(rebaser.rebase_next().unwrap(), &commit_c, &[&commit_bd]); + // D and E are empty, and F is a clean merge with only one child. Thus, F is + // also considered empty. + assert_rebase_skipped(rebaser.rebase_next().unwrap(), &commit_d, &commit_bd); + assert_rebase_skipped(rebaser.rebase_next().unwrap(), &commit_e, &commit_bd); + assert_rebase_skipped(rebaser.rebase_next().unwrap(), &commit_f, &new_commit_c); + assert_rebased(rebaser.rebase_next().unwrap(), &commit_g, &[&new_commit_c]) + } + EmptyBehaviour::AbandonNewlyEmpty => { + // The commit C isn't empty. + let new_commit_c = + assert_rebased(rebaser.rebase_next().unwrap(), &commit_c, &[&commit_bd]); + + // The changes in D are included in BD, so D is newly empty. + assert_rebase_skipped(rebaser.rebase_next().unwrap(), &commit_d, &commit_bd); + // E was already empty, so F is a merge commit with C and E as parents. + // Although it's empty, we still keep it because we don't want to drop merge + // commits. + let new_commit_e = + assert_rebased(rebaser.rebase_next().unwrap(), &commit_e, &[&commit_bd]); + let new_commit_f = assert_rebased( + rebaser.rebase_next().unwrap(), + &commit_f, + &[&new_commit_c, &new_commit_e], + ); + assert_rebased(rebaser.rebase_next().unwrap(), &commit_g, &[&new_commit_f]) + } + }; + + assert!(rebaser.rebase_next().unwrap().is_none()); + assert_eq!(rebaser.rebased().len(), 5); + + assert_eq!( + *tx.mut_repo().view().heads(), + hashset! { + new_head.id().clone(), + } + ); +} From 85b831049d6e0400cfba88e7cdb6bad89550c753 Mon Sep 17 00:00:00 2001 From: Matt Stark <msta@google.com> Date: Tue, 21 Nov 2023 16:49:31 +1100 Subject: [PATCH 13/13] cli: Add a --skip-empty flag to rebase --- CHANGELOG.md | 3 ++ cli/src/commands/rebase.rs | 71 ++++++++++++++++++++++++++++++++------ 2 files changed, 64 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 90379bef6b..c3cba8ab47 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 usual behavior where commits that became unreachable in the Git repo are abandoned ([#2504](https://github.com/martinvonz/jj/pull/2504)). +* `jj rebase` now takes the flag `--skip-empty`, which doesn't copy over commits + that would become empty after a rebase. + ### Fixed bugs diff --git a/cli/src/commands/rebase.rs b/cli/src/commands/rebase.rs index e0613b26e8..8d36808355 100644 --- a/cli/src/commands/rebase.rs +++ b/cli/src/commands/rebase.rs @@ -22,7 +22,7 @@ use jj_lib::backend::{CommitId, ObjectId}; use jj_lib::commit::Commit; use jj_lib::repo::{ReadonlyRepo, Repo}; use jj_lib::revset::{RevsetExpression, RevsetIteratorExt}; -use jj_lib::rewrite::rebase_commit; +use jj_lib::rewrite::{rebase_commit_with_options, EmptyBehaviour, RebaseOptions}; use jj_lib::settings::UserSettings; use tracing::instrument; @@ -143,6 +143,13 @@ pub(crate) struct RebaseArgs { /// commit) #[arg(long, short, required = true)] destination: Vec<RevisionArg>, + + /// If true, when rebasing would produce an empty commit, the commit is + /// skipped. + /// Will never skip merge commits with multiple non-empty parents. + #[arg(long)] + skip_empty: bool, + /// Deprecated. Please prefix the revset with `all:` instead. #[arg(long, short = 'L', hide = true)] allow_large_revsets: bool, @@ -160,6 +167,13 @@ pub(crate) fn cmd_rebase( Please use `jj rebase -d 'all:x|y'` instead of `jj rebase --allow-large-revsets -d x -d y`.", )); } + + let rebase_options = RebaseOptions { + empty: match args.skip_empty { + true => EmptyBehaviour::AbandonAllEmpty, + false => EmptyBehaviour::Keep, + }, + }; let mut workspace_command = command.workspace_helper(ui)?; let new_parents = cli_util::resolve_all_revs(&workspace_command, ui, &args.destination)? .into_iter() @@ -171,6 +185,7 @@ Please use `jj rebase -d 'all:x|y'` instead of `jj rebase --allow-large-revsets &mut workspace_command, &new_parents, rev_str, + &rebase_options, )?; } else if !args.source.is_empty() { let source_commits = @@ -181,6 +196,7 @@ Please use `jj rebase -d 'all:x|y'` instead of `jj rebase --allow-large-revsets &mut workspace_command, &new_parents, &source_commits, + rebase_options, )?; } else { let branch_commits = if args.branch.is_empty() { @@ -194,6 +210,7 @@ Please use `jj rebase -d 'all:x|y'` instead of `jj rebase --allow-large-revsets &mut workspace_command, &new_parents, &branch_commits, + rebase_options, )?; } Ok(()) @@ -205,6 +222,7 @@ fn rebase_branch( workspace_command: &mut WorkspaceCommandHelper, new_parents: &[Commit], branch_commits: &IndexSet<Commit>, + rebase_options: RebaseOptions, ) -> Result<(), CommandError> { let parent_ids = new_parents .iter() @@ -225,7 +243,14 @@ fn rebase_branch( .iter() .commits(workspace_command.repo().store()) .try_collect()?; - rebase_descendants(ui, settings, workspace_command, new_parents, &root_commits) + rebase_descendants( + ui, + settings, + workspace_command, + new_parents, + &root_commits, + rebase_options, + ) } fn rebase_descendants( @@ -234,6 +259,7 @@ fn rebase_descendants( workspace_command: &mut WorkspaceCommandHelper, new_parents: &[Commit], old_commits: &IndexSet<Commit>, + rebase_options: RebaseOptions, ) -> Result<(), CommandError> { workspace_command.check_rewritable(old_commits)?; for old_commit in old_commits.iter() { @@ -251,9 +277,17 @@ fn rebase_descendants( // `rebase_descendants` takes care of sorting in reverse topological order, so // no need to do it here. for old_commit in old_commits { - rebase_commit(settings, tx.mut_repo(), old_commit, new_parents)?; + rebase_commit_with_options( + settings, + tx.mut_repo(), + old_commit, + new_parents, + &rebase_options, + )?; } - let num_rebased = old_commits.len() + tx.mut_repo().rebase_descendants(settings)?; + let num_rebased = old_commits.len() + + tx.mut_repo() + .rebase_descendants_with_options(settings, rebase_options)?; writeln!(ui.stderr(), "Rebased {num_rebased} commits")?; tx.finish(ui)?; Ok(()) @@ -265,6 +299,7 @@ fn rebase_revision( workspace_command: &mut WorkspaceCommandHelper, new_parents: &[Commit], rev_str: &str, + rebase_options: &RebaseOptions, ) -> Result<(), CommandError> { let old_commit = workspace_command.resolve_single_rev(rev_str, ui)?; workspace_command.check_rewritable([&old_commit])?; @@ -328,15 +363,21 @@ fn rebase_revision( rebased_commit_ids.insert( child_commit.id().clone(), - rebase_commit(settings, tx.mut_repo(), child_commit, &new_child_parents)? - .id() - .clone(), + rebase_commit_with_options( + settings, + tx.mut_repo(), + child_commit, + &new_child_parents, + rebase_options, + )? + .id() + .clone(), ); } // Now, rebase the descendants of the children. rebased_commit_ids.extend( tx.mut_repo() - .rebase_descendants_return_map(settings, Default::default())?, + .rebase_descendants_return_map(settings, rebase_options.clone())?, ); let num_rebased_descendants = rebased_commit_ids.len(); @@ -363,8 +404,18 @@ fn rebase_revision( // Finally, it's safe to rebase `old_commit`. At this point, it should no longer // have any children; they have all been rebased and the originals have been // abandoned. - rebase_commit(settings, tx.mut_repo(), &old_commit, &new_parents)?; - debug_assert_eq!(tx.mut_repo().rebase_descendants(settings)?, 0); + rebase_commit_with_options( + settings, + tx.mut_repo(), + &old_commit, + &new_parents, + rebase_options, + )?; + debug_assert_eq!( + tx.mut_repo() + .rebase_descendants_with_options(settings, rebase_options.clone())?, + 0 + ); if num_rebased_descendants > 0 { writeln!(