diff --git a/CHANGELOG.md b/CHANGELOG.md index 35af57788e..0a8510dd86 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,10 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ### Deprecations +* The original configuration syntax for `jj fix` is now deprecated in favor of + one that allows defining multiple tools that can affect different filesets. + These can be used in combination for now. See `jj help fix` for details. + ### New features * External diff tools can now be configured to invoke the tool on each file @@ -56,6 +60,10 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). * The `file()` revset function now accepts fileset as argument. +* `jj fix` can now be configured to run different tools on different filesets. + This simplifies the use case of configuring code formatters for specific file + types. See `jj help fix` for details. + ### Fixed bugs * `jj diff --git` no longer shows the contents of binary files. diff --git a/cli/src/commands/fix.rs b/cli/src/commands/fix.rs index 590dfb9948..bab16b5222 100644 --- a/cli/src/commands/fix.rs +++ b/cli/src/commands/fix.rs @@ -13,16 +13,19 @@ // limitations under the License. use std::collections::{HashMap, HashSet}; +use std::hash::Hash; 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::backend::{BackendError, CommitId, FileId, TreeValue}; +use jj_lib::fileset::{self, FilesetExpression}; +use jj_lib::matchers::{EverythingMatcher, Matcher}; use jj_lib::merged_tree::MergedTreeBuilder; use jj_lib::repo::Repo; -use jj_lib::repo_path::RepoPathBuf; +use jj_lib::repo_path::{RepoPathBuf, RepoPathUiConverter}; use jj_lib::revset::{RevsetExpression, RevsetIteratorExt}; use jj_lib::store::Store; use pollster::FutureExt; @@ -30,9 +33,9 @@ use rayon::iter::IntoParallelIterator; use rayon::prelude::ParallelIterator; use tracing::instrument; -use crate::cli_util::{CommandHelper, RevisionArg}; -use crate::command_error::{config_error_with_message, CommandError}; -use crate::config::CommandNameAndArgs; +use crate::cli_util::{CommandHelper, RevisionArg, WorkspaceCommandHelper}; +use crate::command_error::CommandError; +use crate::config::{CommandNameAndArgs, NonEmptyCommandArgsVec}; use crate::ui::Ui; /// Update files with formatting fixes or other changes @@ -42,26 +45,64 @@ use crate::ui::Ui; /// 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. +/// determined by passing their file content through any external tools the user +/// has configured for those files. Descendants will also be updated by passing +/// their versions of the same files through external tools, which will ensure +/// that the fixes are not lost. This 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 external tools must accept the current file content on standard input, +/// and return the updated file content on standard output. A tool's output will +/// not be used unless it 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: +/// Tools are defined in a table, and are applied in the same order as they are +/// defined. If two tools affect the same file, the second tool will receive its +/// input from the output of the first tool. The attributes of each tool are: +/// - `name`: An arbitrary unique identifier for the tool, which will be used +/// to identify it in error messages. If two or more tools have the same name +/// configured, `jj fix` will fail without making any changes. +/// - `command`: The arguments used to run the tool. The first argument is the +/// path to an executable file. Arguments can contain the substring `$path`, +/// which will be replaced with the repo-relative path of the file being +/// fixed. It is useful to provide the path to tools that include the path in +/// error messages, or behave differently based on the directory or file +/// name. +/// - `patterns`: Determines which files the tool will affect. If this list is +/// empty, no files will be affected by the tool. If there are multiple +/// patterns, the tool is applied only once to each file in the union of the +/// patterns. +/// +/// For example, the following configuration defines how two code formatters +/// (`clang-format` and `black`) will apply to three different file extensions +/// (.cc, .h, and .py): +/// +/// [[fix.tools]] +/// name = "clang-format" +/// command = ["clang-format", "--assume-filename=$path"] +/// patterns = ["glob:'**/*.cc'", +/// "glob:'**/*.h'"] +/// +/// [[fix.tools]] +/// name = "black" +/// command = ["black", "-", "--stdin-filename=$path"] +/// patterns = ["glob:'**/*.py'"] +/// +/// Execution order of tools that affect the same file is deterministic, but +/// currently unspecified, and may change between releases. +/// +/// There is also a deprecated configuration schema that defines a single +/// command that will affect all changed files in the specified revisions. For +/// example, the following configuration would apply the Rust formatter to all +/// changed files (whether they are Rust files or not): /// /// [fix] /// tool-command = ["rustfmt", "--emit", "stdout"] /// -/// And then run the command `jj fix -s @`. +/// The tool defined by `tool-command` acts as if it was the first entry in +/// `fix.tools`, and uses `pattern = "all()"``. Support for `tool-command` +/// will be removed in a future version. #[derive(clap::Args, Clone, Debug)] #[command(verbatim_doc_comment)] pub(crate) struct FixArgs { @@ -82,6 +123,7 @@ pub(crate) fn cmd_fix( args: &FixArgs, ) -> Result<(), CommandError> { let mut workspace_command = command.workspace_helper(ui)?; + let tools_config = get_tools_config(&workspace_command, command.settings().config())?; let root_commits: Vec = if args.source.is_empty() { workspace_command.parse_revset(&RevisionArg::from( command.settings().config().get_string("revsets.fix")?, @@ -166,15 +208,9 @@ pub(crate) fn cmd_fix( } // 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, + &tools_config, &unique_tool_inputs, )?; @@ -261,20 +297,38 @@ struct ToolInput { /// each failed input. fn fix_file_ids<'a>( store: &Store, - tool_command: &CommandNameAndArgs, + tools_config: &ToolsConfig, tool_inputs: &'a HashSet, -) -> BackendResult> { +) -> Result, CommandError> { 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 { + |updates_tx, tool_input| -> Result<(), CommandError> { + let mut matching_tools = tools_config + .tools + .iter() + .filter(|tool_config| tool_config.matcher.matches(&tool_input.repo_path)) + .peekable(); + if matching_tools.peek().is_some() { + // The first matching tool gets its input from the committed file, and any + // subsequent matching tool gets its input from the previous matching tool's + // output. + let mut old_content = vec![]; + let mut read = store.read_file(&tool_input.repo_path, &tool_input.file_id)?; + read.read_to_end(&mut old_content)?; + let new_content = + matching_tools.fold(old_content.clone(), |prev_content, tool_config| { + match run_tool(&tool_config.command, tool_input, &prev_content) { + Ok(next_content) => next_content, + // TODO: Because the stderr is passed through, this isn't always failing + // silently, but it should do something better will the exit code, tool + // name, etc. + Err(_) => prev_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(); @@ -328,3 +382,111 @@ fn run_tool( Err(()) } } + +/// Represents an entry in the `fix.tools` config table. +struct ToolConfig { + /// The command that will be run to fix a matching file. + command: CommandNameAndArgs, + /// The matcher that determines if this tool matches a file. + matcher: Box, + // TODO: Store the `name` field here and print it with the command's stderr, to clearly + // associate any errors/warnings with the tool and its configuration entry. +} + +/// Represents the `fix.tools` config table. +struct ToolsConfig { + /// Some tools, stored in the order they will be executed if more than one + /// of them matches the same file. + tools: Vec, +} + +/// Simplifies deserialization of the config values while building a ToolConfig. +#[derive(Clone, Debug, Default, Eq, PartialEq, serde::Deserialize)] +#[serde(default)] +struct RawToolConfig { + command: Vec, + patterns: Vec, +} + +/// Parses the `fix.tools` config table. +/// +/// Parses the deprecated `fix.tool-command` config as if it was the first entry +/// in `fix.tools`. +/// +/// Fails if any of the commands or patterns are obviously unusable, but does +/// not check for issues that might still occur later like missing executables. +/// This is a place where we could fail earlier in some cases, though. +fn get_tools_config( + workspace_command: &WorkspaceCommandHelper, + config: &config::Config, +) -> Result { + let mut tools_config = ToolsConfig { tools: Vec::new() }; + if let Ok(tool_command) = config.get("fix.tool-command") { + // This doesn't change the displayed indices of the `fix.tools` definitions, and + // doesn't have a `name` that could conflict with them. That would matter more + // if we already had better error handling that made use of the `name`. + tools_config.tools.push(ToolConfig { + command: tool_command, + matcher: Box::new(EverythingMatcher), + }); + } + if let Ok(tools_table) = config.get_table("fix.tools") { + // Convert the map into a sorted vector early so errors are deterministic. + let mut tools: Vec = tools_table + .into_iter() + .sorted_by(|a, b| a.0.cmp(&b.0)) + .map(|(name, value)| -> Result { + let tool: RawToolConfig = value.try_deserialize()?; + if tool.patterns.is_empty() { + Err(config::ConfigError::Message(format!( + "`fix.tools.{}.patterns` must not be empty. Use patterns=['all()'] if you \ + want to run this tool on every file.", + name + )) + .into()) + } else { + Ok(ToolConfig { + command: CommandNameAndArgs::Vec( + NonEmptyCommandArgsVec::try_from(tool.command).map_err(|e| { + config::ConfigError::Message(format!( + "`fix.tools.{}.command`: {}", + name, e + )) + })?, + ), + matcher: FilesetExpression::union_all( + tool.patterns + .iter() + .map(|arg| { + fileset::parse( + arg, + // TODO: This is probably a misuse of Fs, but there is + // currently no other option for non-cwd-relative fileset + // parsing. + &RepoPathUiConverter::Fs { + cwd: workspace_command.workspace_root().clone(), + base: workspace_command.workspace_root().clone(), + }, + ) + }) + .try_collect()?, + ) + .to_matcher(), + }) + } + }) + .try_collect()?; + tools_config.tools.append(&mut tools); + } + if tools_config.tools.is_empty() { + // TODO: This is not a useful message when one or both fields are present but + // have the wrong type. After removing `fix.tool-command`, it will be simpler to + // propagate any errors from `config.get_array("fix.tools")`. + Err(config::ConfigError::Message( + "At least one entry of `fix.tools` or `fix.tool-command` is required.".to_string(), + ) + .into()) + } else { + Ok(tools_config) + } +} diff --git a/cli/src/config-schema.json b/cli/src/config-schema.json index 79271de1e6..8fb2fd6684 100644 --- a/cli/src/config-schema.json +++ b/cli/src/config-schema.json @@ -506,7 +506,7 @@ } } }, - "fix": { + "fix": { "type": "object", "description": "Settings for jj fix", "properties": { @@ -515,7 +515,35 @@ "items": { "type": "string" }, - "description": "Shell command that takes file content on stdin and returns fixed file content on stdout" + "description": "Shell command that takes file content on stdin and returns fixed file content on stdout (deprecated)" + }, + "tools": { + "type": "array", + "items": { + "type": "object", + "description": "Settings for how specific filesets are affected by a tool", + "properties": { + "name": { + "type": "string", + "description": "A unique identifier for this tool" + }, + "command": { + "type": "array", + "items": { + "type": "string" + }, + "description": "Arguments used to execute this tool" + }, + "patterns": { + "type": "array", + "items": { + "type": "string" + }, + "description": "Filesets that will be affected by this tool" + } + } + }, + "description": "Settings for tools run by jj fix" } } } diff --git a/cli/tests/cli-reference@.md.snap b/cli/tests/cli-reference@.md.snap index 66ab5779ae..9c5b69770b 100644 --- a/cli/tests/cli-reference@.md.snap +++ b/cli/tests/cli-reference@.md.snap @@ -776,26 +776,64 @@ 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: +determined by passing their file content through any external tools the user +has configured for those files. Descendants will also be updated by passing +their versions of the same files through external tools, which will ensure +that the fixes are not lost. This 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 tools must accept the current file content on standard input, +and return the updated file content on standard output. A tool's output will +not be used unless it exits with a successful exit code. Output on standard +error will be passed through to the terminal. + +Tools are defined in a table, and are applied in the same order as they are +defined. If two tools affect the same file, the second tool will receive its +input from the output of the first tool. The attributes of each tool are: + - `name`: An arbitrary unique identifier for the tool, which will be used + to identify it in error messages. If two or more tools have the same name + configured, `jj fix` will fail without making any changes. + - `command`: The arguments used to run the tool. The first argument is the + path to an executable file. Arguments can contain the substring `$path`, + which will be replaced with the repo-relative path of the file being + fixed. It is useful to provide the path to tools that include the path in + error messages, or behave differently based on the directory or file + name. + - `patterns`: Determines which files the tool will affect. If this list is + empty, no files will be affected by the tool. If there are multiple + patterns, the tool is applied only once to each file in the union of the + patterns. + +For example, the following configuration defines how two code formatters +(`clang-format` and `black`) will apply to three different file extensions +(.cc, .h, and .py): + +[[fix.tools]] +name = "clang-format" +command = ["clang-format", "--assume-filename=$path"] +patterns = ["glob:'**/*.cc'", + "glob:'**/*.h'"] + +[[fix.tools]] +name = "black" +command = ["black", "-", "--stdin-filename=$path"] +patterns = ["glob:'**/*.py'"] + +Execution order of tools that affect the same file is deterministic, but +currently unspecified, and may change between releases. + +There is also a deprecated configuration schema that defines a single +command that will affect all changed files in the specified revisions. For +example, the following configuration would apply the Rust formatter to all +changed files (whether they are Rust files or not): [fix] tool-command = ["rustfmt", "--emit", "stdout"] -And then run the command `jj fix -s @`. +The tool defined by `tool-command` acts as if it was the first entry in +`fix.tools`, and uses `pattern = "all()"``. Support for `tool-command` +will be removed in a future version. **Usage:** `jj fix [OPTIONS] [PATHS]...` diff --git a/cli/tests/test_fix_command.rs b/cli/tests/test_fix_command.rs index b3153df3b3..0ed1b43930 100644 --- a/cli/tests/test_fix_command.rs +++ b/cli/tests/test_fix_command.rs @@ -16,6 +16,7 @@ use std::os::unix::fs::PermissionsExt; use std::path::PathBuf; +use gix::bstr::ByteVec; use itertools::Itertools; use jj_lib::file_util::try_symlink; @@ -30,6 +31,9 @@ fn init_with_fake_formatter(args: &[&str]) -> (TestEnvironment, PathBuf) { 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"\\"); + // The deprecated configuration syntax is still used by tests where it doesn't + // make a meaningful difference in coverage. Otherwise, we would have to add + // dedicated test coverage for the deprecated syntax until it is removed. test_env.add_config(&format!( r#"fix.tool-command = ["{}"]"#, [escaped_formatter_path.as_str()] @@ -41,16 +45,365 @@ fn init_with_fake_formatter(args: &[&str]) -> (TestEnvironment, PathBuf) { } #[test] -fn test_fix_no_config() { +fn test_config_no_tools() { let test_env = TestEnvironment::default(); - test_env.jj_cmd_ok(test_env.env_root(), &["init", "repo", "--git"]); + test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]); + let repo_path = test_env.env_root().join("repo"); + + std::fs::write(repo_path.join("file"), "content\n").unwrap(); + let stderr = test_env.jj_cmd_failure(&repo_path, &["fix"]); + insta::assert_snapshot!(stderr, @r###" + Config error: At least one entry of `fix.tools` or `fix.tool-command` is required. + For help, see https://github.com/martinvonz/jj/blob/main/docs/config.md. + "###); + + let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "file", "-r", "@"]); + insta::assert_snapshot!(content, @"content\n"); +} + +#[test] +fn test_config_both_legacy_and_table_tools() { + 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 = ["{formatter}", "--append", "legacy change"] + + [fix.tools.tool-1] + command = ["{formatter}", "--append", "tables change"] + patterns = ["tables-file"] + "###, + formatter = escaped_formatter_path.as_str() + )); + + std::fs::write(repo_path.join("legacy-file"), "legacy content\n").unwrap(); + std::fs::write(repo_path.join("tables-file"), "tables content\n").unwrap(); + + let (_stdout, _stderr) = test_env.jj_cmd_ok(&repo_path, &["fix"]); + + let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "legacy-file", "-r", "@"]); + insta::assert_snapshot!(content, @r###" + legacy content + legacy change + "###); + let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "tables-file", "-r", "@"]); + insta::assert_snapshot!(content, @r###" + tables content + legacy change + tables change + "###); +} + +#[test] +fn test_config_multiple_tools() { + 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.tools.tool-1] + command = ["{formatter}", "--uppercase"] + patterns = ["foo"] + + [fix.tools.tool-2] + command = ["{formatter}", "--lowercase"] + patterns = ["bar"] + "###, + formatter = escaped_formatter_path.as_str() + )); + + std::fs::write(repo_path.join("foo"), "Foo\n").unwrap(); + std::fs::write(repo_path.join("bar"), "Bar\n").unwrap(); + std::fs::write(repo_path.join("baz"), "Baz\n").unwrap(); + + let (_stdout, _stderr) = test_env.jj_cmd_ok(&repo_path, &["fix"]); + + let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "foo", "-r", "@"]); + insta::assert_snapshot!(content, @"FOO\n"); + let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "bar", "-r", "@"]); + insta::assert_snapshot!(content, @"bar\n"); + let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "baz", "-r", "@"]); + insta::assert_snapshot!(content, @"Baz\n"); +} + +#[test] +fn test_config_multiple_tools_with_same_name() { + 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"\\"); + + // Multiple definitions with the same `name` are not allowed, because it is + // likely to be a mistake, and mistakes are risky when they rewrite files. + test_env.add_config(&format!( + r###" + [fix.tools.my-tool] + command = ["{formatter}", "--uppercase"] + patterns = ["foo"] + + [fix.tools.my-tool] + command = ["{formatter}", "--lowercase"] + patterns = ["bar"] + "###, + formatter = escaped_formatter_path.as_str() + )); + + std::fs::write(repo_path.join("foo"), "Foo\n").unwrap(); + std::fs::write(repo_path.join("bar"), "Bar\n").unwrap(); + + let stderr = test_env.jj_cmd_failure(&repo_path, &["fix"]); + insta::assert_snapshot!(stderr, @r###" + Config error: redefinition of table `fix.tools.my-tool` for key `fix.tools.my-tool` at line 6 column 9 in ../config/config0002.toml + For help, see https://github.com/martinvonz/jj/blob/main/docs/config.md. + "###); + + let content = std::fs::read(repo_path.join("foo")) + .unwrap() + .into_string() + .unwrap(); + insta::assert_snapshot!(content, @"Foo\n"); + let content = std::fs::read(repo_path.join("bar")) + .unwrap() + .into_string() + .unwrap(); + insta::assert_snapshot!(content, @"Bar\n"); +} + +#[test] +fn test_config_tables_overlapping_patterns() { + 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.tools.tool-1] + command = ["{formatter}", "--append", "tool-1"] + patterns = ["foo", "bar"] + + [fix.tools.tool-2] + command = ["{formatter}", "--append", "tool-2"] + patterns = ["bar", "baz"] + "###, + formatter = escaped_formatter_path.as_str() + )); + + std::fs::write(repo_path.join("foo"), "foo\n").unwrap(); + std::fs::write(repo_path.join("bar"), "bar\n").unwrap(); + std::fs::write(repo_path.join("baz"), "baz\n").unwrap(); + + let (_stdout, _stderr) = test_env.jj_cmd_ok(&repo_path, &["fix"]); + + let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "foo", "-r", "@"]); + insta::assert_snapshot!(content, @r###" + foo + tool-1 + "###); + let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "bar", "-r", "@"]); + insta::assert_snapshot!(content, @r###" + bar + tool-1 + tool-2 + "###); + let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "baz", "-r", "@"]); + insta::assert_snapshot!(content, @r###" + baz + tool-2 + "###); +} + +#[test] +fn test_config_tables_all_commands_missing() { + 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"); + test_env.add_config( + r###" + [fix.tools.my-tool-missing-command-1] + patterns = ["foo"] + + [fix.tools.my-tool-missing-command-2] + patterns = ['glob:"ba*"'] + "###, + ); + + std::fs::write(repo_path.join("foo"), "foo\n").unwrap(); + + let stderr = test_env.jj_cmd_failure(&repo_path, &["fix"]); + insta::assert_snapshot!(stderr, @r###" + Config error: `fix.tools.my-tool-missing-command-1.command`: command arguments should not be empty + For help, see https://github.com/martinvonz/jj/blob/main/docs/config.md. + "###); + + let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "foo", "-r", "@"]); + insta::assert_snapshot!(content, @"foo\n"); +} + +#[test] +fn test_config_tables_some_commands_missing() { + 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.tools.tool-1] + command = ["{formatter}", "--uppercase"] + patterns = ["foo"] + + [fix.tools.my-tool-missing-command] + patterns = ['bar'] + "###, + formatter = escaped_formatter_path.as_str() + )); + + std::fs::write(repo_path.join("foo"), "foo\n").unwrap(); + + let stderr = test_env.jj_cmd_failure(&repo_path, &["fix"]); + insta::assert_snapshot!(stderr, @r###" + Config error: `fix.tools.my-tool-missing-command.command`: command arguments should not be empty + For help, see https://github.com/martinvonz/jj/blob/main/docs/config.md. + "###); + + let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "foo", "-r", "@"]); + insta::assert_snapshot!(content, @"foo\n"); +} + +#[test] +fn test_config_tables_empty_patterns_list() { + 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 stderr = test_env.jj_cmd_failure(&repo_path, &["fix", "-s", "@"]); + 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.tools.my-tool-empty-patterns] + command = ["{formatter}", "--uppercase"] + patterns = [] + "###, + formatter = escaped_formatter_path.as_str() + )); + + std::fs::write(repo_path.join("foo"), "foo\n").unwrap(); + + let stderr = test_env.jj_cmd_failure(&repo_path, &["fix"]); insta::assert_snapshot!(stderr, @r###" - Config error: Invalid `fix.tool-command` - Caused by: configuration property "fix.tool-command" not found + Config error: `fix.tools.my-tool-empty-patterns.patterns` must not be empty. Use patterns=['all()'] if you want to run this tool on every file. For help, see https://github.com/martinvonz/jj/blob/main/docs/config.md. "###); + + let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "foo", "-r", "@"]); + insta::assert_snapshot!(content, @"foo\n"); +} + +#[test] +fn test_config_filesets() { + 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.tools.my-tool-match-one] + command = ["{formatter}", "--uppercase"] + patterns = ['glob:"a*"'] + + [fix.tools.my-tool-match-two] + command = ["{formatter}", "--reverse"] + patterns = ['glob:"b*"'] + + [fix.tools.my-tool-match-none] + command = ["{formatter}", "--append", "SHOULD NOT APPEAR"] + patterns = ['glob:"this-doesnt-match-anything-*"'] + "###, + formatter = escaped_formatter_path.as_str() + )); + + std::fs::write(repo_path.join("a1"), "a1\n").unwrap(); + std::fs::write(repo_path.join("b1"), "b1\n").unwrap(); + std::fs::write(repo_path.join("b2"), "b2\n").unwrap(); + + let (_stdout, _stderr) = test_env.jj_cmd_ok(&repo_path, &["fix"]); + + let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "a1", "-r", "@"]); + insta::assert_snapshot!(content, @"A1\n"); + let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "b1", "-r", "@"]); + insta::assert_snapshot!(content, @"1b\n"); + let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "b2", "-r", "@"]); + insta::assert_snapshot!(content, @"2b\n"); +} + +#[test] +fn test_relative_paths() { + 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.tools.tool] + command = ["{formatter}", "--stdout", "Fixed!"] + patterns = ['glob:"foo*"'] + "###, + formatter = escaped_formatter_path.as_str() + )); + + std::fs::create_dir(repo_path.join("dir")).unwrap(); + std::fs::write(repo_path.join("foo1"), "unfixed\n").unwrap(); + std::fs::write(repo_path.join("foo2"), "unfixed\n").unwrap(); + std::fs::write(repo_path.join("dir/foo3"), "unfixed\n").unwrap(); + + // Positional arguments are cwd-relative, but the configured patterns are + // repo-relative, so this command fixes the empty intersection of those + // filesets. + let (_stdout, _stderr) = test_env.jj_cmd_ok(&repo_path.join("dir"), &["fix", "foo3"]); + let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "foo1", "-r", "@"]); + insta::assert_snapshot!(content, @"unfixed\n"); + let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "foo2", "-r", "@"]); + insta::assert_snapshot!(content, @"unfixed\n"); + let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "dir/foo3", "-r", "@"]); + insta::assert_snapshot!(content, @"unfixed\n"); + + // Positional arguments can specify a subset of the configured fileset. + let (_stdout, _stderr) = test_env.jj_cmd_ok(&repo_path.join("dir"), &["fix", "../foo1"]); + let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "foo1", "-r", "@"]); + insta::assert_snapshot!(content, @"Fixed!\n"); + let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "foo2", "-r", "@"]); + insta::assert_snapshot!(content, @"unfixed\n"); + let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "dir/foo3", "-r", "@"]); + insta::assert_snapshot!(content, @"unfixed\n"); + + // The current directory does not change the interpretation of the config, so + // foo2 is fixed but not dir/foo3. + let (_stdout, _stderr) = test_env.jj_cmd_ok(&repo_path.join("dir"), &["fix"]); + let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "foo1", "-r", "@"]); + insta::assert_snapshot!(content, @"Fixed!\n"); + let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "foo2", "-r", "@"]); + insta::assert_snapshot!(content, @"Fixed!\n"); + let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "dir/foo3", "-r", "@"]); + insta::assert_snapshot!(content, @"unfixed\n"); } #[test] diff --git a/cli/tests/test_util_command.rs b/cli/tests/test_util_command.rs index ffd2128af5..03bf68a9d9 100644 --- a/cli/tests/test_util_command.rs +++ b/cli/tests/test_util_command.rs @@ -30,7 +30,7 @@ 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": { + "fix": { [...] } }