From 006171d8555c88851b691a8d924d229bb26f44d0 Mon Sep 17 00:00:00 2001 From: Evan Mesterhazy Date: Fri, 16 Feb 2024 14:01:34 -0500 Subject: [PATCH] Implement advance-branches for jj commit ## Feature Description If enabled in the user or repository settings, the local branches pointing to the parents of the revision targeted by `jj commit` will be advanced to the newly created commit. Support for `jj new` will be added in a future change. This behavior can be enabled by default for all branches by setting the following in the config.toml: ``` [experimental-advance-branches] enabled-branches = ["glob:*"] ``` Specific branches can also be disabled: ``` [experimental-advance-branches] enabled-branches = ["glob:*"] disabled-branches = ["main"] ``` Branches that match a disabled pattern will not be advanced, even if they also match an enabled pattern. This implements feature request #2338. --- cli/src/cli_util.rs | 128 +++++++++++++++- cli/src/commands/commit.rs | 5 + cli/src/config-schema.json | 20 +++ cli/tests/runner.rs | 1 + cli/tests/test_advance_branches.rs | 236 +++++++++++++++++++++++++++++ lib/src/view.rs | 10 ++ 6 files changed, 399 insertions(+), 1 deletion(-) create mode 100644 cli/tests/test_advance_branches.rs diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index 1c37c99fbb..0a5a2ffa86 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -43,7 +43,7 @@ use jj_lib::id_prefix::IdPrefixContext; use jj_lib::matchers::{EverythingMatcher, Matcher, PrefixMatcher}; use jj_lib::merged_tree::MergedTree; use jj_lib::object_id::ObjectId; -use jj_lib::op_store::{OpStoreError, OperationId, WorkspaceId}; +use jj_lib::op_store::{OpStoreError, OperationId, RefTarget, WorkspaceId}; use jj_lib::op_walk::OpsetEvaluationError; use jj_lib::operation::Operation; use jj_lib::repo::{ @@ -389,6 +389,77 @@ impl ReadonlyUserRepo { } } +/// A branch that should be advanced to satisfy the "advance-branches" feature. +/// This is a helper for `WorkspaceCommandTransaction`. It provides a type-safe +/// way to separate the work of checking whether a branch can be advanced and +/// actually advancing it. Advancing the branch never fails, but can't be done +/// until the new `CommitId` is available. Splitting the work in this way also +/// allows us to identify eligible branches without actually moving them and +/// return config errors to the user early. +pub struct AdvanceableBranch { + name: String, + old_commit_id: CommitId, +} + +/// Helper for parsing and evaluating settings for the advance-branches feature. +/// Settings are configured in the jj config.toml as lists of [`StringPattern`]s +/// for enabled and disabled branches. Example: +/// ```toml +/// [experimental-advance-branches] +/// # Enable the feature for all branches except "main". +/// enabled-branches = ["glob:*"] +/// disabled-branches = ["main"] +/// ``` +struct AdvanceBranchesSettings { + enabled_branches: Vec, + disabled_branches: Vec, +} + +impl AdvanceBranchesSettings { + fn from_config(config: &config::Config) -> Result { + let get_setting = |setting_key| { + let setting = format!("experimental-advance-branches.{setting_key}"); + match config.get::>(&setting).optional()? { + Some(patterns) => patterns + .into_iter() + .map(|s| { + StringPattern::parse(&s).map_err(|e| { + config_error_with_message( + format!("Error parsing '{s}' for {setting}"), + e, + ) + }) + }) + .collect(), + None => Ok(Vec::new()), + } + }; + Ok(Self { + enabled_branches: get_setting("enabled-branches")?, + disabled_branches: get_setting("disabled-branches")?, + }) + } + + /// Returns true if the advance-branches feature is enabled for + /// `branch_name`. + fn branch_is_eligible(&self, branch_name: &str) -> bool { + if self + .disabled_branches + .iter() + .any(|d| d.matches(branch_name)) + { + return false; + } + self.enabled_branches.iter().any(|e| e.matches(branch_name)) + } + + /// Returns true if the config includes at least one "enabled-branches" + /// pattern. + fn feature_enabled(&self) -> bool { + !self.enabled_branches.is_empty() + } +} + /// Provides utilities for writing a command that works on a [`Workspace`] /// (which most commands do). pub struct WorkspaceCommandHelper { @@ -1362,6 +1433,44 @@ Then run `jj squash` to move the resolution into the conflicted commit."#, Ok(()) } + + /// Identifies branches which are eligible to be moved automatically during + /// `jj commit` and `jj new`. Whether a branch is eligible is determined by + /// its target and the user and repo config for "advance-branches". + /// + /// Returns a Vec of branches in `repo` that point to any of the `from` + /// commits and that are eligible to advance. The `from` commits are + /// typically the parents of the target commit of `jj commit` or `jj new`. + /// + /// Branches are not moved until + /// `WorkspaceCommandTransaction::advance_branches()` is called with the + /// `AdvanceableBranch`s returned by this function. + /// + /// Returns an empty `std::Vec` if no branches are eligible to advance. + pub fn get_advanceable_branches<'a>( + &self, + from: impl IntoIterator, + ) -> Result, CommandError> { + let ab_settings = AdvanceBranchesSettings::from_config(self.settings.config())?; + if !ab_settings.feature_enabled() { + // Return early if we know that there's no work to do. + return Ok(Vec::new()); + } + + let mut advanceable_branches = Vec::new(); + for from_commit in from { + for (name, _) in self.repo().view().local_branches_for_commit(from_commit) { + if ab_settings.branch_is_eligible(name) { + advanceable_branches.push(AdvanceableBranch { + name: name.to_owned(), + old_commit_id: from_commit.clone(), + }); + } + } + } + + Ok(advanceable_branches) + } } /// A [`Transaction`] tied to a particular workspace. @@ -1448,6 +1557,23 @@ impl WorkspaceCommandTransaction<'_> { pub fn into_inner(self) -> Transaction { self.tx } + + /// Moves each branch in `branches` from an old commit it's associated with + /// (configured by `get_advanceable_branches`) to the `move_to` commit. If + /// the branch is conflicted before the update, it will remain conflicted + /// after the update, but the conflict will involve the `move_to` commit + /// instead of the old commit. + pub fn advance_branches(&mut self, branches: Vec, move_to: &CommitId) { + for branch in branches { + // This removes the old commit ID from the branch's RefTarget and + // replaces it with the `move_to` ID. + self.mut_repo().merge_local_branch( + &branch.name, + &RefTarget::normal(branch.old_commit_id), + &RefTarget::normal(move_to.clone()), + ); + } + } } fn find_workspace_dir(cwd: &Path) -> &Path { diff --git a/cli/src/commands/commit.rs b/cli/src/commands/commit.rs index 2ad459046d..5e1d35750f 100644 --- a/cli/src/commands/commit.rs +++ b/cli/src/commands/commit.rs @@ -54,6 +54,7 @@ pub(crate) fn cmd_commit( .get_wc_commit_id() .ok_or_else(|| user_error("This command requires a working copy"))?; let commit = workspace_command.repo().store().get_commit(commit_id)?; + let advanceable_branches = workspace_command.get_advanceable_branches(commit.parent_ids())?; let matcher = workspace_command.matcher_from_values(&args.paths)?; let diff_selector = workspace_command.diff_selector(ui, args.tool.as_deref(), args.interactive)?; @@ -119,6 +120,10 @@ new working-copy commit. commit.tree_id().clone(), ) .write()?; + + // Does nothing if there's no branches to advance. + tx.advance_branches(advanceable_branches, new_commit.id()); + for workspace_id in workspace_ids { tx.mut_repo().edit(workspace_id, &new_wc_commit).unwrap(); } diff --git a/cli/src/config-schema.json b/cli/src/config-schema.json index a6bca7fffc..3267b1b7a5 100644 --- a/cli/src/config-schema.json +++ b/cli/src/config-schema.json @@ -385,6 +385,26 @@ } } }, + "experimental-advance-branches": { + "type": "object", + "description": "Settings controlling the 'advance-branches' feature which moves branches forward when new commits are created.", + "properties": { + "enabled-branches": { + "type": "array", + "description": "Patterns used to identify branches which may be advanced.", + "items": { + "type": "string" + } + }, + "disabled-branches": { + "type": "array", + "description": "Patterns used to identify branches which are not advanced. Takes precedence over 'enabled-branches'.", + "items": { + "type": "string" + } + } + } + }, "signing": { "type": "object", "description": "Settings for verifying and creating cryptographic commit signatures", diff --git a/cli/tests/runner.rs b/cli/tests/runner.rs index dcd71c016a..20801685b2 100644 --- a/cli/tests/runner.rs +++ b/cli/tests/runner.rs @@ -9,6 +9,7 @@ fn test_no_forgotten_test_files() { } mod test_abandon_command; +mod test_advance_branches; mod test_alias; mod test_branch_command; mod test_builtin_aliases; diff --git a/cli/tests/test_advance_branches.rs b/cli/tests/test_advance_branches.rs new file mode 100644 index 0000000000..384b1de3a0 --- /dev/null +++ b/cli/tests/test_advance_branches.rs @@ -0,0 +1,236 @@ +use std::path::Path; + +use itertools::Itertools; + +use crate::common::TestEnvironment; + +fn get_log_output_with_branches(test_env: &TestEnvironment, cwd: &Path) -> String { + let template = r#"commit_id.short() ++ " br:{" ++ local_branches ++ "} dsc: " ++ description"#; + test_env.jj_cmd_success(cwd, &["log", "-T", template]) +} + +fn enable_advance_branches_for_patterns(test_env: &TestEnvironment, cwd: &Path, patterns: &[&str]) { + #[rustfmt::skip] + let pattern_string: String = patterns.iter().map(|x| format!("\"{}\"", x)).join(","); + test_env.jj_cmd_success( + cwd, + &[ + "config", + "set", + "--repo", + "experimental-advance-branches.enabled-branches", + &format!("[{}]", pattern_string), + ], + ); +} + +fn set_advance_branches(test_env: &TestEnvironment, cwd: &Path, value: bool) { + if value { + enable_advance_branches_for_patterns(test_env, cwd, &["glob:*"]); + } else { + enable_advance_branches_for_patterns(test_env, cwd, &[""]); + } +} + +// Check that enabling and disabling advance-branches works as expected. +#[test] +fn test_advance_branches_enabled() { + let test_env = TestEnvironment::default(); + test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]); + let workspace_path = test_env.env_root().join("repo"); + + // First, test with advance-branches enabled. Start by creating a branch on the + // root commit. + set_advance_branches(&test_env, &workspace_path, true); + test_env.jj_cmd_ok( + &workspace_path, + &["branch", "create", "-r", "@-", "test_branch"], + ); + + // Check the initial state of the repo. + insta::assert_snapshot!(get_log_output_with_branches(&test_env, &workspace_path), @r###" + @ 230dd059e1b0 br:{} dsc: + ◉ 000000000000 br:{test_branch} dsc: + "###); + + // Run jj commit, which will advance the branch pointing to @-. + test_env.jj_cmd_ok(&workspace_path, &["commit", "-m=first"]); + insta::assert_snapshot!(get_log_output_with_branches(&test_env, &workspace_path), @r###" + @ 24bb7f9da598 br:{} dsc: + ◉ 95f2456c4bbd br:{test_branch} dsc: first + ◉ 000000000000 br:{} dsc: + "###); + + // Now disable advance branches and commit again. The branch shouldn't move. + set_advance_branches(&test_env, &workspace_path, false); + test_env.jj_cmd_ok(&workspace_path, &["commit", "-m=second"]); + insta::assert_snapshot!(get_log_output_with_branches(&test_env, &workspace_path), @r###" + @ b29edd893970 br:{} dsc: + ◉ ebf7d96fb6ad br:{} dsc: second + ◉ 95f2456c4bbd br:{test_branch} dsc: first + ◉ 000000000000 br:{} dsc: + "###); +} + +// Check that only a branch pointing to @- advances. Branches pointing to @ are +// not advanced. +#[test] +fn test_advance_branches_at_minus() { + let test_env = TestEnvironment::default(); + test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]); + let workspace_path = test_env.env_root().join("repo"); + + set_advance_branches(&test_env, &workspace_path, true); + test_env.jj_cmd_ok(&workspace_path, &["branch", "create", "test_branch"]); + + insta::assert_snapshot!(get_log_output_with_branches(&test_env, &workspace_path), @r###" + @ 230dd059e1b0 br:{test_branch} dsc: + ◉ 000000000000 br:{} dsc: + "###); + + test_env.jj_cmd_ok(&workspace_path, &["commit", "-m=first"]); + insta::assert_snapshot!(get_log_output_with_branches(&test_env, &workspace_path), @r###" + @ 24bb7f9da598 br:{} dsc: + ◉ 95f2456c4bbd br:{test_branch} dsc: first + ◉ 000000000000 br:{} dsc: + "###); + + // Create a second branch pointing to @. On the next commit, only the first + // branch, which points to @-, will advance. + test_env.jj_cmd_ok(&workspace_path, &["branch", "create", "test_branch2"]); + test_env.jj_cmd_ok(&workspace_path, &["commit", "-m=second"]); + insta::assert_snapshot!(get_log_output_with_branches(&test_env, &workspace_path), @r###" + @ b29edd893970 br:{} dsc: + ◉ ebf7d96fb6ad br:{test_branch test_branch2} dsc: second + ◉ 95f2456c4bbd br:{} dsc: first + ◉ 000000000000 br:{} dsc: + "###); +} + +// Test that per-branch overrides invert the behavior of +// experimental-advance-branches.enabled. +#[test] +fn test_advance_branches_overrides() { + let test_env = TestEnvironment::default(); + test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]); + let workspace_path = test_env.env_root().join("repo"); + + // advance-branches is disabled by default. + test_env.jj_cmd_ok( + &workspace_path, + &["branch", "create", "-r", "@-", "test_branch"], + ); + + // Check the initial state of the repo. + insta::assert_snapshot!(get_log_output_with_branches(&test_env, &workspace_path), @r###" + @ 230dd059e1b0 br:{} dsc: + ◉ 000000000000 br:{test_branch} dsc: + "###); + + // Commit will not advance the branch since advance-branches is disabled. + test_env.jj_cmd_ok(&workspace_path, &["commit", "-m=first"]); + insta::assert_snapshot!(get_log_output_with_branches(&test_env, &workspace_path), @r###" + @ 7e3a6f5e0f15 br:{} dsc: + ◉ 307e33f70413 br:{} dsc: first + ◉ 000000000000 br:{test_branch} dsc: + "###); + + // Now enable advance branches for "test_branch", move the branch, and commit + // again. + test_env.add_config( + r#"[experimental-advance-branches] + enabled-branches = ["test_branch"] + "#, + ); + test_env.jj_cmd_ok( + &workspace_path, + &["branch", "set", "test_branch", "-r", "@-"], + ); + insta::assert_snapshot!(get_log_output_with_branches(&test_env, &workspace_path), @r###" + @ 7e3a6f5e0f15 br:{} dsc: + ◉ 307e33f70413 br:{test_branch} dsc: first + ◉ 000000000000 br:{} dsc: + "###); + test_env.jj_cmd_ok(&workspace_path, &["commit", "-m=second"]); + insta::assert_snapshot!(get_log_output_with_branches(&test_env, &workspace_path), @r###" + @ 8c1bd3e7de60 br:{} dsc: + ◉ 468d1ab20fb3 br:{test_branch} dsc: second + ◉ 307e33f70413 br:{} dsc: first + ◉ 000000000000 br:{} dsc: + "###); + + // Now disable advance branches for "test_branch" and "second_branch", which + // we will use later. Disabling always takes precedence over enabling. + test_env.add_config( + r#"[experimental-advance-branches] + enabled-branches = ["test_branch", "second_branch"] + disabled-branches = ["test_branch"] + "#, + ); + test_env.jj_cmd_ok(&workspace_path, &["commit", "-m=third"]); + insta::assert_snapshot!(get_log_output_with_branches(&test_env, &workspace_path), @r###" + @ 5888a83948dd br:{} dsc: + ◉ 50e9c28e6d85 br:{} dsc: third + ◉ 468d1ab20fb3 br:{test_branch} dsc: second + ◉ 307e33f70413 br:{} dsc: first + ◉ 000000000000 br:{} dsc: + "###); + + // If we create a new branch at @- and move test_branch there as well. When + // we commit, only "second_branch" will advance since "test_branch" is disabled. + test_env.jj_cmd_ok( + &workspace_path, + &["branch", "create", "second_branch", "-r", "@-"], + ); + test_env.jj_cmd_ok( + &workspace_path, + &["branch", "set", "test_branch", "-r", "@-"], + ); + insta::assert_snapshot!(get_log_output_with_branches(&test_env, &workspace_path), @r###" + @ 5888a83948dd br:{} dsc: + ◉ 50e9c28e6d85 br:{second_branch test_branch} dsc: third + ◉ 468d1ab20fb3 br:{} dsc: second + ◉ 307e33f70413 br:{} dsc: first + ◉ 000000000000 br:{} dsc: + "###); + test_env.jj_cmd_ok(&workspace_path, &["commit", "-m=fourth"]); + insta::assert_snapshot!(get_log_output_with_branches(&test_env, &workspace_path), @r###" + @ 666d42aedca7 br:{} dsc: + ◉ f23aa63eeb99 br:{second_branch} dsc: fourth + ◉ 50e9c28e6d85 br:{test_branch} dsc: third + ◉ 468d1ab20fb3 br:{} dsc: second + ◉ 307e33f70413 br:{} dsc: first + ◉ 000000000000 br:{} dsc: + "###); +} + +// If multiple eligible branches point to @-, all of them will be advanced. +#[test] +fn test_advance_branches_multiple_branches() { + let test_env = TestEnvironment::default(); + test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]); + let workspace_path = test_env.env_root().join("repo"); + + set_advance_branches(&test_env, &workspace_path, true); + test_env.jj_cmd_ok( + &workspace_path, + &["branch", "create", "-r", "@-", "first_branch"], + ); + test_env.jj_cmd_ok( + &workspace_path, + &["branch", "create", "-r", "@-", "second_branch"], + ); + // Check the initial state of the repo. + insta::assert_snapshot!(get_log_output_with_branches(&test_env, &workspace_path), @r###" + @ 230dd059e1b0 br:{} dsc: + ◉ 000000000000 br:{first_branch second_branch} dsc: + "###); + + // Both branches are eligible and both will advance. + test_env.jj_cmd_ok(&workspace_path, &["commit", "-m=first"]); + insta::assert_snapshot!(get_log_output_with_branches(&test_env, &workspace_path), @r###" + @ f307e5d9f90b br:{} dsc: + ◉ 0fca5c9228e6 br:{first_branch second_branch} dsc: first + ◉ 000000000000 br:{} dsc: + "###); +} diff --git a/lib/src/view.rs b/lib/src/view.rs index 4a1b3af1bb..1125bd0b2b 100644 --- a/lib/src/view.rs +++ b/lib/src/view.rs @@ -123,6 +123,16 @@ impl View { .map(|(name, target)| (name.as_ref(), target)) } + /// Iterates local branches `(name, target)` in lexicographical order where + /// the target adds `commit_id`. + pub fn local_branches_for_commit<'a: 'b, 'b>( + &'a self, + commit_id: &'b CommitId, + ) -> impl Iterator + 'b { + self.local_branches() + .filter(|(_, target)| target.added_ids().contains(commit_id)) + } + /// Iterates local branch `(name, target)`s matching the given pattern. /// Entries are sorted by `name`. pub fn local_branches_matching<'a: 'b, 'b>(