From bd538e3747d609d1c8d576753ec7559e57ff9812 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 | 251 ++++++++++++++++++++---- cli/src/config-schema.json | 32 ++- cli/tests/cli-reference@.md.snap | 67 +++++-- cli/tests/test_fix_command.rs | 325 ++++++++++++++++++++++++++++++- cli/tests/test_util_command.rs | 2 +- 6 files changed, 628 insertions(+), 57 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ace2489aa8..ba6f4178cf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,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 @@ -46,6 +50,10 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). address unconditionally. Only ASCII case folding is currently implemented, but this will likely change in the future. +* `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 ## [0.19.0] - 2024-07-03 diff --git a/cli/src/commands/fix.rs b/cli/src/commands/fix.rs index 590dfb9948..b9019fc10b 100644 --- a/cli/src/commands/fix.rs +++ b/cli/src/commands/fix.rs @@ -17,9 +17,11 @@ use std::io::Write; use std::process::Stdio; use std::sync::mpsc::channel; +use config::ConfigError; use futures::StreamExt; use itertools::Itertools; use jj_lib::backend::{BackendError, BackendResult, CommitId, FileId, TreeValue}; +use jj_lib::matchers::{EverythingMatcher, Matcher}; use jj_lib::merged_tree::MergedTreeBuilder; use jj_lib::repo::Repo; use jj_lib::repo_path::RepoPathBuf; @@ -30,9 +32,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 +44,61 @@ 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"] +/// +/// 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 +119,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 +204,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,7 +293,7 @@ struct ToolInput { /// each failed input. fn fix_file_ids<'a>( store: &Store, - tool_command: &CommandNameAndArgs, + tools_config: &ToolsConfig, tool_inputs: &'a HashSet, ) -> BackendResult> { let (updates_tx, updates_rx) = channel(); @@ -270,16 +302,46 @@ fn fix_file_ids<'a>( tool_inputs.into_par_iter().try_for_each_init( || updates_tx.clone(), |updates_tx, tool_input| -> Result<(), BackendError> { - let mut read = store.read_file(&tool_input.repo_path, &tool_input.file_id)?; - let mut old_content = vec![]; - read.read_to_end(&mut old_content).unwrap(); - if let Ok(new_content) = run_tool(tool_command, tool_input, &old_content) { - if new_content != *old_content { - let new_file_id = - store.write_file(&tool_input.repo_path, &mut new_content.as_slice())?; - updates_tx.send((tool_input, new_file_id)).unwrap(); + // 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. In either case, we store the input in `prev_content`, but we avoid + // reading anything until we know at least one tool matches. + let mut prev_content: Option> = None; + let mut any_changes: bool = false; + for tool_config in tools_config.tools.iter() { + if tool_config.matcher.matches(&tool_input.repo_path) { + if prev_content.is_none() { + let mut content = vec![]; + let mut read = + store.read_file(&tool_input.repo_path, &tool_input.file_id)?; + read.read_to_end(&mut content).unwrap(); + prev_content = Some(content); + } + if let Ok(next_content) = run_tool( + &tool_config.command, + tool_input, + prev_content.as_ref().unwrap(), + ) { + if next_content != *prev_content.as_ref().unwrap() { + prev_content = Some(next_content); + any_changes = true; + } + } } } + if any_changes { + // If there were multiple matching tools, the last one might have produced the + // same file content that we started with, which could mean we don't need to + // rewrite a commit. Rather than keep the original file content around for + // comparison, we just rely on the resulting FileID being the same so that we + // don't rewrite the commit. That is not expected to be a common scenario, so + // optimizing it is not currently important. + let new_file_id = store.write_file( + &tool_input.repo_path, + &mut prev_content.as_ref().unwrap().as_slice(), + )?; + updates_tx.send((tool_input, new_file_id)).unwrap(); + } Ok(()) }, )?; @@ -328,3 +390,126 @@ 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, +} + +/// 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(tool_config_array) = config.get_array("fix.tools") { + let mut tool_name_first_seen_index = HashMap::new(); + let mut tools: Vec = tool_config_array + .into_iter() + .enumerate() + .map(|(index, val)| -> Result { + let table = val.into_table()?; + let name = table + .get("name") + .ok_or(config::ConfigError::Message(format!( + "`fix.tools` entry at index {} is missing required field `name`.", + index + )))? + .to_string(); + if let Some(first_index) = tool_name_first_seen_index.insert(name.clone(), index) { + return Err(config::ConfigError::Message(format!( + "`fix.tools` entries at indices {} and {} have the same `name`: {}", + first_index, index, name + )) + .into()); + } + let patterns: Vec = table + .get("patterns") + .ok_or(config::ConfigError::Message(format!( + "`fix.tools` entry `{}` at index {} is missing required field `patterns`.", + name, index + )))? + .clone() + .into_array()? + .into_iter() + .map(|value| -> Result { value.into_string() }) + .try_collect()?; + if patterns.is_empty() { + Err(config::ConfigError::Message(format!( + "`fix.tools` entry `{}` at index {} must have at least one pattern.", + name, index + )) + .into()) + } else { + Ok(ToolConfig { + command: CommandNameAndArgs::Vec( + NonEmptyCommandArgsVec::try_from( + table + .get("command") + .ok_or(config::ConfigError::Message(format!( + "`fix.tools` entry `{}` at index {} is missing required \ + field `command`.", + name, index + )))? + .clone() + .into_array()? + .iter() + .map(|value| value.to_string()) + .collect_vec(), + ) + .map_err(|e| { + config::ConfigError::Message(format!( + "`fix.tools` entry `{}` at index {}: {}", + name, index, e + )) + })?, + ), + matcher: workspace_command + .parse_union_filesets(&patterns)? + .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 5fbfdba40d..972dfc0830 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..31ff6ff2d7 100644 --- a/cli/tests/cli-reference@.md.snap +++ b/cli/tests/cli-reference@.md.snap @@ -776,26 +776,61 @@ 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"] + +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..7171c7193d 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,327 @@ 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]] + name = "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]] + name = "tool-1" + command = ["{formatter}", "--uppercase"] + patterns = ["foo"] + + [[fix.tools]] + name = "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]] + name = "my-tool" + command = ["{formatter}", "--uppercase"] + patterns = ["foo"] + + [[fix.tools]] + name = "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: `fix.tools` entries at indices 0 and 1 have the same `name`: my-tool + 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]] + name = "tool-1" + command = ["{formatter}", "--append", "tool-1"] + patterns = ["foo", "bar"] + + [[fix.tools]] + name = "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]] + name = "my-tool-missing-command-1" + patterns = ["foo"] + + [[fix.tools]] + name = "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` entry `my-tool-missing-command-1` at index 0 is missing required field `command`. + 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]] + name = "tool-1" + command = ["{formatter}", "--uppercase"] + patterns = ["foo"] + + [[fix.tools]] + name = "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` entry `my-tool-missing-command` at index 1 is missing required field `command`. + 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]] + name = "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` entry `my-tool-empty-patterns` at index 0 must have at least one pattern. 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]] + name = "my-tool-match-one" + command = ["{formatter}", "--uppercase"] + patterns = ['glob:"fo*"'] + + [[fix.tools]] + name = "my-tool-match-two" + command = ["{formatter}", "--reverse"] + patterns = ['glob:"ba*"'] + + [[fix.tools]] + name = "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("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, @"rab\n"); + let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "baz", "-r", "@"]); + insta::assert_snapshot!(content, @"zab\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": { [...] } }