From a3f37f178b5c6c2e5f94cfb25017004ea24dc8d3 Mon Sep 17 00:00:00 2001 From: Matt Stark Date: Fri, 15 Nov 2024 14:26:59 +1100 Subject: [PATCH] cli: Make fix tools run from the repo root. Fixes #4866 --- CHANGELOG.md | 2 ++ cli/src/commands/fix.rs | 13 +++++++- cli/tests/test_fix_command.rs | 56 +++++++++++++++++++++++------------ 3 files changed, 51 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2ad6f28658..c7ae26f764 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -49,6 +49,8 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ### Fixed bugs * `jj config unset ` no longer removes a table (such as `[ui]`.) +* Formatters called by `jj fix` now always run from the repo root + ([#4616](https://github.com/martinvonz/jj/issues/4616)) ## [0.23.0] - 2024-11-06 diff --git a/cli/src/commands/fix.rs b/cli/src/commands/fix.rs index 71fabc8292..2b7a28b6b0 100644 --- a/cli/src/commands/fix.rs +++ b/cli/src/commands/fix.rs @@ -15,6 +15,7 @@ use std::collections::HashMap; use std::collections::HashSet; use std::io::Write; +use std::path::Path; use std::process::Stdio; use std::sync::mpsc::channel; @@ -158,6 +159,11 @@ pub(crate) fn cmd_fix( .parse_file_patterns(ui, &args.paths)? .to_matcher(); + // See #4866. + // Fix commands must run from the repo root, as it may read files such as + // .clang-format that depend on the working directory being correct. + let working_dir = workspace_command.repo_path().to_path_buf(); + let mut tx = workspace_command.start_transaction(); // Collect all of the unique `ToolInput`s we're going to use. Tools should be @@ -239,6 +245,7 @@ pub(crate) fn cmd_fix( tx.repo().store().as_ref(), &tools_config, &unique_tool_inputs, + &working_dir, )?; // Substitute the fixed file IDs into all of the affected commits. Currently, @@ -326,6 +333,7 @@ fn fix_file_ids<'a>( store: &Store, tools_config: &ToolsConfig, tool_inputs: &'a HashSet, + working_dir: &Path, ) -> Result, CommandError> { let (updates_tx, updates_rx) = channel(); // TODO: Switch to futures, or document the decision not to. We don't need @@ -347,7 +355,8 @@ fn fix_file_ids<'a>( 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) { + match run_tool(&tool_config.command, tool_input, &prev_content, working_dir) + { 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 @@ -386,6 +395,7 @@ fn run_tool( tool_command: &CommandNameAndArgs, tool_input: &ToolInput, old_content: &[u8], + working_dir: &Path, ) -> Result, ()> { // TODO: Pipe stderr so we can tell the user which commit, file, and tool it is // associated with. @@ -393,6 +403,7 @@ fn run_tool( vars.insert("path", tool_input.repo_path.as_internal_file_string()); let mut child = tool_command .to_command_with_variables(&vars) + .current_dir(working_dir) .stdin(Stdio::piped()) .stdout(Stdio::piped()) .spawn() diff --git a/cli/tests/test_fix_command.rs b/cli/tests/test_fix_command.rs index 8a963d4f65..dd68b7cb55 100644 --- a/cli/tests/test_fix_command.rs +++ b/cli/tests/test_fix_command.rs @@ -14,6 +14,7 @@ #[cfg(unix)] use std::os::unix::fs::PermissionsExt; +use std::path::Path; use std::path::PathBuf; use itertools::Itertools; @@ -21,6 +22,19 @@ use jj_lib::file_util::try_symlink; use crate::common::TestEnvironment; +fn redact_path(s: &str, path: &Path, label: &str) -> String { + // When the test runs on Windows, backslashes in the path complicate things by + // changing the double quotes to single quotes in the serialized TOML. + s.replace( + &if cfg!(windows) { + format!(r#"'{}'"#, path.to_str().unwrap()) + } else { + format!(r#""{}""#, path.to_str().unwrap()) + }, + &format!(""), + ) +} + /// Set up a repo where the `jj fix` command uses the fake editor with the given /// flags. Returns a function that redacts the formatter executable's path from /// a given string for test determinism. @@ -42,16 +56,7 @@ fn init_with_fake_formatter(args: &[&str]) -> (TestEnvironment, PathBuf, impl Fn .join(r#"', '"#) )); (test_env, repo_path, move |snapshot: &str| { - // When the test runs on Windows, backslashes in the path complicate things by - // changing the double quotes to single quotes in the serialized TOML. - snapshot.replace( - &if cfg!(windows) { - format!(r#"'{}'"#, formatter_path.to_str().unwrap()) - } else { - format!(r#""{}""#, formatter_path.to_str().unwrap()) - }, - "", - ) + redact_path(snapshot, &formatter_path, "formatter") }) } @@ -734,11 +739,18 @@ fn test_fix_cyclic() { #[test] fn test_deduplication() { + let logfile = tempfile::Builder::new() + .prefix("jj-fix-log") + .tempfile() + .unwrap(); // Append all fixed content to a log file. This assumes we're running the tool // in the root directory of the repo, which is worth reconsidering if we // establish a contract regarding cwd. - let (test_env, repo_path, redact) = - init_with_fake_formatter(&["--uppercase", "--tee", "$path-fixlog"]); + let (test_env, repo_path, redact) = init_with_fake_formatter(&[ + "--uppercase", + "--tee", + logfile.path().as_os_str().to_str().unwrap(), + ]); // There are at least two interesting cases: the content is repeated immediately // in the child commit, or later in another descendant. @@ -756,11 +768,11 @@ fn test_deduplication() { let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["fix", "-s", "a"]); insta::assert_snapshot!(stdout, @""); - insta::assert_snapshot!(redact(&stderr), @r###" + insta::assert_snapshot!(redact(&redact_path(&stderr, logfile.path(), "logfile")), @r###" Warning: The `fix.tool-command` config option is deprecated and will be removed in a future version. Hint: Replace it with the following: [fix.tools.legacy-tool-command] - command = [, "--uppercase", "--tee", "$path-fixlog"] + command = [, "--uppercase", "--tee", ] patterns = ["all()"] Fixed 4 commits of 4 checked. @@ -780,7 +792,7 @@ fn test_deduplication() { // Each new content string only appears once in the log, because all the other // inputs (like file name) were identical, and so the results were re-used. We // sort the log because the order of execution inside `jj fix` is undefined. - insta::assert_snapshot!(sorted_lines(repo_path.join("file-fixlog")), @"BAR\nFOO\n"); + insta::assert_snapshot!(sorted_lines(logfile.path().to_path_buf()), @"BAR\nFOO\n"); } fn sorted_lines(path: PathBuf) -> String { @@ -795,18 +807,24 @@ fn sorted_lines(path: PathBuf) -> String { #[test] fn test_executed_but_nothing_changed() { + let logfile = tempfile::Builder::new() + .prefix("jj-fix-log") + .tempfile() + .unwrap(); + // Show that the tool ran by causing a side effect with --tee, and test that we // do the right thing when the tool's output is exactly equal to its input. - let (test_env, repo_path, redact) = init_with_fake_formatter(&["--tee", "$path-copy"]); + let (test_env, repo_path, redact) = + init_with_fake_formatter(&["--tee", logfile.path().as_os_str().to_str().unwrap()]); std::fs::write(repo_path.join("file"), "content\n").unwrap(); let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["fix", "-s", "@"]); insta::assert_snapshot!(stdout, @""); - insta::assert_snapshot!(redact(&stderr), @r###" + insta::assert_snapshot!(redact(&redact_path(&stderr, logfile.path(), "logfile")), @r###" Warning: The `fix.tool-command` config option is deprecated and will be removed in a future version. Hint: Replace it with the following: [fix.tools.legacy-tool-command] - command = [, "--tee", "$path-copy"] + command = [, "--tee", ] patterns = ["all()"] Fixed 0 commits of 1 checked. @@ -814,7 +832,7 @@ fn test_executed_but_nothing_changed() { "###); let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "file", "-r", "@"]); insta::assert_snapshot!(content, @"content\n"); - let copy_content = std::fs::read_to_string(repo_path.join("file-copy").as_os_str()).unwrap(); + let copy_content = std::fs::read_to_string(logfile.path()).unwrap(); insta::assert_snapshot!(copy_content, @"content\n"); }