From 3cfd3be2a62106126503eb40f9396f757b73b980 Mon Sep 17 00:00:00 2001 From: Danny Hooper Date: Thu, 6 Jun 2024 21:42:50 -0500 Subject: [PATCH 1/2] cli `fix`: add `revsets.fix` config for default revset to be fixed --- CHANGELOG.md | 3 ++ cli/src/commands/fix.rs | 21 ++++++------ cli/src/config-schema.json | 5 +++ cli/src/config/revsets.toml | 1 + cli/tests/cli-reference@.md.snap | 2 +- cli/tests/test_fix_command.rs | 55 ++++++++++++++++++++++++++++++++ 6 files changed, 77 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3c07e878f0..bdcd132c7a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * Show paths to config files when configuration errors occur +* `jj fix` now supports configuring the default revset for `-s` using the + `revsets.fix` config. + ### Fixed bugs ## [0.18.0] - 2024-06-05 diff --git a/cli/src/commands/fix.rs b/cli/src/commands/fix.rs index 6112e0ef87..cdf1181db8 100644 --- a/cli/src/commands/fix.rs +++ b/cli/src/commands/fix.rs @@ -65,7 +65,9 @@ use crate::ui::Ui; #[derive(clap::Args, Clone, Debug)] #[command(verbatim_doc_comment)] pub(crate) struct FixArgs { - /// Fix files in the specified revision(s) and their descendants + /// Fix files in the specified revision(s) and their descendants. If no + /// revisions are specified, this defaults to the `revsets.fix` setting, or + /// `@` if it is not set. #[arg(long, short)] source: Vec, /// Fix only these paths @@ -80,14 +82,15 @@ pub(crate) fn cmd_fix( args: &FixArgs, ) -> Result<(), CommandError> { let mut workspace_command = command.workspace_helper(ui)?; - let root_commits: Vec = workspace_command - .parse_union_revsets(if args.source.is_empty() { - &[RevisionArg::AT] - } else { - &args.source - })? - .evaluate_to_commit_ids()? - .collect(); + let root_commits: Vec = if args.source.is_empty() { + workspace_command.parse_revset(&RevisionArg::from( + command.settings().config().get_string("revsets.fix")?, + ))? + } else { + workspace_command.parse_union_revsets(&args.source)? + } + .evaluate_to_commit_ids()? + .collect(); workspace_command.check_rewritable(root_commits.iter())?; let matcher = workspace_command .parse_file_patterns(&args.paths)? diff --git a/cli/src/config-schema.json b/cli/src/config-schema.json index d9267ac88c..95df003898 100644 --- a/cli/src/config-schema.json +++ b/cli/src/config-schema.json @@ -346,6 +346,11 @@ "type": "object", "description": "Revset expressions used by various commands", "properties": { + "fix": { + "type": "string", + "description": "Default set of revisions to fix when no explicit revset is given for jj fix", + "default": "@" + }, "log": { "type": "string", "description": "Default set of revisions to show when no explicit revset is given for jj log and similar commands", diff --git a/cli/src/config/revsets.toml b/cli/src/config/revsets.toml index 60517d3ffb..776ae15cb0 100644 --- a/cli/src/config/revsets.toml +++ b/cli/src/config/revsets.toml @@ -2,6 +2,7 @@ # adding/updating any of these aliases [revsets] +fix = "@" log = "@ | ancestors(immutable_heads().., 2) | trunk()" [revset-aliases] diff --git a/cli/tests/cli-reference@.md.snap b/cli/tests/cli-reference@.md.snap index 8b074b21e1..7454d797ec 100644 --- a/cli/tests/cli-reference@.md.snap +++ b/cli/tests/cli-reference@.md.snap @@ -798,7 +798,7 @@ And then run the command `jj fix -s @`. ###### **Options:** -* `-s`, `--source ` — Fix files in the specified revision(s) and their descendants +* `-s`, `--source ` — Fix files in the specified revision(s) and their descendants. If no revisions are specified, this defaults to the `revsets.fix` setting, or `@` if it is not set diff --git a/cli/tests/test_fix_command.rs b/cli/tests/test_fix_command.rs index 2db3e696c3..2e66ed884c 100644 --- a/cli/tests/test_fix_command.rs +++ b/cli/tests/test_fix_command.rs @@ -139,6 +139,61 @@ fn test_fix_sibling_commit() { insta::assert_snapshot!(content, @"child2"); } +#[test] +fn test_default_revset() { + let (test_env, repo_path) = init_with_fake_formatter(&["--uppercase"]); + std::fs::write(repo_path.join("file"), "foo").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["branch", "create", "foo"]); + test_env.jj_cmd_ok(&repo_path, &["new"]); + std::fs::write(repo_path.join("file"), "bar").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["branch", "create", "bar"]); + test_env.jj_cmd_ok(&repo_path, &["new", "foo"]); + std::fs::write(repo_path.join("file"), "baz").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["branch", "create", "baz"]); + + // With no args and no revset configuration, we fix `@`. + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["fix"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Fixed 1 commits of 1 checked. + Working copy now at: mzvwutvl 5205c5f1 baz | (no description set) + Parent commit : qpvuntsm 34af74d0 foo | (no description set) + Added 0 files, modified 1 files, removed 0 files + "###); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file", "-r", "foo"]); + insta::assert_snapshot!(content, @"foo"); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file", "-r", "bar"]); + insta::assert_snapshot!(content, @"bar"); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file", "-r", "baz"]); + insta::assert_snapshot!(content, @"BAZ"); +} + +#[test] +fn test_custom_default_revset() { + let (test_env, repo_path) = init_with_fake_formatter(&["--uppercase"]); + + std::fs::write(repo_path.join("file"), "foo").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["branch", "create", "foo"]); + test_env.jj_cmd_ok(&repo_path, &["new"]); + std::fs::write(repo_path.join("file"), "bar").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["branch", "create", "bar"]); + + // Check out a different commit so that the schema default `@` would behave + // differently from our customized default. + test_env.jj_cmd_ok(&repo_path, &["new", "-r", "foo"]); + test_env.add_config(r#"revsets.fix = "bar""#); + + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["fix"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Fixed 1 commits of 1 checked. + "###); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file", "-r", "foo"]); + insta::assert_snapshot!(content, @"foo"); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file", "-r", "bar"]); + insta::assert_snapshot!(content, @"BAR"); +} + #[test] fn test_fix_immutable_commit() { let (test_env, repo_path) = init_with_fake_formatter(&["--uppercase"]); From ab16829217ed34e650809e70b552f526ea45559a Mon Sep 17 00:00:00 2001 From: Danny Hooper Date: Thu, 6 Jun 2024 22:10:21 -0500 Subject: [PATCH 2/2] cli `fix`: change default from `-s @` to `-s 'reachable(@, mutable())'` Most of the value of `jj fix` over a shell script is in formatting commits other than `@`. `@::` often doesn't contain those other commits, so `-s @` is a bad default. We could get the same effect from `-s 'mutable() & ::@'`, but `reachable()` is a bit more explicit and simple to read. We could also base this on excluding `trunk()`, but that just seems like an indirection for `mutable()` that might ignore the user's intent if they have configured part of trunk to be mutable. --- CHANGELOG.md | 3 ++ cli/src/commands/fix.rs | 2 +- cli/src/config-schema.json | 2 +- cli/src/config/revsets.toml | 2 +- cli/tests/cli-reference@.md.snap | 2 +- cli/tests/test_fix_command.rs | 51 ++++++++++++++++++++++---------- 6 files changed, 42 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bdcd132c7a..c871505f9a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * Dropped support for automatic upgrade of repo formats used by versions before 0.12.0. +* `jj fix` now defaults to the broader revset `-s reachable(@, mutable())` + instead of `-s @`. + ### Deprecations ### New features diff --git a/cli/src/commands/fix.rs b/cli/src/commands/fix.rs index cdf1181db8..590dfb9948 100644 --- a/cli/src/commands/fix.rs +++ b/cli/src/commands/fix.rs @@ -67,7 +67,7 @@ use crate::ui::Ui; pub(crate) struct FixArgs { /// Fix files in the specified revision(s) and their descendants. If no /// revisions are specified, this defaults to the `revsets.fix` setting, or - /// `@` if it is not set. + /// `reachable(@, mutable())` if it is not set. #[arg(long, short)] source: Vec, /// Fix only these paths diff --git a/cli/src/config-schema.json b/cli/src/config-schema.json index 95df003898..25934f95de 100644 --- a/cli/src/config-schema.json +++ b/cli/src/config-schema.json @@ -349,7 +349,7 @@ "fix": { "type": "string", "description": "Default set of revisions to fix when no explicit revset is given for jj fix", - "default": "@" + "default": "reachable(@, mutable())" }, "log": { "type": "string", diff --git a/cli/src/config/revsets.toml b/cli/src/config/revsets.toml index 776ae15cb0..d84a745b3e 100644 --- a/cli/src/config/revsets.toml +++ b/cli/src/config/revsets.toml @@ -2,7 +2,7 @@ # adding/updating any of these aliases [revsets] -fix = "@" +fix = "reachable(@, mutable())" log = "@ | ancestors(immutable_heads().., 2) | trunk()" [revset-aliases] diff --git a/cli/tests/cli-reference@.md.snap b/cli/tests/cli-reference@.md.snap index 7454d797ec..49ab95d811 100644 --- a/cli/tests/cli-reference@.md.snap +++ b/cli/tests/cli-reference@.md.snap @@ -798,7 +798,7 @@ And then run the command `jj fix -s @`. ###### **Options:** -* `-s`, `--source ` — Fix files in the specified revision(s) and their descendants. If no revisions are specified, this defaults to the `revsets.fix` setting, or `@` if it is not set +* `-s`, `--source ` — Fix files in the specified revision(s) and their descendants. If no revisions are specified, this defaults to the `revsets.fix` setting, or `reachable(@, mutable())` if it is not set diff --git a/cli/tests/test_fix_command.rs b/cli/tests/test_fix_command.rs index 2e66ed884c..695d7b2b1b 100644 --- a/cli/tests/test_fix_command.rs +++ b/cli/tests/test_fix_command.rs @@ -142,30 +142,49 @@ fn test_fix_sibling_commit() { #[test] fn test_default_revset() { let (test_env, repo_path) = init_with_fake_formatter(&["--uppercase"]); + std::fs::write(repo_path.join("file"), "trunk1").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["branch", "create", "trunk1"]); + test_env.jj_cmd_ok(&repo_path, &["new"]); + std::fs::write(repo_path.join("file"), "trunk2").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["branch", "create", "trunk2"]); + test_env.jj_cmd_ok(&repo_path, &["new", "trunk1"]); std::fs::write(repo_path.join("file"), "foo").unwrap(); test_env.jj_cmd_ok(&repo_path, &["branch", "create", "foo"]); + test_env.jj_cmd_ok(&repo_path, &["new", "trunk1"]); + std::fs::write(repo_path.join("file"), "bar1").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["branch", "create", "bar1"]); test_env.jj_cmd_ok(&repo_path, &["new"]); - std::fs::write(repo_path.join("file"), "bar").unwrap(); - test_env.jj_cmd_ok(&repo_path, &["branch", "create", "bar"]); - test_env.jj_cmd_ok(&repo_path, &["new", "foo"]); - std::fs::write(repo_path.join("file"), "baz").unwrap(); - test_env.jj_cmd_ok(&repo_path, &["branch", "create", "baz"]); - - // With no args and no revset configuration, we fix `@`. + std::fs::write(repo_path.join("file"), "bar2").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["branch", "create", "bar2"]); + test_env.jj_cmd_ok(&repo_path, &["new"]); + std::fs::write(repo_path.join("file"), "bar3").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["branch", "create", "bar3"]); + test_env.jj_cmd_ok(&repo_path, &["edit", "bar2"]); + + // With no args and no revset configuration, we fix `reachable(@, mutable())`, + // which includes bar{1,2,3} and excludes trunk{1,2} (which is immutable) and + // foo (which is mutable but not reachable). + test_env.add_config(r#"revset-aliases."immutable_heads()" = "trunk2""#); let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["fix"]); insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(stderr, @r###" - Fixed 1 commits of 1 checked. - Working copy now at: mzvwutvl 5205c5f1 baz | (no description set) - Parent commit : qpvuntsm 34af74d0 foo | (no description set) + Fixed 3 commits of 3 checked. + Working copy now at: yostqsxw 0bd830d2 bar2 | (no description set) + Parent commit : yqosqzyt 4747dd17 bar1 | (no description set) Added 0 files, modified 1 files, removed 0 files "###); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file", "-r", "trunk1"]); + insta::assert_snapshot!(content, @"trunk1"); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file", "-r", "trunk2"]); + insta::assert_snapshot!(content, @"trunk2"); let content = test_env.jj_cmd_success(&repo_path, &["print", "file", "-r", "foo"]); insta::assert_snapshot!(content, @"foo"); - let content = test_env.jj_cmd_success(&repo_path, &["print", "file", "-r", "bar"]); - insta::assert_snapshot!(content, @"bar"); - let content = test_env.jj_cmd_success(&repo_path, &["print", "file", "-r", "baz"]); - insta::assert_snapshot!(content, @"BAZ"); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file", "-r", "bar1"]); + insta::assert_snapshot!(content, @"BAR1"); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file", "-r", "bar2"]); + insta::assert_snapshot!(content, @"BAR2"); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file", "-r", "bar3"]); + insta::assert_snapshot!(content, @"BAR3"); } #[test] @@ -178,8 +197,8 @@ fn test_custom_default_revset() { std::fs::write(repo_path.join("file"), "bar").unwrap(); test_env.jj_cmd_ok(&repo_path, &["branch", "create", "bar"]); - // Check out a different commit so that the schema default `@` would behave - // differently from our customized default. + // Check out a different commit so that the schema default `reachable(@, + // mutable())` would behave differently from our customized default. test_env.jj_cmd_ok(&repo_path, &["new", "-r", "foo"]); test_env.add_config(r#"revsets.fix = "bar""#);