From cf9d62ece00ad1797e56117865563ac98abce7c1 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 | 27 +++++++++++++++++++-------- 3 files changed, 33 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2ad6f286581..f99735e8fca 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]`.) +* `jj fix` now runs 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 71fabc8292d..2b7a28b6b05 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 8a963d4f656..4bc5c481381 100644 --- a/cli/tests/test_fix_command.rs +++ b/cli/tests/test_fix_command.rs @@ -734,11 +734,16 @@ fn test_fix_cyclic() { #[test] fn test_deduplication() { + let logfile = tempfile::Builder::new() + .prefix("jj-fix-log") + .tempfile() + .unwrap(); + let logfile_path_str = logfile.path().as_os_str().to_str().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"]); + init_with_fake_formatter(&["--uppercase", "--tee", logfile_path_str]); // There are at least two interesting cases: the content is repeated immediately // in the child commit, or later in another descendant. @@ -756,11 +761,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(&stderr.replace(logfile_path_str, "$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", "$logfile"] patterns = ["all()"] Fixed 4 commits of 4 checked. @@ -780,7 +785,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 +800,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(); + let logfile_path_str = logfile.path().as_os_str().to_str().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_str]); 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(&stderr.replace(logfile_path_str, "$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", "$logfile"] patterns = ["all()"] Fixed 0 commits of 1 checked. @@ -814,7 +825,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"); }