diff --git a/CHANGELOG.md b/CHANGELOG.md index bfd9ce82a5..2009f6a0c6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -58,6 +58,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * Upgraded `scm-record` from v0.2.0 to v0.3.0. See release notes at https://github.com/arxanas/scm-record/releases/tag/v0.3.0 +* New command `jj fix` that can be configured to update commits by running code + formatters (or similar tools) on changed files. The configuration schema and + flags are minimal for now, with a number of improvements planned (for example, + [#3800](https://github.com/martinvonz/jj/issues/3800) and + [#3801](https://github.com/martinvonz/jj/issues/3801)). + ### Fixed bugs * Previously, `jj git push` only made sure that the branch is in the expected diff --git a/Cargo.lock b/Cargo.lock index 2b71e7fbff..407a4cadde 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1715,6 +1715,7 @@ dependencies = [ "pest", "pest_derive", "pollster", + "rayon", "regex", "rpassword", "scm-record", diff --git a/cli/Cargo.toml b/cli/Cargo.toml index 7283e9a8c5..fd1085b8b9 100644 --- a/cli/Cargo.toml +++ b/cli/Cargo.toml @@ -75,6 +75,7 @@ once_cell = { workspace = true } pest = { workspace = true } pest_derive = { workspace = true } pollster = { workspace = true } +rayon = { workspace = true } regex = { workspace = true } rpassword = { workspace = true } scm-record = { workspace = true } diff --git a/cli/src/commands/fix.rs b/cli/src/commands/fix.rs index 04e8775e4a..795bd03038 100644 --- a/cli/src/commands/fix.rs +++ b/cli/src/commands/fix.rs @@ -13,11 +13,13 @@ // limitations under the License. use std::collections::{HashMap, HashSet}; +use std::io::Write; +use std::process::Stdio; +use std::sync::mpsc::channel; use futures::StreamExt; use itertools::Itertools; use jj_lib::backend::{BackendError, BackendResult, CommitId, FileId, TreeValue}; -use jj_lib::commit::{Commit, CommitIteratorExt}; use jj_lib::matchers::EverythingMatcher; use jj_lib::merged_tree::MergedTreeBuilder; use jj_lib::repo::Repo; @@ -25,19 +27,48 @@ use jj_lib::repo_path::RepoPathBuf; use jj_lib::revset::{RevsetExpression, RevsetIteratorExt}; use jj_lib::store::Store; use pollster::FutureExt; +use rayon::iter::IntoParallelIterator; +use rayon::prelude::ParallelIterator; use tracing::instrument; use crate::cli_util::{CommandHelper, RevisionArg}; -use crate::command_error::CommandError; +use crate::command_error::{config_error_with_message, CommandError}; +use crate::config::CommandNameAndArgs; use crate::ui::Ui; -/// Format files +/// Update files with formatting fixes or other changes +/// +/// The primary use case for this command is to apply the results of automatic +/// code formatting tools to revisions that may not be properly formatted yet. +/// It can also be used to modify files with other tools like `sed` or `sort`. +/// +/// The changed files in the given revisions will be updated with any fixes +/// determined by passing their file content through the external tool. +/// Descendants will also be updated by passing their versions of the same files +/// through the same external tool, which will never result in new conflicts. +/// Files with existing conflicts will be updated on all sides of the conflict, +/// which can potentially increase or decrease the number of conflict markers. +/// +/// The external tool must accept the current file content on standard input, +/// and return the updated file content on standard output. The output will not +/// be used unless the tool exits with a successful exit code. Output on +/// standard error will be passed through to the terminal. +/// +/// The configuration schema is expected to change in the future. For now, it +/// defines a single command that will affect all changed files in the specified +/// revisions. For example, to format some Rust code changed in the working copy +/// revision, you could write this configuration: +/// +/// [fix] +/// tool-command = ["rustfmt", "--emit", "stdout"] +/// +/// And then run the command `jj fix -s @`. #[derive(clap::Args, Clone, Debug)] #[command(verbatim_doc_comment)] pub(crate) struct FixArgs { - /// Fix these commits and all their descendants + /// Fix files in the specified revision(s) and their descendants #[arg(long, short)] - sources: Vec, + source: Vec, } #[instrument(skip_all)] @@ -47,107 +78,245 @@ 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(&args.sources)? - .evaluate_to_commits()? - .try_collect()?; - workspace_command.check_rewritable(root_commits.iter().ids())?; + let root_commits: Vec = workspace_command + .parse_union_revsets(if args.source.is_empty() { + &[RevisionArg::AT] + } else { + &args.source + })? + .evaluate_to_commit_ids()? + .collect(); + workspace_command.check_rewritable(root_commits.iter())?; let mut tx = workspace_command.start_transaction(); - // Collect all FileIds we're going to format and which commits they appear in - let commits: Vec<_> = RevsetExpression::commits(root_commits.iter().ids().cloned().collect()) + // Collect all of the unique `ToolInput`s we're going to use. Tools should be + // deterministic, and should not consider outside information, so it is safe to + // deduplicate inputs that correspond to multiple files or commits. This is + // typically more efficient, but it does prevent certain use cases like + // providing commit IDs as inputs to be inserted into files. We also need to + // record the mapping between tool inputs and paths/commits, to efficiently + // rewrite the commits later. + // + // If a path is being fixed in a particular commit, it must also be fixed in all + // that commit's descendants. We do this as a way of propagating changes, + // under the assumption that it is more useful than performing a rebase and + // risking merge conflicts. In the case of code formatters, rebasing wouldn't + // reliably produce well formatted code anyway. Deduplicating inputs helps + // to prevent quadratic growth in the number of tool executions required for + // doing this in long chains of commits with disjoint sets of modified files. + let commits: Vec<_> = RevsetExpression::commits(root_commits.to_vec()) .descendants() .evaluate_programmatic(tx.base_repo().as_ref())? .iter() .commits(tx.repo().store()) .try_collect()?; - let mut file_ids = HashSet::new(); - let mut commit_paths: HashMap> = HashMap::new(); + let mut unique_tool_inputs: HashSet = HashSet::new(); + let mut commit_paths: HashMap> = HashMap::new(); for commit in commits.iter().rev() { - let mut paths = vec![]; - // Paths modified in parent commits in the set should also be updated in this - // commit + 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_from_slice(parent_paths); + paths.extend(parent_paths.iter().cloned()); } } - let parent_tree = commit.parent_tree(tx.repo())?; + + // Also fix any new paths that were changed in this commit. let tree = commit.tree()?; + let parent_tree = commit.parent_tree(tx.repo())?; let mut diff_stream = parent_tree.diff_stream(&tree, &EverythingMatcher); async { while let Some((repo_path, diff)) = diff_stream.next().await { let (_before, after) = diff?; + // Deleted files have no file content to fix, and they have no terms in `after`, + // so we don't add any tool inputs for them. Conflicted files produce one tool + // input for each side of the conflict. for term in after.into_iter().flatten() { + // We currently only support fixing the content of normal files, so we skip + // directories and symlinks, and we ignore the executable bit. if let TreeValue::File { id, executable: _ } = term { - file_ids.insert((repo_path.clone(), id)); - paths.push(repo_path.clone()); + // TODO: Consider filename arguments and tool configuration instead of + // passing every changed file into the tool. Otherwise, the tool has to + // be modified to implement that kind of stuff. + let tool_input = ToolInput { + file_id: id.clone(), + repo_path: repo_path.clone(), + }; + unique_tool_inputs.insert(tool_input.clone()); + paths.insert(repo_path.clone()); } } } Ok::<(), BackendError>(()) } .block_on()?; + commit_paths.insert(commit.id().clone(), paths); } - let formatted = format_files(tx.repo().store().as_ref(), &file_ids)?; + // Run the configured tool on all of the chosen inputs. + // TODO: Support configuration of multiple tools and which files they affect. + let tool_command: CommandNameAndArgs = command + .settings() + .config() + .get("fix.tool-command") + .map_err(|err| config_error_with_message("Invalid `fix.tool-command`", err))?; + let fixed_file_ids = fix_file_ids( + tx.repo().store().as_ref(), + &tool_command, + &unique_tool_inputs, + )?; + // Substitute the fixed file IDs into all of the affected commits. Currently, + // fixes cannot delete or rename files, change the executable bit, or modify + // other parts of the commit like the description. + let mut num_checked_commits = 0; + let mut num_fixed_commits = 0; tx.mut_repo().transform_descendants( command.settings(), - root_commits.iter().ids().cloned().collect_vec(), + root_commits.iter().cloned().collect_vec(), |mut rewriter| { - let paths = commit_paths.get(rewriter.old_commit().id()).unwrap(); + // TODO: Build the trees in parallel before `transform_descendants()` and only + // keep the tree IDs in memory, so we can pass them to the rewriter. + let repo_paths = commit_paths.get(rewriter.old_commit().id()).unwrap(); let old_tree = rewriter.old_commit().tree()?; let mut tree_builder = MergedTreeBuilder::new(old_tree.id().clone()); - for path in paths { - let old_value = old_tree.path_value(path); + let mut changes = 0; + for repo_path in repo_paths { + let old_value = old_tree.path_value(repo_path)?; let new_value = old_value.map(|old_term| { if let Some(TreeValue::File { id, executable }) = old_term { - if let Some(new_id) = formatted.get(&(path, id)) { - Some(TreeValue::File { + let tool_input = ToolInput { + file_id: id.clone(), + repo_path: repo_path.clone(), + }; + if let Some(new_id) = fixed_file_ids.get(&tool_input) { + return Some(TreeValue::File { id: new_id.clone(), executable: *executable, - }) - } else { - old_term.clone() + }); } - } else { - old_term.clone() } + old_term.clone() }); if new_value != old_value { - tree_builder.set_or_remove(path.clone(), new_value); + tree_builder.set_or_remove(repo_path.clone(), new_value); + changes += 1; } } - let new_tree = tree_builder.write_tree(rewriter.mut_repo().store())?; - let builder = rewriter.reparent(command.settings())?; - builder.set_tree_id(new_tree).write()?; + num_checked_commits += 1; + if changes > 0 { + num_fixed_commits += 1; + let new_tree = tree_builder.write_tree(rewriter.mut_repo().store())?; + let builder = rewriter.reparent(command.settings())?; + builder.set_tree_id(new_tree).write()?; + } Ok(()) }, )?; + writeln!( + ui.status(), + "Fixed {num_fixed_commits} commits of {num_checked_commits} checked." + )?; + tx.finish(ui, format!("fixed {num_fixed_commits} commits")) +} - tx.finish(ui, format!("fixed {} commits", root_commits.len())) +/// Represents the API between `jj fix` and the tools it runs. +// TODO: Add the set of changed line/byte ranges, so those can be passed into code formatters via +// flags. This will help avoid introducing unrelated changes when working on code with out of date +// formatting. +#[derive(PartialEq, Eq, Hash, Clone)] +struct ToolInput { + /// File content is the primary input, provided on the tool's standard + /// input. We use the `FileId` as a placeholder here, so we can hold all + /// the inputs in memory without also holding all the content at once. + file_id: FileId, + + /// The path is provided to allow passing it into the tool so it can + /// potentially: + /// - Choose different behaviors for different file names, extensions, etc. + /// - Update parts of the file's content that should be derived from the + /// file's path. + repo_path: RepoPathBuf, } -fn format_files<'a>( +/// Applies `run_tool()` to the inputs and stores the resulting file content. +/// +/// Returns a map describing the subset of `tool_inputs` that resulted in +/// changed file content. Failures when handling an input will cause it to be +/// omitted from the return value, which is indistinguishable from succeeding +/// with no changes. +/// TODO: Better error handling so we can tell the user what went wrong with +/// each failed input. +fn fix_file_ids<'a>( store: &Store, - file_ids: &'a HashSet<(RepoPathBuf, FileId)>, -) -> BackendResult> { + tool_command: &CommandNameAndArgs, + tool_inputs: &'a HashSet, +) -> BackendResult> { + let (updates_tx, updates_rx) = channel(); + // TODO: Switch to futures, or document the decision not to. We don't need + // threads unless the threads will be doing more than waiting for pipes. + tool_inputs.into_par_iter().try_for_each_init( + || updates_tx.clone(), + |updates_tx, tool_input| -> Result<(), BackendError> { + let mut read = store.read_file(&tool_input.repo_path, &tool_input.file_id)?; + let mut old_content = vec![]; + read.read_to_end(&mut old_content).unwrap(); + if let Ok(new_content) = run_tool(tool_command, tool_input, &old_content) { + if new_content != *old_content { + let new_file_id = + store.write_file(&tool_input.repo_path, &mut new_content.as_slice())?; + updates_tx.send((tool_input, new_file_id)).unwrap(); + } + } + Ok(()) + }, + )?; + drop(updates_tx); let mut result = HashMap::new(); - for (path, id) in file_ids { - // TODO: read asynchronously - let mut read = store.read_file(path, id)?; - let mut buf = vec![]; - read.read_to_end(&mut buf).unwrap(); - // TODO: Call a formatter instead of just uppercasing - for b in &mut buf { - b.make_ascii_uppercase(); - } - // TODO: Don't write it if it didn't change - let formatted_id = store.write_file(path, &mut buf.as_slice())?; - result.insert((path, id), formatted_id); + while let Ok((tool_input, new_file_id)) = updates_rx.recv() { + result.insert(tool_input, new_file_id); } Ok(result) } + +/// Runs the `tool_command` to fix the given file content. +/// +/// The `old_content` is assumed to be that of the `tool_input`'s `FileId`, but +/// this is not verified. +/// +/// Returns the new file content, whose value will be the same as `old_content` +/// unless the command introduced changes. Returns `None` if there were any +/// failures when starting, stopping, or communicating with the subprocess. +fn run_tool( + tool_command: &CommandNameAndArgs, + tool_input: &ToolInput, + old_content: &[u8], +) -> Result, ()> { + // TODO: Pipe stderr so we can tell the user which commit, file, and tool it is + // associated with. + let mut vars: HashMap<&str, &str> = HashMap::new(); + vars.insert("path", tool_input.repo_path.as_internal_file_string()); + let mut child = tool_command + .to_command_with_variables(&vars) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .spawn() + .or(Err(()))?; + let mut stdin = child.stdin.take().unwrap(); + let output = std::thread::scope(|s| { + s.spawn(move || { + stdin.write_all(old_content).ok(); + }); + Some(child.wait_with_output().or(Err(()))) + }) + .unwrap()?; + if output.status.success() { + Ok(output.stdout) + } else { + Err(()) + } +} diff --git a/cli/src/commands/mod.rs b/cli/src/commands/mod.rs index e479b8e807..188147ad59 100644 --- a/cli/src/commands/mod.rs +++ b/cli/src/commands/mod.rs @@ -93,7 +93,6 @@ enum Command { Duplicate(duplicate::DuplicateArgs), Edit(edit::EditArgs), Files(files::FilesArgs), - #[command(hide = true)] Fix(fix::FixArgs), #[command(subcommand)] Git(git::GitCommand), diff --git a/cli/src/config-schema.json b/cli/src/config-schema.json index 7ccb03bd9e..d9267ac88c 100644 --- a/cli/src/config-schema.json +++ b/cli/src/config-schema.json @@ -482,6 +482,19 @@ "additionalProperties": true } } + }, + "fix": { + "type": "object", + "description": "Settings for jj fix", + "properties": { + "tool-command": { + "type": "array", + "items": { + "type": "string" + }, + "description": "Shell command that takes file content on stdin and returns fixed file content on stdout" + } + } } } } diff --git a/cli/tests/cli-reference@.md.snap b/cli/tests/cli-reference@.md.snap index 9df2ab8a16..a2be897b00 100644 --- a/cli/tests/cli-reference@.md.snap +++ b/cli/tests/cli-reference@.md.snap @@ -37,6 +37,7 @@ This document contains the help content for the `jj` command-line program. * [`jj duplicate`↴](#jj-duplicate) * [`jj edit`↴](#jj-edit) * [`jj files`↴](#jj-files) +* [`jj fix`↴](#jj-fix) * [`jj git`↴](#jj-git) * [`jj git remote`↴](#jj-git-remote) * [`jj git remote add`↴](#jj-git-remote-add) @@ -117,6 +118,7 @@ To get started, see the tutorial at https://github.com/martinvonz/jj/blob/main/d * `duplicate` — Create a new change with the same content as an existing one * `edit` — Sets the specified revision as the working-copy revision * `files` — List files in a revision +* `fix` — Update files with formatting fixes or other changes * `git` — Commands for working with Git remotes and the underlying Git repo * `init` — Create a new repo in the given directory * `interdiff` — Compare the changes of two commits @@ -758,6 +760,44 @@ List files in a revision +## `jj fix` + +Update files with formatting fixes or other changes + +The primary use case for this command is to apply the results of automatic +code formatting tools to revisions that may not be properly formatted yet. +It can also be used to modify files with other tools like `sed` or `sort`. + +The changed files in the given revisions will be updated with any fixes +determined by passing their file content through the external tool. +Descendants will also be updated by passing their versions of the same files +through the same external tool, which will never result in new conflicts. +Files with existing conflicts will be updated on all sides of the conflict, +which can potentially increase or decrease the number of conflict markers. + +The external tool must accept the current file content on standard input, +and return the updated file content on standard output. The output will not +be used unless the tool exits with a successful exit code. Output on +standard error will be passed through to the terminal. + +The configuration schema is expected to change in the future. For now, it +defines a single command that will affect all changed files in the specified +revisions. For example, to format some Rust code changed in the working copy +revision, you could write this configuration: + +[fix] +tool-command = ["rustfmt", "--emit", "stdout"] + +And then run the command `jj fix -s @`. + +**Usage:** `jj fix [OPTIONS]` + +###### **Options:** + +* `-s`, `--source ` — Fix files in the specified revision(s) and their descendants + + + ## `jj git` Commands for working with Git remotes and the underlying Git repo diff --git a/cli/tests/runner.rs b/cli/tests/runner.rs index b0a207254d..afa39c3773 100644 --- a/cli/tests/runner.rs +++ b/cli/tests/runner.rs @@ -27,6 +27,7 @@ mod test_diff_command; mod test_diffedit_command; mod test_duplicate_command; mod test_edit_command; +mod test_fix_command; mod test_generate_md_cli_help; mod test_git_clone; mod test_git_colocated; diff --git a/cli/tests/test_fix_command.rs b/cli/tests/test_fix_command.rs new file mode 100644 index 0000000000..2b42948fea --- /dev/null +++ b/cli/tests/test_fix_command.rs @@ -0,0 +1,524 @@ +// Copyright 2024 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. + +#[cfg(unix)] +use std::os::unix::fs::PermissionsExt; +use std::path::PathBuf; + +use itertools::Itertools; +use jj_lib::file_util::try_symlink; + +use crate::common::TestEnvironment; + +/// Set up a repo where the `jj fix` command uses the fake editor with the given +/// flags. +fn init_with_fake_formatter(args: &[&str]) -> (TestEnvironment, PathBuf) { + 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"\\"); + test_env.add_config(&format!( + r#"fix.tool-command = ["{}"]"#, + [escaped_formatter_path.as_str()] + .iter() + .chain(args) + .join(r#"", ""#) + )); + (test_env, repo_path) +} + +#[test] +fn test_fix_no_config() { + 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"); + let stderr = test_env.jj_cmd_failure(&repo_path, &["fix", "-s", "@"]); + insta::assert_snapshot!(stderr, @r###" + Config error: Invalid `fix.tool-command` + Caused by: configuration property "fix.tool-command" not found + For help, see https://github.com/martinvonz/jj/blob/main/docs/config.md. + "###); +} + +#[test] +fn test_fix_empty_commit() { + let (test_env, repo_path) = init_with_fake_formatter(&["--uppercase"]); + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["fix", "-s", "@"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Fixed 0 commits of 1 checked. + Nothing changed. + "###); +} + +#[test] +fn test_fix_leaf_commit() { + let (test_env, repo_path) = init_with_fake_formatter(&["--uppercase"]); + std::fs::write(repo_path.join("file"), "unaffected").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["new"]); + std::fs::write(repo_path.join("file"), "affected").unwrap(); + + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["fix", "-s", "@"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Fixed 1 commits of 1 checked. + Working copy now at: rlvkpnrz 8b02703b (no description set) + Parent commit : qpvuntsm fda57e40 (no description set) + Added 0 files, modified 1 files, removed 0 files + "###); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file", "-r", "@-"]); + insta::assert_snapshot!(content, @"unaffected"); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file", "-r", "@"]); + insta::assert_snapshot!(content, @"AFFECTED"); +} + +#[test] +fn test_fix_parent_commit() { + let (test_env, repo_path) = init_with_fake_formatter(&["--uppercase"]); + // Using one file name for all commits adds coverage of some possible bugs. + std::fs::write(repo_path.join("file"), "parent").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["branch", "create", "parent"]); + test_env.jj_cmd_ok(&repo_path, &["new"]); + std::fs::write(repo_path.join("file"), "child1").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["branch", "create", "child1"]); + test_env.jj_cmd_ok(&repo_path, &["new", "-r", "parent"]); + std::fs::write(repo_path.join("file"), "child2").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["branch", "create", "child2"]); + + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["fix", "-s", "parent"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Fixed 3 commits of 3 checked. + Working copy now at: mzvwutvl d6abb1f4 child2 | (no description set) + Parent commit : qpvuntsm 4f4d2103 parent | (no description set) + Added 0 files, modified 1 files, removed 0 files + "###); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file", "-r", "parent"]); + insta::assert_snapshot!(content, @"PARENT"); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file", "-r", "child1"]); + insta::assert_snapshot!(content, @"CHILD1"); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file", "-r", "child2"]); + insta::assert_snapshot!(content, @"CHILD2"); +} + +#[test] +fn test_fix_sibling_commit() { + let (test_env, repo_path) = init_with_fake_formatter(&["--uppercase"]); + std::fs::write(repo_path.join("file"), "parent").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["branch", "create", "parent"]); + test_env.jj_cmd_ok(&repo_path, &["new"]); + std::fs::write(repo_path.join("file"), "child1").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["branch", "create", "child1"]); + test_env.jj_cmd_ok(&repo_path, &["new", "-r", "parent"]); + std::fs::write(repo_path.join("file"), "child2").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["branch", "create", "child2"]); + + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["fix", "-s", "child1"]); + 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", "parent"]); + insta::assert_snapshot!(content, @"parent"); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file", "-r", "child1"]); + insta::assert_snapshot!(content, @"CHILD1"); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file", "-r", "child2"]); + insta::assert_snapshot!(content, @"child2"); +} + +#[test] +fn test_fix_immutable_commit() { + let (test_env, repo_path) = init_with_fake_formatter(&["--uppercase"]); + std::fs::write(repo_path.join("file"), "immutable").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["branch", "create", "immutable"]); + test_env.jj_cmd_ok(&repo_path, &["new"]); + std::fs::write(repo_path.join("file"), "mutable").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["branch", "create", "mutable"]); + test_env.add_config(r#"revset-aliases."immutable_heads()" = "immutable""#); + + let stderr = test_env.jj_cmd_failure(&repo_path, &["fix", "-s", "immutable"]); + insta::assert_snapshot!(stderr, @r###" + Error: Commit 83eee3c8dce2 is immutable + Hint: Pass `--ignore-immutable` or configure the set of immutable commits via `revset-aliases.immutable_heads()`. + "###); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file", "-r", "immutable"]); + insta::assert_snapshot!(content, @"immutable"); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file", "-r", "mutable"]); + insta::assert_snapshot!(content, @"mutable"); +} + +#[test] +fn test_fix_empty_file() { + let (test_env, repo_path) = init_with_fake_formatter(&["--uppercase"]); + std::fs::write(repo_path.join("file"), "").unwrap(); + + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["fix", "-s", "@"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Fixed 0 commits of 1 checked. + Nothing changed. + "###); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file", "-r", "@"]); + insta::assert_snapshot!(content, @""); +} + +#[test] +fn test_fix_cyclic() { + let (test_env, repo_path) = init_with_fake_formatter(&["--reverse"]); + std::fs::write(repo_path.join("file"), "content\n").unwrap(); + + 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: qpvuntsm affcf432 (no description set) + Parent commit : zzzzzzzz 00000000 (empty) (no description set) + Added 0 files, modified 1 files, removed 0 files + "###); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file", "-r", "@"]); + insta::assert_snapshot!(content, @"tnetnoc\n"); + + 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: qpvuntsm 2de05835 (no description set) + Parent commit : zzzzzzzz 00000000 (empty) (no description set) + Added 0 files, modified 1 files, removed 0 files + "###); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file", "-r", "@"]); + insta::assert_snapshot!(content, @"content\n"); +} + +#[test] +fn test_deduplication() { + // Append all fixed content to a log file. This assumes we're running the tool + // in the root directory of the repo, which is worth reconsidering if we + // establish a contract regarding cwd. + let (test_env, repo_path) = init_with_fake_formatter(&["--uppercase", "--tee", "$path-fixlog"]); + + // There are at least two interesting cases: the content is repeated immediately + // in the child commit, or later in another descendant. + std::fs::write(repo_path.join("file"), "foo\n").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["branch", "create", "a"]); + test_env.jj_cmd_ok(&repo_path, &["new"]); + std::fs::write(repo_path.join("file"), "bar\n").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["branch", "create", "b"]); + test_env.jj_cmd_ok(&repo_path, &["new"]); + std::fs::write(repo_path.join("file"), "bar\n").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["branch", "create", "c"]); + test_env.jj_cmd_ok(&repo_path, &["new"]); + std::fs::write(repo_path.join("file"), "foo\n").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["branch", "create", "d"]); + + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["fix", "-s", "a"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Fixed 4 commits of 4 checked. + Working copy now at: yqosqzyt 5ac0edc4 d | (no description set) + Parent commit : mzvwutvl 90d9a032 c | (empty) (no description set) + Added 0 files, modified 1 files, removed 0 files + "###); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file", "-r", "a"]); + insta::assert_snapshot!(content, @"FOO\n"); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file", "-r", "b"]); + insta::assert_snapshot!(content, @"BAR\n"); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file", "-r", "c"]); + insta::assert_snapshot!(content, @"BAR\n"); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file", "-r", "d"]); + insta::assert_snapshot!(content, @"FOO\n"); + + // Each new content string only appears once in the log, because all the other + // inputs (like file name) were identical, and so the results were re-used. We + // sort the log because the order of execution inside `jj fix` is undefined. + insta::assert_snapshot!(sorted_lines(repo_path.join("file-fixlog")), @"BAR\nFOO\n"); +} + +fn sorted_lines(path: PathBuf) -> String { + let mut log: Vec<_> = std::fs::read_to_string(path.as_os_str()) + .unwrap() + .lines() + .map(String::from) + .collect(); + log.sort(); + log.join("\n") +} + +#[test] +fn test_executed_but_nothing_changed() { + // Show that the tool ran by causing a side effect with --tee, and test that we + // do the right thing when the tool's output is exactly equal to its input. + let (test_env, repo_path) = init_with_fake_formatter(&["--tee", "$path-copy"]); + std::fs::write(repo_path.join("file"), "content\n").unwrap(); + + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["fix", "-s", "@"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Fixed 0 commits of 1 checked. + Nothing changed. + "###); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file", "-r", "@"]); + insta::assert_snapshot!(content, @"content\n"); + let copy_content = std::fs::read_to_string(repo_path.join("file-copy").as_os_str()).unwrap(); + insta::assert_snapshot!(copy_content, @"content\n"); +} + +#[test] +fn test_failure() { + let (test_env, repo_path) = init_with_fake_formatter(&["--fail"]); + std::fs::write(repo_path.join("file"), "content").unwrap(); + + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["fix", "-s", "@"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Fixed 0 commits of 1 checked. + Nothing changed. + "###); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file", "-r", "@"]); + insta::assert_snapshot!(content, @"content"); +} + +#[test] +fn test_stderr_success() { + let (test_env, repo_path) = + init_with_fake_formatter(&["--stderr", "error", "--stdout", "new content"]); + std::fs::write(repo_path.join("file"), "old content").unwrap(); + + // TODO: Associate the stderr lines with the relevant tool/file/commit instead + // of passing it through directly. + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["fix", "-s", "@"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + errorFixed 1 commits of 1 checked. + Working copy now at: qpvuntsm e8c5cda3 (no description set) + Parent commit : zzzzzzzz 00000000 (empty) (no description set) + Added 0 files, modified 1 files, removed 0 files + "###); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file", "-r", "@"]); + insta::assert_snapshot!(content, @"new content"); +} + +#[test] +fn test_stderr_failure() { + let (test_env, repo_path) = + init_with_fake_formatter(&["--stderr", "error", "--stdout", "new content", "--fail"]); + std::fs::write(repo_path.join("file"), "old content").unwrap(); + + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["fix", "-s", "@"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + errorFixed 0 commits of 1 checked. + Nothing changed. + "###); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file", "-r", "@"]); + insta::assert_snapshot!(content, @"old content"); +} + +#[test] +fn test_missing_command() { + 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.add_config(r#"fix.tool-command = ["this_executable_shouldnt_exist"]"#); + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["fix", "-s", "@"]); + insta::assert_snapshot!(stdout, @""); + // TODO: We should display a warning about invalid tool configurations. When we + // support multiple tools, we should also keep going to see if any of the other + // executions succeed. + insta::assert_snapshot!(stderr, @r###" + Fixed 0 commits of 1 checked. + Nothing changed. + "###); +} + +#[test] +fn test_fix_file_types() { + let (test_env, repo_path) = init_with_fake_formatter(&["--uppercase"]); + std::fs::write(repo_path.join("file"), "content").unwrap(); + std::fs::create_dir(repo_path.join("dir")).unwrap(); + try_symlink("file", repo_path.join("link")).unwrap(); + + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["fix", "-s", "@"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Fixed 1 commits of 1 checked. + Working copy now at: qpvuntsm 72bf7048 (no description set) + Parent commit : zzzzzzzz 00000000 (empty) (no description set) + Added 0 files, modified 1 files, removed 0 files + "###); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file", "-r", "@"]); + insta::assert_snapshot!(content, @"CONTENT"); +} + +#[cfg(unix)] +#[test] +fn test_fix_executable() { + let (test_env, repo_path) = init_with_fake_formatter(&["--uppercase"]); + let path = repo_path.join("file"); + std::fs::write(&path, "content").unwrap(); + let mut permissions = std::fs::metadata(&path).unwrap().permissions(); + permissions.set_mode(permissions.mode() | 0o111); + std::fs::set_permissions(&path, permissions).unwrap(); + + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["fix", "-s", "@"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Fixed 1 commits of 1 checked. + Working copy now at: qpvuntsm eea49ac9 (no description set) + Parent commit : zzzzzzzz 00000000 (empty) (no description set) + Added 0 files, modified 1 files, removed 0 files + "###); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file", "-r", "@"]); + insta::assert_snapshot!(content, @"CONTENT"); + let executable = std::fs::metadata(&path).unwrap().permissions().mode() & 0o111; + assert_eq!(executable, 0o111); +} + +#[test] +fn test_fix_trivial_merge_commit() { + // All the changes are attributable to a parent, so none are fixed (in the same + // way that none would be shown in `jj diff -r @`). + let (test_env, repo_path) = init_with_fake_formatter(&["--uppercase"]); + std::fs::write(repo_path.join("file_a"), "content a").unwrap(); + std::fs::write(repo_path.join("file_c"), "content c").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["branch", "create", "a"]); + test_env.jj_cmd_ok(&repo_path, &["new", "@-"]); + std::fs::write(repo_path.join("file_b"), "content b").unwrap(); + std::fs::write(repo_path.join("file_c"), "content c").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["branch", "create", "b"]); + test_env.jj_cmd_ok(&repo_path, &["new", "a", "b"]); + + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["fix", "-s", "@"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Fixed 0 commits of 1 checked. + Nothing changed. + "###); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file_a", "-r", "@"]); + insta::assert_snapshot!(content, @"content a"); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file_b", "-r", "@"]); + insta::assert_snapshot!(content, @"content b"); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file_c", "-r", "@"]); + insta::assert_snapshot!(content, @"content c"); +} + +#[test] +fn test_fix_adding_merge_commit() { + // None of the changes are attributable to a parent, so they are all fixed (in + // the same way that they would be shown in `jj diff -r @`). + let (test_env, repo_path) = init_with_fake_formatter(&["--uppercase"]); + std::fs::write(repo_path.join("file_a"), "content a").unwrap(); + std::fs::write(repo_path.join("file_c"), "content c").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["branch", "create", "a"]); + test_env.jj_cmd_ok(&repo_path, &["new", "@-"]); + std::fs::write(repo_path.join("file_b"), "content b").unwrap(); + std::fs::write(repo_path.join("file_c"), "content c").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["branch", "create", "b"]); + test_env.jj_cmd_ok(&repo_path, &["new", "a", "b"]); + std::fs::write(repo_path.join("file_a"), "change a").unwrap(); + std::fs::write(repo_path.join("file_b"), "change b").unwrap(); + std::fs::write(repo_path.join("file_c"), "change c").unwrap(); + std::fs::write(repo_path.join("file_d"), "change d").unwrap(); + + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["fix", "-s", "@"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Fixed 1 commits of 1 checked. + Working copy now at: mzvwutvl 899a1398 (no description set) + Parent commit : qpvuntsm 34782c48 a | (no description set) + Parent commit : kkmpptxz 82e9bc6a b | (no description set) + Added 0 files, modified 4 files, removed 0 files + "###); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file_a", "-r", "@"]); + insta::assert_snapshot!(content, @"CHANGE A"); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file_b", "-r", "@"]); + insta::assert_snapshot!(content, @"CHANGE B"); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file_c", "-r", "@"]); + insta::assert_snapshot!(content, @"CHANGE C"); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file_d", "-r", "@"]); + insta::assert_snapshot!(content, @"CHANGE D"); +} + +#[test] +fn test_fix_both_sides_of_conflict() { + let (test_env, repo_path) = init_with_fake_formatter(&["--uppercase"]); + std::fs::write(repo_path.join("file"), "content a\n").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["branch", "create", "a"]); + test_env.jj_cmd_ok(&repo_path, &["new", "@-"]); + std::fs::write(repo_path.join("file"), "content b\n").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["branch", "create", "b"]); + test_env.jj_cmd_ok(&repo_path, &["new", "a", "b"]); + + // The conflicts are not different from the merged parent, so they would not be + // fixed if we didn't fix the parents also. + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["fix", "-s", "a", "-s", "b"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Fixed 3 commits of 3 checked. + Working copy now at: mzvwutvl b7967885 (conflict) (empty) (no description set) + Parent commit : qpvuntsm 06fe435a a | (no description set) + Parent commit : kkmpptxz ce7ee79e b | (no description set) + Added 0 files, modified 1 files, removed 0 files + There are unresolved conflicts at these paths: + file 2-sided conflict + "###); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file", "-r", "a"]); + insta::assert_snapshot!(content, @r###" + CONTENT A + "###); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file", "-r", "b"]); + insta::assert_snapshot!(content, @r###" + CONTENT B + "###); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file", "-r", "@"]); + insta::assert_snapshot!(content, @r###" + <<<<<<< Conflict 1 of 1 + %%%%%%% Changes from base to side #1 + +CONTENT A + +++++++ Contents of side #2 + CONTENT B + >>>>>>> Conflict 1 of 1 ends + "###); +} + +#[test] +fn test_fix_resolve_conflict() { + // If both sides of the conflict look the same after being fixed, the conflict + // will be resolved. + let (test_env, repo_path) = init_with_fake_formatter(&["--uppercase"]); + std::fs::write(repo_path.join("file"), "Content\n").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["branch", "create", "a"]); + test_env.jj_cmd_ok(&repo_path, &["new", "@-"]); + std::fs::write(repo_path.join("file"), "cOnTeNt\n").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["branch", "create", "b"]); + test_env.jj_cmd_ok(&repo_path, &["new", "a", "b"]); + + // The conflicts are not different from the merged parent, so they would not be + // fixed if we didn't fix the parents also. + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["fix", "-s", "a", "-s", "b"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Fixed 3 commits of 3 checked. + Working copy now at: mzvwutvl 98dec555 (conflict) (no description set) + Parent commit : qpvuntsm 3c63716f a | (no description set) + Parent commit : kkmpptxz 82703f5e b | (no description set) + Added 0 files, modified 1 files, removed 0 files + "###); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file", "-r", "@"]); + insta::assert_snapshot!(content, @r###" + CONTENT + "###); +} diff --git a/cli/tests/test_util_command.rs b/cli/tests/test_util_command.rs index a5ccb85036..c304b5c761 100644 --- a/cli/tests/test_util_command.rs +++ b/cli/tests/test_util_command.rs @@ -30,6 +30,8 @@ fn test_util_config_schema() { "description": "User configuration for Jujutsu VCS. See https://github.com/martinvonz/jj/blob/main/docs/config.md for details", "properties": { [...] + "fix": { + [...] } } "###)