From bf543402ccfb7c41e8f9aa9353befbb8e66d08bc Mon Sep 17 00:00:00 2001 From: Danny Hooper Date: Fri, 26 Jul 2024 19:44:03 -0500 Subject: [PATCH] cli: fix: add --include-unchanged-files flag to allow fixing as yet unchanged files This enables workflows like "insert a commit that reformats the code in one of my project directories". `jj fix --include-unchanged-files` is an easy way to fix everything in the repo. `jj fix --include-unchanged-files ` fixes all of the `` even if they are unchanged. This is mostly orthogonal to other features, so not many tests are added. This is a significant and simple enough improvement that I think it's appropriate to make it here instead of waiting for a `jj run`-based solution. --- CHANGELOG.md | 4 + cli/src/commands/fix.rs | 31 ++++--- cli/tests/cli-reference@.md.snap | 1 + cli/tests/test_fix_command.rs | 147 +++++++++++++++++++++++++++++++ 4 files changed, 172 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index abb70b6da..bbf0cc404 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,10 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ### New features +* `jj fix` now allows fixing unchanged files with the `--include-unchanged-files` flag. This + can be used to more easily introduce automatic formatting changes in a new + commit separate from other changes. + ### Fixed bugs * Fixed panic when parsing invalid conflict markers of a particular form. diff --git a/cli/src/commands/fix.rs b/cli/src/commands/fix.rs index 718d10e85..a371e7a24 100644 --- a/cli/src/commands/fix.rs +++ b/cli/src/commands/fix.rs @@ -28,6 +28,7 @@ use jj_lib::fileset; use jj_lib::fileset::FilesetExpression; use jj_lib::matchers::EverythingMatcher; use jj_lib::matchers::Matcher; +use jj_lib::merged_tree::MergedTree; use jj_lib::merged_tree::MergedTreeBuilder; use jj_lib::merged_tree::TreeDiffEntry; use jj_lib::repo::Repo; @@ -36,6 +37,7 @@ use jj_lib::repo_path::RepoPathUiConverter; use jj_lib::revset::RevsetExpression; use jj_lib::revset::RevsetIteratorExt; use jj_lib::store::Store; +use jj_lib::tree::Tree; use pollster::FutureExt; use rayon::iter::IntoParallelIterator; use rayon::prelude::ParallelIterator; @@ -125,6 +127,10 @@ pub(crate) struct FixArgs { /// Fix only these paths #[arg(value_hint = clap::ValueHint::AnyPath)] paths: Vec, + /// Fix unchanged files in addition to changed ones. If no paths are + /// specified, all files in the repo will be fixed. + #[arg(long)] + include_unchanged_files: bool, } #[instrument(skip_all)] @@ -177,19 +183,22 @@ pub(crate) fn cmd_fix( for commit in commits.iter().rev() { let mut paths: HashSet = HashSet::new(); - // Fix all paths that were fixed in ancestors, so we don't lose those changes. - // We do this instead of rebasing onto those changes, to avoid merge conflicts. - for parent_id in commit.parent_ids() { - if let Some(parent_paths) = commit_paths.get(parent_id) { - paths.extend(parent_paths.iter().cloned()); + // If --include-unchanged-files, we always fix every matching file in the tree. + // Otherwise, we fix the matching changed files in this commit, plus any that + // were fixed in ancestors, so we don't lose those changes. We do this + // instead of rebasing onto those changes, to avoid merge conflicts. + let parent_tree = if args.include_unchanged_files { + MergedTree::resolved(Tree::empty(tx.repo().store().clone(), RepoPathBuf::root())) + } else { + for parent_id in commit.parent_ids() { + if let Some(parent_paths) = commit_paths.get(parent_id) { + paths.extend(parent_paths.iter().cloned()); + } } - } - - // Also fix any new paths that were changed in this commit. - let tree = commit.tree()?; - let parent_tree = commit.parent_tree(tx.repo())?; + commit.parent_tree(tx.repo())? + }; // TODO: handle copy tracking - let mut diff_stream = parent_tree.diff_stream(&tree, &matcher); + let mut diff_stream = parent_tree.diff_stream(&commit.tree()?, &matcher); async { while let Some(TreeDiffEntry { path: repo_path, diff --git a/cli/tests/cli-reference@.md.snap b/cli/tests/cli-reference@.md.snap index 507e45310..bc966d4f3 100644 --- a/cli/tests/cli-reference@.md.snap +++ b/cli/tests/cli-reference@.md.snap @@ -900,6 +900,7 @@ will be removed in a future version. ###### **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 `reachable(@, mutable())` if it is not set +* `--include-unchanged-files` — Fix unchanged files in addition to changed ones. If no paths are specified, all files in the repo will be fixed diff --git a/cli/tests/test_fix_command.rs b/cli/tests/test_fix_command.rs index 241d91836..8ad77e8cb 100644 --- a/cli/tests/test_fix_command.rs +++ b/cli/tests/test_fix_command.rs @@ -1123,3 +1123,150 @@ fn test_fix_resolve_conflict() { CONTENT "###); } + +#[test] +fn test_all_files() { + let test_env = TestEnvironment::default(); + test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]); + let repo_path = test_env.env_root().join("repo"); + let formatter_path = assert_cmd::cargo::cargo_bin("fake-formatter"); + assert!(formatter_path.is_file()); + let escaped_formatter_path = formatter_path.to_str().unwrap().replace('\\', r"\\"); + + // Consider a few cases: + // File A: in patterns, changed in child + // File B: in patterns, NOT changed in child + // File C: NOT in patterns, NOT changed in child + // File D: NOT in patterns, changed in child + // Some files will be in subdirectories to make sure we're covering that aspect + // of matching. + test_env.add_config(&format!( + r###" + [fix.tools.tool] + command = ["{formatter}", "--append", "fixed"] + patterns = ["a/a", "b/b"] + "###, + formatter = escaped_formatter_path.as_str() + )); + + std::fs::create_dir(repo_path.join("a")).unwrap(); + std::fs::create_dir(repo_path.join("b")).unwrap(); + std::fs::create_dir(repo_path.join("c")).unwrap(); + std::fs::write(repo_path.join("a/a"), "parent aaa\n").unwrap(); + std::fs::write(repo_path.join("b/b"), "parent bbb\n").unwrap(); + std::fs::write(repo_path.join("c/c"), "parent ccc\n").unwrap(); + std::fs::write(repo_path.join("ddd"), "parent ddd\n").unwrap(); + let (_stdout, _stderr) = test_env.jj_cmd_ok(&repo_path, &["commit", "-m", "parent"]); + + std::fs::write(repo_path.join("a/a"), "child aaa\n").unwrap(); + std::fs::write(repo_path.join("ddd"), "child ddd\n").unwrap(); + let (_stdout, _stderr) = test_env.jj_cmd_ok(&repo_path, &["describe", "-m", "child"]); + + // Specifying files means exactly those files will be fixed in each revision, + // although some like file C won't have any tools configured to make changes to + // them. Specified but unfixed files are silently skipped, whether they lack + // configuration, are ignored, don't exist, aren't normal files, etc. + let (stdout, stderr) = test_env.jj_cmd_ok( + &repo_path, + &[ + "fix", + "--include-unchanged-files", + "b/b", + "c/c", + "does_not.exist", + ], + ); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Fixed 2 commits of 2 checked. + Working copy now at: rlvkpnrz c098d165 child + Parent commit : qpvuntsm 0bb31627 parent + Added 0 files, modified 1 files, removed 0 files + "###); + + let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "a/a", "-r", "@-"]); + insta::assert_snapshot!(content, @r###" + parent aaa + "###); + let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "b/b", "-r", "@-"]); + insta::assert_snapshot!(content, @r###" + parent bbb + fixed + "###); + let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "c/c", "-r", "@-"]); + insta::assert_snapshot!(content, @r###" + parent ccc + "###); + let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "ddd", "-r", "@-"]); + insta::assert_snapshot!(content, @r###" + parent ddd + "###); + + let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "a/a", "-r", "@"]); + insta::assert_snapshot!(content, @r###" + child aaa + "###); + let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "b/b", "-r", "@"]); + insta::assert_snapshot!(content, @r###" + parent bbb + fixed + "###); + let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "c/c", "-r", "@"]); + insta::assert_snapshot!(content, @r###" + parent ccc + "###); + let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "ddd", "-r", "@"]); + insta::assert_snapshot!(content, @r###" + child ddd + "###); + + // Not specifying files means all files will be fixed in each revision. + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["fix", "--include-unchanged-files"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Fixed 2 commits of 2 checked. + Working copy now at: rlvkpnrz c5d0aa1d child + Parent commit : qpvuntsm b4d02ca9 parent + Added 0 files, modified 2 files, removed 0 files + "###); + + let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "a/a", "-r", "@-"]); + insta::assert_snapshot!(content, @r###" + parent aaa + fixed + "###); + let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "b/b", "-r", "@-"]); + insta::assert_snapshot!(content, @r###" + parent bbb + fixed + fixed + "###); + let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "c/c", "-r", "@-"]); + insta::assert_snapshot!(content, @r###" + parent ccc + "###); + let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "ddd", "-r", "@-"]); + insta::assert_snapshot!(content, @r###" + parent ddd + "###); + + let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "a/a", "-r", "@"]); + insta::assert_snapshot!(content, @r###" + child aaa + fixed + "###); + let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "b/b", "-r", "@"]); + insta::assert_snapshot!(content, @r###" + parent bbb + fixed + fixed + "###); + let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "c/c", "-r", "@"]); + insta::assert_snapshot!(content, @r###" + parent ccc + "###); + let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "ddd", "-r", "@"]); + insta::assert_snapshot!(content, @r###" + child ddd + "###); +}