From 3c119a4f135659c04c6f373fb9912dcc363a4c97 Mon Sep 17 00:00:00 2001 From: Danny Hooper Date: Wed, 15 May 2024 17:19:55 -0500 Subject: [PATCH] cli: implement enough of `jj fix` to run a single tool on all files --- Cargo.lock | 1 + cli/Cargo.toml | 1 + cli/src/commands/fix.rs | 277 +++++++++++++---- cli/src/commands/mod.rs | 1 - cli/src/config-schema.json | 10 + cli/testing/fake-formatter.rs | 2 +- cli/tests/cli-reference@.md.snap | 20 ++ cli/tests/runner.rs | 1 + cli/tests/test_fix_command.rs | 513 +++++++++++++++++++++++++++++++ cli/tests/test_util_command.rs | 2 + 10 files changed, 771 insertions(+), 57 deletions(-) create mode 100644 cli/tests/test_fix_command.rs diff --git a/Cargo.lock b/Cargo.lock index f6ba68c531..d3aa327991 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1700,6 +1700,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..8441e5ba9c 100644 --- a/cli/src/commands/fix.rs +++ b/cli/src/commands/fix.rs @@ -13,6 +13,10 @@ // limitations under the License. use std::collections::{HashMap, HashSet}; +use std::io::Write; +use std::process::{Command, Stdio}; +use std::sync::mpsc::channel; +use std::sync::Arc; use futures::StreamExt; use itertools::Itertools; @@ -25,17 +29,34 @@ 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::cli_util::{CommandHelper, RevisionArg, WorkspaceCommandTransaction}; use crate::command_error::CommandError; 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 ignored. #[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, } @@ -48,106 +69,252 @@ pub(crate) fn cmd_fix( ) -> Result<(), CommandError> { let mut workspace_command = command.workspace_helper(ui)?; let root_commits: Vec = workspace_command - .parse_union_revsets(&args.sources)? + .parse_union_revsets(if args.sources.is_empty() { + &[RevisionArg::AT] + } else { + &args.sources + })? .evaluate_to_commits()? .try_collect()?; workspace_command.check_rewritable(root_commits.iter().ids())?; - let mut tx = workspace_command.start_transaction(); + let mut tx: WorkspaceCommandTransaction = workspace_command.start_transaction(); - // Collect all FileIds we're going to format and which commits they appear in + // Collect all of the unique tool inputs 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.iter().ids().cloned().collect()) .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 all_tool_inputs: HashSet = HashSet::new(); + let mut commit_tool_inputs: 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 - for parent_id in commit.parent_ids() { - if let Some(parent_paths) = commit_paths.get(parent_id) { - paths.extend_from_slice(parent_paths); - } - } - let parent_tree = commit.parent_tree(tx.repo())?; + // Fix paths that are different from all parents. This means that files in merge + // commits are only fixed if they have evil changes, or if a parent was fixed. + let mut paths_to_fix: HashSet = HashSet::new(); 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() { - if let TreeValue::File { id, executable: _ } = term { - file_ids.insert((repo_path.clone(), id)); - paths.push(repo_path.clone()); + // 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 + { + // 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. + paths_to_fix.insert(repo_path.clone()); } } } Ok::<(), BackendError>(()) } .block_on()?; - commit_paths.insert(commit.id().clone(), paths); + + // Also fix all paths that were fixed in ancestors, so we don't lose those + // changes. We do this instead of rebasing onto those changes. + for parent_id in commit.parent_ids() { + if let Some(parent_tool_inputs) = commit_tool_inputs.get(parent_id) { + for repo_path in parent_tool_inputs.keys() { + paths_to_fix.insert(repo_path.clone()); + } + } + } + + // Get this commit's file IDs for the paths to be fixed. Conflicting files will + // have multiple versions to be fixed, so we map paths to a set of inputs. + let mut tool_inputs_map: HashMap> = HashMap::new(); + for repo_path in paths_to_fix { + let value = tree.path_value(&repo_path); + for term in value.iter().flatten() { + if let TreeValue::File { id, executable: _ } = term { + let tool_input = ToolInput { + file_id: id.clone(), + repo_path: repo_path.clone(), + }; + all_tool_inputs.insert(tool_input.clone()); + tool_inputs_map + .entry(repo_path.clone()) + .or_default() + .push(tool_input.clone()); + } + } + } + commit_tool_inputs.insert(commit.id().clone(), tool_inputs_map); } - 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 = command.settings().config().get_string("fix.tool-command")?; + let fixed_file_ids = + fixed_file_ids(tx.repo().store().as_ref(), &tool_command, &all_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(), |mut rewriter| { - let paths = commit_paths.get(rewriter.old_commit().id()).unwrap(); + let tool_inputs_map = commit_tool_inputs.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, tool_inputs) in tool_inputs_map { + 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 { - id: new_id.clone(), - executable: *executable, - }) - } else { - old_term.clone() + // We could use a map from N ToolInputs to file IDs, but N is likely small. + for tool_input in tool_inputs { + if tool_input.file_id == *id { + 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() } + 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")) +} + +/// 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 + // file contents at once. + file_id: FileId, - tx.finish(ui, format!("fixed {} commits", root_commits.len())) + // 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 fixed_file_ids<'a>( store: &Store, - file_ids: &'a HashSet<(RepoPathBuf, FileId)>, -) -> BackendResult> { + tool_command: &str, + tool_inputs: &'a HashSet, +) -> BackendResult> { + let (updates_tx, updates_rx) = channel(); + 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(); + let old_content = Arc::new(old_content); + if let Some(new_content) = run_tool(tool_command, tool_input, old_content.clone()) { + 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 given shell command to fix the given file content. +/// +/// The old_content is assumed to be that of the tool_input's FileId, but it 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. +/// Takes shared ownership of old_content for the duration of the call, to help +/// avoid cloning it or deadlocking the subprocess I/O. +fn run_tool( + shell_command: &str, + tool_input: &ToolInput, + old_content: Arc>, +) -> Option> { + // TODO: Pipe stderr so we can tell the user which commit, file, and tool it is + // associated with. + let mut child = Command::new("sh") + .arg("-c") + .arg(shell_command.replace("{path}", tool_input.repo_path.as_internal_file_string())) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .spawn() + .ok()?; + let mut stdin = child.stdin.take()?; + std::thread::spawn(move || { + stdin.write_all(&old_content).ok(); + }); + let output = child.wait_with_output().ok()?; + if output.status.success() { + Some(output.stdout) + } else { + None + } +} diff --git a/cli/src/commands/mod.rs b/cli/src/commands/mod.rs index 7a0af85977..53aa490d63 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..c9c72a0a44 100644 --- a/cli/src/config-schema.json +++ b/cli/src/config-schema.json @@ -482,6 +482,16 @@ "additionalProperties": true } } + }, + "fix": { + "type": "object", + "description": "Settings for jj fix", + "properties": { + "tool-command": { + "type": "string", + "description": "Shell command that takes file content on stdin and returns fixed file content on stdout" + } + } } } } diff --git a/cli/testing/fake-formatter.rs b/cli/testing/fake-formatter.rs index aa06c89c16..29669da9f6 100644 --- a/cli/testing/fake-formatter.rs +++ b/cli/testing/fake-formatter.rs @@ -36,7 +36,7 @@ struct Args { #[arg(long, default_value_t = false)] fail: bool, - /// Reverse the characters in each line when reading stdin + /// Reverse the characters in each line when reading stdin. #[arg(long, default_value_t = false)] reverse: bool, diff --git a/cli/tests/cli-reference@.md.snap b/cli/tests/cli-reference@.md.snap index c74cbbfa50..86d9595c15 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) @@ -118,6 +119,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 the underlying Git repo * `init` — Create a new repo in the given directory * `interdiff` — Compare the changes of two commits @@ -760,6 +762,24 @@ 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 ignored. + +**Usage:** `jj fix [OPTIONS]` + +###### **Options:** + +* `-s`, `--sources ` — Fix files in the specified revision(s) and their descendants + + + ## `jj git` Commands for working with the underlying Git repo diff --git a/cli/tests/runner.rs b/cli/tests/runner.rs index 38134d8a0e..87629e64df 100644 --- a/cli/tests/runner.rs +++ b/cli/tests/runner.rs @@ -26,6 +26,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..1984a1cb2a --- /dev/null +++ b/cli/tests/test_fix_command.rs @@ -0,0 +1,513 @@ +// 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. + +use std::os::unix::fs::PermissionsExt; +use std::path::PathBuf; + +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(), &["init", "repo", "--git"]); + 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, &args + )); + (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: 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("--reverse"); + 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("--reverse"); + 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 c6c12b8c (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, @"detceffa"); +} + +#[test] +fn test_fix_parent_commit() { + let (test_env, repo_path) = init_with_fake_formatter("--reverse"); + // 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 f20be793 child2 | (no description set) + Parent commit : qpvuntsm ebd65bb6 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, @"tnerap"); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file", "-r", "child1"]); + insta::assert_snapshot!(content, @"1dlihc"); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file", "-r", "child2"]); + insta::assert_snapshot!(content, @"2dlihc"); +} + +#[test] +fn test_fix_sibling_commit() { + let (test_env, repo_path) = init_with_fake_formatter("--reverse"); + 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, @"1dlihc"); + 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("--reverse"); + 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("--reverse"); + 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("--reverse --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 bcb48084 d | (no description set) + Parent commit : mzvwutvl c09b05f5 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, @"oof\n"); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file", "-r", "b"]); + insta::assert_snapshot!(content, @"rab\n"); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file", "-r", "c"]); + insta::assert_snapshot!(content, @"rab\n"); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file", "-r", "d"]); + insta::assert_snapshot!(content, @"oof\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")), @"oof\nrab\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_no_op() { + let (test_env, repo_path) = init_with_fake_formatter(""); + 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 1 commits of 1 checked. + Working copy now at: qpvuntsm 38946f90 (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"); +} + +#[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("--reverse"); + 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 b277bd2a (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"); +} + +#[test] +fn test_fix_executable() { + let (test_env, repo_path) = init_with_fake_formatter("--reverse"); + 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 8149af23 (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"); + 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 @`). + let (test_env, repo_path) = init_with_fake_formatter("--reverse"); + 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_evil_merge_commit() { + // None of the changes are attributable to a parent, so they are all fixed. + let (test_env, repo_path) = init_with_fake_formatter("--reverse"); + 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"), "evil a").unwrap(); + std::fs::write(repo_path.join("file_b"), "evil b").unwrap(); + std::fs::write(repo_path.join("file_c"), "evil c").unwrap(); + std::fs::write(repo_path.join("file_d"), "evil 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 43aa7c45 (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, @"a live"); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file_b", "-r", "@"]); + insta::assert_snapshot!(content, @"b live"); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file_c", "-r", "@"]); + insta::assert_snapshot!(content, @"c live"); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file_d", "-r", "@"]); + insta::assert_snapshot!(content, @"d live"); +} + +#[test] +fn test_fix_both_sides_of_conflict() { + let (test_env, repo_path) = init_with_fake_formatter("--reverse"); + 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 36064b9c (conflict) (empty) (no description set) + Parent commit : qpvuntsm eb631ae2 a | (no description set) + Parent commit : kkmpptxz 9c4b133b 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###" + a tnetnoc + "###); + let content = test_env.jj_cmd_success(&repo_path, &["print", "file", "-r", "b"]); + insta::assert_snapshot!(content, @r###" + b tnetnoc + "###); + 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 + +a tnetnoc + +++++++ Contents of side #2 + b tnetnoc + >>>>>>> + "###); +} + +#[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("--stdout 'new content'"); + 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 9c3c9a13 (conflict) (no description set) + Parent commit : qpvuntsm 47e1f1d4 a | (no description set) + Parent commit : kkmpptxz dbfec95a 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###" + new 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": { + [...] } } "###)