cli: Make invalid fix tools fail gracefully.

Fixes jj-vcs#4866

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;
 
@@ -181,6 +182,17 @@ pub(crate) fn cmd_fix(
         .try_collect()?;
     let mut unique_tool_inputs: HashSet<ToolInput> = HashSet::new();
     let mut commit_paths: HashMap<CommitId, HashSet<RepoPathBuf>> = HashMap::new();
+
+    // See https://github.com/martinvonz/jj/issues/4866
+    // TLDR is that a fix tool should not read from a file, and one which does will
+    // squash all your changes together. To prevent a tool from reading a file,
+    // we make the relative paths we provide invalid by changing the working
+    // directory.
+    let working_dir = tempfile::Builder::new()
+        .prefix("jj-fix-")
+        .tempdir()
+        .unwrap();
+
     for commit in commits.iter().rev() {
         let mut paths: HashSet<RepoPathBuf> = HashSet::new();
 
@@ -237,6 +249,7 @@ pub(crate) fn cmd_fix(
         tx.repo().store().as_ref(),
         &tools_config,
         &unique_tool_inputs,
+        working_dir.path(),
     )?;
 
     // Substitute the fixed file IDs into all of the affected commits. Currently,
@@ -324,6 +337,7 @@ fn fix_file_ids<'a>(
     store: &Store,
     tools_config: &ToolsConfig,
     tool_inputs: &'a HashSet<ToolInput>,
+    working_dir: &Path,
 ) -> Result<HashMap<&'a ToolInput, FileId>, CommandError> {
     let (updates_tx, updates_rx) = channel();
     // TODO: Switch to futures, or document the decision not to. We don't need
@@ -345,7 +359,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
@@ -384,13 +399,15 @@ fn run_tool(
     tool_command: &CommandNameAndArgs,
     tool_input: &ToolInput,
     old_content: &[u8],
+    working_dir: &Path,
 ) -> Result<Vec<u8>, ()> {
     // TODO: Pipe stderr so we can tell the user which commit, file, and tool it is
     // associated with.
     let mut vars: HashMap<&str, &str> = HashMap::new();
     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()

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 = [<redacted formatter path>, "--uppercase", "--tee", "$path-fixlog"]
+                command = [<redacted formatter path>, "--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,26 +800,32 @@ 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 = [<redacted formatter path>, "--tee", "$path-copy"]
+                command = [<redacted formatter path>, "--tee", "$logfile"]
                 patterns = ["all()"]
                 
     Fixed 0 commits of 1 checked.
     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");
 }