From 0eb882ea79d50ef1b9be715d8f4d6e88ab811cbe Mon Sep 17 00:00:00 2001 From: Danny Hooper Date: Wed, 10 Jul 2024 18:22:08 -0500 Subject: [PATCH] cli `jj fix`: add ability to configure multiple tools for different filesets The high level changes include: - Reworking `fix_file_ids()` to loop over multiple candidate tools per file, piping file content between them. Only the final file content is written to the store, and content is no longer read for changed files that don't match any of the configured patterns. - New struct `ToolsConfig` to represent the parsed/validated configuration. - New function `get_tools_config()` to create a `ToolsConfig` from a `Config`. - New tests; the only old behavior that has changed is that we don't require `fix.tool-command` if `fix.tools` defines one or more tools. The general approach to validating the config is to fail early if anything is weird. Co-Authored-By: Josh Steadmon --- CHANGELOG.md | 8 + cli/src/commands/fix.rs | 230 +++++++++++++++++--- cli/src/config-schema.json | 32 ++- cli/tests/cli-reference@.md.snap | 70 ++++-- cli/tests/test_fix_command.rs | 363 ++++++++++++++++++++++++++++++- cli/tests/test_util_command.rs | 2 +- 6 files changed, 647 insertions(+), 58 deletions(-) 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": { [...] } }