internal: Improve VCS implementation and handling. (#2569)

* Polish.

* Fixes.

* Fix diff.

* Add warnings.

* Polish.

* Update hooks.

* Fix tests.

* Fix tests.

Author: milesj

CHANGELOG.md

@@ -1,5 +1,44 @@
 # Changelog
 
+## Unreleased
+
+#### 🚀 Updates
+
+- **Processes**
+  - Improved our "stream and capture output" child process handling to operate on bytes instead of
+    lines, which should resolve some edge cases with output not being written to the console, or
+    being written out of order.
+- **VCS**
+  - Hardened all executed Git commands: revisions are validated against argument injection,
+    credential prompts now fail immediately instead of hanging, and the fsmonitor daemon is now
+    disabled to avoid it blocking process pipes.
+  - Reworked merge base resolution to be more accurate and performant. The most recent divergence
+    point is now preferred when local and remote branches are out of sync, and a warning is now
+    logged when a merge base could not be resolved, as diffs may be inaccurate without one.
+  - Reworked how renames are handled. When diffing between revisions, the old path is now reported
+    as "deleted" and the new path as "added", instead of both being "modified". For statuses, the
+    old path is now reported as "deleted", instead of being omitted entirely. Additionally, type
+    changes and unmerged files are now reported, instead of being omitted.
+
+#### 🐞 Fixes
+
+- Fixed an issue where an explicit head revision was ignored when diffing between revisions, and the
+  current working tree was compared against instead.
+- Fixed an issue where diffing against the previous revision would fail in repositories with a
+  single commit.
+- Fixed an issue where file names with spaces or special characters were excluded from file tree
+  results.
+- Fixed an issue where Git submodules added between 2 revisions were not included when diffing.
+- Fixed an issue where Git hooks could not be set up from the primary working tree when other
+  worktrees exist.
+- Fixed an issue where moon would take over a hooks directory managed by another tool (husky,
+  lefthook, etc) when `core.hooksPath` was already configured, overwriting its hook files, and
+  deleting the entire directory when hooks were disabled.
+- Fixed an issue where Windows hook wrappers would not forward arguments containing spaces
+  correctly, and would arbitrarily cap forwarding at 5 arguments.
+- Fixed an issue where PowerShell hooks would mangle user variables that start with `$ARG`, like
+  `$ARGS`.
+
 ## 2.3.3
 
 #### 🛡️ Security

Cargo.lock

@@ -99,10 +99,15 @@ async fn query_changed_files_without_stdin(
         && vcs.is_default_branch(&current_branch)
         && options.default_branch;
 
-    // Don't check for shallow if base is set,
-    // since we can assume the user knows what they're doing
+    // Don't bail on shallow if base is set, since we can assume the
+    // user knows what they're doing, but still warn them, as the diff
+    // may be inaccurate without a merge base
     if base_value.is_none() {
         check_shallow!(vcs);
+    } else if vcs.is_shallow_checkout().await? {
+        warn!(
+            "Detected a shallow checkout while comparing against an explicit base, changed files may be inaccurate. A full Git history is recommended, e.g. a fetch depth of 0."
+        );
     }
 
     let only_local = options.local && base_value.is_none();

crates/app/src/queries/changed_files.rs

@@ -442,13 +442,17 @@ mod exec {
                 cmd.arg("exec").arg(target("syntaxVar"));
             });
 
-            assert
-                .success()
-                .stdout(predicate::str::contains(if cfg!(windows) {
-                    "substituted-value\nin substituted-value quotes\nprefixed-substituted-value\nsubstituted-value-suffixed"
-                } else {
-                    "substituted-value in substituted-value quotes prefixed-substituted-value substituted-value-suffixed"
-                }));
+            // Windows tasks emit CRLF line endings, so normalize before asserting
+            let stdout = assert.stdout().replace("\r\n", "\n");
+            assert.success();
+
+            let expected = if cfg!(windows) {
+                "substituted-value\nin substituted-value quotes\nprefixed-substituted-value\nsubstituted-value-suffixed"
+            } else {
+                "substituted-value in substituted-value quotes prefixed-substituted-value substituted-value-suffixed"
+            };
+
+            assert!(stdout.contains(expected), "stdout: {stdout}");
         }
 
         #[test]
@@ -954,7 +958,7 @@ mod exec {
                             .join(".moon/cache/states/outputs/noOutput/stdout.log")
                     )
                     .unwrap(),
-                    "No outputs!"
+                    "No outputs!\n"
                 );
             }
 
@@ -1564,6 +1568,10 @@ mod exec {
                 cmd.arg("exec").arg(target("affectedFiles"));
             });
 
+            // Windows tasks emit CRLF line endings, so normalize before asserting
+            let stdout = assert.stdout().replace("\r\n", "\n");
+            assert.success();
+
             let root = sandbox.path().join(PROJECT_DIR);
 
             let mut files = fs::read_dir(&root)
@@ -1587,9 +1595,13 @@ mod exec {
                 .join(" ");
             let envs = files.join(if cfg!(windows) { ";" } else { ":" });
 
-            assert.success().stdout(
-                predicate::str::contains(format!("Args: {args}\n"))
-                    .and(predicate::str::contains(format!("Env: {envs}\n"))),
+            assert!(
+                stdout.contains(&format!("Args: {args}\n")),
+                "stdout: {stdout}"
+            );
+            assert!(
+                stdout.contains(&format!("Env: {envs}\n")),
+                "stdout: {stdout}"
             );
         }
 
@@ -1612,9 +1624,17 @@ mod exec {
             });
             let envs = ["input1.txt", "input2.txt"].join(if cfg!(windows) { ";" } else { ":" });
 
-            assert.success().stdout(
-                predicate::str::contains("Args: ./input1.txt ./input2.txt\n")
-                    .and(predicate::str::contains(format!("Env: {envs}\n"))),
+            // Windows tasks emit CRLF line endings, so normalize before asserting
+            let stdout = assert.stdout().replace("\r\n", "\n");
+            assert.success();
+
+            assert!(
+                stdout.contains("Args: ./input1.txt ./input2.txt\n"),
+                "stdout: {stdout}"
+            );
+            assert!(
+                stdout.contains(&format!("Env: {envs}\n")),
+                "stdout: {stdout}"
             );
         }
 
@@ -1636,10 +1656,15 @@ mod exec {
                     .arg("--affected");
             });
 
-            assert.success().stdout(
-                predicate::str::contains("Args: ./input1.txt ./input2.txt\n")
-                    .and(predicate::str::contains("Env: \n")),
+            // Windows tasks emit CRLF line endings, so normalize before asserting
+            let stdout = assert.stdout().replace("\r\n", "\n");
+            assert.success();
+
+            assert!(
+                stdout.contains("Args: ./input1.txt ./input2.txt\n"),
+                "stdout: {stdout}"
             );
+            assert!(stdout.contains("Env: \n"), "stdout: {stdout}");
         }
 
         #[test]
@@ -1661,9 +1686,14 @@ mod exec {
             });
             let envs = ["input1.txt", "input2.txt"].join(if cfg!(windows) { ";" } else { ":" });
 
-            assert.success().stdout(
-                predicate::str::contains("Args: \n")
-                    .and(predicate::str::contains(format!("Env: {envs}\n"))),
+            // Windows tasks emit CRLF line endings, so normalize before asserting
+            let stdout = assert.stdout().replace("\r\n", "\n");
+            assert.success();
+
+            assert!(stdout.contains("Args: \n"), "stdout: {stdout}");
+            assert!(
+                stdout.contains(&format!("Env: {envs}\n")),
+                "stdout: {stdout}"
             );
         }
     }

crates/cli/tests/exec_test.rs

@@ -210,7 +210,10 @@ impl Command {
         Ok(output)
     }
 
+    #[allow(unused, unreachable_code)]
     pub async fn exec_stream_and_capture_output(&mut self) -> miette::Result<Output> {
+        return self.exec_stream_and_capture_output_bytes().await;
+
         let registry = ProcessRegistry::instance();
         let instant = Instant::now();
         let mut command = self.create_async_command()?;

crates/process/src/exec_command.rs

@@ -132,7 +132,7 @@ mod exec_stream_and_capture_output {
             .unwrap();
 
         assert!(output.success());
-        assert_eq!(output.stdout, b"a\nb");
+        assert_eq!(output.stdout, b"a\nb\n");
         assert_eq!(output.stderr, b"err");
     }
 }

crates/process/tests/exec_command_test.rs

@@ -16,14 +16,15 @@ moon_config = { path = "../config" }
 moon_hash = { path = "../hash" }
 moon_vcs = { path = "../vcs" }
 miette = { workspace = true }
-rustc-hash = { workspace = true }
+regex = { workspace = true }
 serde = { workspace = true }
 starbase_utils = { workspace = true }
 system_env = { workspace = true }
 tracing = { workspace = true }
 
 [dev-dependencies]
 moon_test_utils = { path = "../test-utils" }
+rustc-hash = { workspace = true }
 starbase_sandbox = { workspace = true }
 tokio = { workspace = true }
 

crates/vcs-hooks/Cargo.toml

@@ -6,10 +6,14 @@ use moon_common::{
 };
 use moon_config::{VcsConfig, VcsHookFormat};
 use moon_vcs::VcsHookEnvironment;
+use regex::Regex;
 use starbase_utils::fs;
 use std::path::Path;
+use std::sync::LazyLock;
 use tracing::{debug, instrument, warn};
 
+static ARG_VAR_PATTERN: LazyLock<Regex> = LazyLock::new(|| Regex::new(r"\$(ARG\d+)").unwrap());
+
 pub enum ShellType {
     Bash,
     Pwsh,
@@ -77,7 +81,10 @@ impl<'app> HooksGenerator<'app> {
 
             self.remove_hook_files(&hooks_dir, &state.data.hook_names)?;
 
-            if hooks_dir.exists() {
+            // Only remove the directory itself when moon owns it. State from
+            // older moon versions may reference a directory managed by another
+            // tool (husky, lefthook, etc), which we should not delete!
+            if (dir == ".moon/hooks" || dir == ".config/moon/hooks") && hooks_dir.exists() {
                 fs::remove_dir_all(hooks_dir)?;
             }
         }
@@ -203,7 +210,7 @@ impl<'app> HooksGenerator<'app> {
         hook_name: &str,
         commands: &[String],
     ) -> miette::Result<()> {
-        let hook = self.format_hook_file(commands, true)?;
+        let hook = self.format_hook_file(commands)?;
 
         // Bash only
         if self.is_bash_format() {
@@ -224,21 +231,31 @@ impl<'app> HooksGenerator<'app> {
             // Create a bash hook to call the PowerShell script
             let bash_hook_path = env.hooks_dir.join(hook_name);
 
-            self.write_file(&bash_hook_path, format!(
-                "#!/bin/sh\n{} -NoLogo -NoProfile -ExecutionPolicy Bypass -File \"{}\" $1 $2 $3 $4 $5",
-                if matches!(self.shell, ShellType::Pwsh) {
-                    "pwsh.exe"
-                } else {
-                    "powershell.exe"
-                },
-                hook_path.relative_to(&env.working_dir).unwrap(),
-            ))?;
+            // Prefer a path relative from the working directory, which is
+            // where hooks are executed from, but fall back to the absolute
+            // path when the hooks directory is outside of it
+            let script_path = match hook_path.relative_to(&env.working_dir) {
+                Ok(rel_path) => rel_path.to_string(),
+                Err(_) => hook_path.to_string_lossy().to_string(),
+            };
+
+            self.write_file(
+                &bash_hook_path,
+                format!(
+                    "#!/bin/sh\n{} -NoLogo -NoProfile -ExecutionPolicy Bypass -File \"{script_path}\" \"$@\"",
+                    if matches!(self.shell, ShellType::Pwsh) {
+                        "pwsh.exe"
+                    } else {
+                        "powershell.exe"
+                    },
+                ),
+            )?;
         }
 
         Ok(())
     }
 
-    fn format_hook_file(&self, commands: &[String], with_header: bool) -> miette::Result<String> {
+    fn format_hook_file(&self, commands: &[String]) -> miette::Result<String> {
         let mut contents = vec![];
 
         if self.is_bash_format() {
@@ -270,13 +287,11 @@ impl<'app> HooksGenerator<'app> {
             ]);
         }
 
-        if with_header {
-            contents.extend([
-                "# Automatically generated by moon. DO NOT MODIFY!",
-                "# https://moonrepo.dev/docs/guides/vcs-hooks",
-                "",
-            ]);
-        }
+        contents.extend([
+            "# Automatically generated by moon. DO NOT MODIFY!",
+            "# https://moonrepo.dev/docs/guides/vcs-hooks",
+            "",
+        ]);
 
         for command in commands {
             contents.push(command);
@@ -298,7 +313,11 @@ impl<'app> HooksGenerator<'app> {
         let mut content = contents.join("\n");
 
         if !self.is_bash_format() {
-            content = content.replace("$ARG", "$env:ARG");
+            // Only rewrite the variables that we inject (ARG0, ARG1, etc),
+            // otherwise user variables like $ARGS would also be mangled
+            content = ARG_VAR_PATTERN
+                .replace_all(&content, "$$env:$1")
+                .to_string();
         }
 
         Ok(content)