Fixing tests and specifying paths more carefully

cli/internal/scm/git_go.go

@@ -11,6 +11,7 @@ package scm
 
 import (
 	"fmt"
+	"github.com/vercel/turbo/cli/internal/turbopath"
 	"os/exec"
 	"path/filepath"
 	"strings"
@@ -20,13 +21,13 @@ import (
 
 // git implements operations on a git repository.
 type git struct {
-	repoRoot string
+	repoRoot turbopath.AbsoluteSystemPath
 }
 
 // ChangedFiles returns a list of modified files since the given commit, optionally including untracked files.
 func (g *git) ChangedFiles(fromCommit string, toCommit string, relativeTo string) ([]string, error) {
 	if relativeTo == "" {
-		relativeTo = g.repoRoot
+		relativeTo = g.repoRoot.ToString()
 	}
 	relSuffix := []string{"--", relativeTo}
 	command := []string{"diff", "--name-only", toCommit}

cli/internal/scm/git_rust.go

@@ -12,22 +12,23 @@ package scm
 import (
 	"fmt"
 	"github.com/vercel/turbo/cli/internal/ffi"
+	"github.com/vercel/turbo/cli/internal/turbopath"
 )
 
 // git implements operations on a git repository.
 type git struct {
-	repoRoot string
+	repoRoot turbopath.AbsoluteSystemPath
 }
 
 // ChangedFiles returns a list of modified files since the given commit, optionally including untracked files.
 func (g *git) ChangedFiles(fromCommit string, toCommit string, monorepoRoot string) ([]string, error) {
-	return ffi.ChangedFiles(g.repoRoot, monorepoRoot, fromCommit, toCommit)
+	return ffi.ChangedFiles(g.repoRoot.ToString(), monorepoRoot, fromCommit, toCommit)
 }
 
 func (g *git) PreviousContent(fromCommit string, filePath string) ([]byte, error) {
 	if fromCommit == "" {
 		return nil, fmt.Errorf("Need commit sha to inspect file contents")
 	}
 
-	return ffi.PreviousContent(g.repoRoot, fromCommit, filePath)
+	return ffi.PreviousContent(g.repoRoot.ToString(), fromCommit, filePath)
 }

cli/internal/scm/scm.go

@@ -7,11 +7,8 @@
 package scm
 
 import (
-	"path/filepath"
-
 	"github.com/pkg/errors"
 
-	"github.com/vercel/turbo/cli/internal/fs"
 	"github.com/vercel/turbo/cli/internal/turbopath"
 )
 
@@ -27,16 +24,16 @@ type SCM interface {
 
 // newGitSCM returns a new SCM instance for this repo root.
 // It returns nil if there is no known implementation there.
-func newGitSCM(repoRoot string) SCM {
-	if fs.PathExists(filepath.Join(repoRoot, ".git")) {
+func newGitSCM(repoRoot turbopath.AbsoluteSystemPath) SCM {
+	if repoRoot.UntypedJoin(".git").Exists() {
 		return &git{repoRoot: repoRoot}
 	}
 	return nil
 }
 
 // newFallback returns a new SCM instance for this repo root.
 // If there is no known implementation it returns a stub.
-func newFallback(repoRoot string) (SCM, error) {
+func newFallback(repoRoot turbopath.AbsoluteSystemPath) (SCM, error) {
 	if scm := newGitSCM(repoRoot); scm != nil {
 		return scm, nil
 	}
@@ -52,5 +49,5 @@ func FromInRepo(repoRoot turbopath.AbsoluteSystemPath) (SCM, error) {
 	if err != nil {
 		return nil, err
 	}
-	return newFallback(dotGitDir.Dir().ToStringDuringMigration())
+	return newFallback(dotGitDir.Dir())
 }

crates/turborepo-scm/Cargo.toml

@@ -9,9 +9,9 @@ license = "MPL-2.0"
 [dependencies]
 anyhow = { workspace = true }
 dunce = { workspace = true }
+git2 = { version = "0.16.1", default-features = false }
 thiserror = { workspace = true }
 turbopath = { workspace = true }
 
 [dev-dependencies]
-git2 = { version = "0.16.1", default-features = false }
 tempfile = { workspace = true }

crates/turborepo-scm/src/git.rs

@@ -34,49 +34,65 @@ pub fn changed_files(
     let git_root = AbsoluteSystemPathBuf::new(git_root)?;
     let turbo_root = AbsoluteSystemPathBuf::new(turbo_root)?;
     let turbo_root_relative_to_git_root = git_root.anchor(&turbo_root)?;
+    let pathspec = turbo_root_relative_to_git_root.to_str()?;
 
     let mut files = HashSet::new();
-    let output = Command::new("git")
-        .arg("diff")
-        .arg("--name-only")
-        .arg(to_commit)
-        .arg("--")
-        .arg(turbo_root_relative_to_git_root.to_str().unwrap())
-        .current_dir(&git_root)
-        .output()
-        .expect("failed to execute process");
-
-    add_files_from_stdout(&mut files, &git_root, &turbo_root, output.stdout);
+
+    let output = execute_git_command(&git_root, &["diff", "--name-only", to_commit], pathspec)?;
+
+    add_files_from_stdout(&mut files, &git_root, &turbo_root, output);
 
     if let Some(from_commit) = from_commit {
-        let output = Command::new("git")
-            .arg("diff")
-            .arg("--name-only")
-            .arg(format!("{}...{}", from_commit, to_commit))
-            .arg("--")
-            .arg(turbo_root_relative_to_git_root.to_str().unwrap())
-            .current_dir(&git_root)
-            .output()
-            .expect("failed to execute process");
-
-        add_files_from_stdout(&mut files, &git_root, &turbo_root, output.stdout);
+        let output = execute_git_command(
+            &git_root,
+            &[
+                "diff",
+                "--name-only",
+                &format!("{}...{}", from_commit, to_commit),
+            ],
+            pathspec,
+        )?;
+
+        add_files_from_stdout(&mut files, &git_root, &turbo_root, output);
     }
 
-    let output = Command::new("git")
-        .arg("ls-files")
-        .arg("--other")
-        .arg("--exclude-standard")
-        .arg("--")
-        .arg(turbo_root_relative_to_git_root.to_str().unwrap())
-        .current_dir(&git_root)
-        .output()
-        .expect("failed to execute process");
+    let output = execute_git_command(
+        &git_root,
+        &["ls-files", "--others", "--exclude-standard"],
+        pathspec,
+    )?;
 
-    add_files_from_stdout(&mut files, &git_root, &turbo_root, output.stdout);
+    add_files_from_stdout(&mut files, &git_root, &turbo_root, output);
 
     Ok(files)
 }
 
+fn execute_git_command(
+    git_root: &AbsoluteSystemPathBuf,
+    args: &[&str],
+    pathspec: &str,
+) -> Result<Vec<u8>, Error> {
+    let mut command = Command::new("git");
+    command.args(args).current_dir(&git_root);
+
+    add_pathspec(&mut command, pathspec);
+
+    let output = command.output()?;
+
+    if !output.status.success() {
+        let stderr = String::from_utf8(output.stderr).unwrap();
+        Err(Error::Git(stderr))
+    } else {
+        Ok(output.stdout)
+    }
+}
+
+fn add_pathspec(command: &mut Command, pathspec: &str) {
+    if pathspec != "" {
+        command.arg("--").arg(pathspec);
+    }
+}
+
 fn add_files_from_stdout(
     files: &mut HashSet<String>,
     git_root: &AbsoluteSystemPathBuf,
@@ -123,20 +139,42 @@ pub fn previous_content(
     from_commit: &str,
     file_path: PathBuf,
 ) -> Result<Vec<u8>, Error> {
-    let output = Command::new("git")
-        .arg("show")
-        .arg(format!("{}:{}", from_commit, file_path.to_str().unwrap()))
-        .current_dir(&git_root)
-        .output()?;
+    // If git root is not absolute, we error.
+    let git_root = AbsoluteSystemPathBuf::new(git_root)?;
 
-    Ok(output.stdout)
+    // However for file path we handle both absolute and relative paths
+    // Note that we assume any relative file path is relative to the git root
+    let anchored_file_path = if file_path.is_absolute() {
+        let absolute_file_path = AbsoluteSystemPathBuf::new(file_path)?;
+        git_root.anchor(&absolute_file_path)?
+    } else {
+        file_path.as_path().try_into()?
+    };
+
+    let mut command = Command::new("git");
+    let command = command
+        .arg("show")
+        .arg(format!(
+            "{}:{}",
+            from_commit,
+            anchored_file_path.to_str().unwrap()
+        ))
+        .current_dir(&git_root);
+
+    let output = command.output()?;
+    if output.status.success() {
+        Ok(output.stdout)
+    } else {
+        Err(Error::Git(
+            String::from_utf8_lossy(&output.stderr).to_string(),
+        ))
+    }
 }
 
 #[cfg(test)]
 mod tests {
     use std::{
         collections::HashSet,
-        env::set_current_dir,
         fs,
         path::{Path, PathBuf},
         process::Command,
@@ -479,16 +517,6 @@ mod tests {
         )?;
         assert_eq!(content, b"let z = 1;");
 
-        set_current_dir(repo_root.path())?;
-
-        // Check that relative paths work as well
-        let content = previous_content(
-            PathBuf::from("."),
-            second_commit_oid.to_string().as_str(),
-            PathBuf::from("./foo.js"),
-        )?;
-        assert_eq!(content, b"let z = 1;");
-
         let content = previous_content(
             repo_root.path().to_path_buf(),
             second_commit_oid.to_string().as_str(),

crates/turborepo-scm/src/lib.rs

@@ -6,7 +6,9 @@ pub mod git;
 #[derive(Debug, Error)]
 pub enum Error {
     #[error("git error: {0}")]
-    Git(#[from] git2::Error),
+    Git2(#[from] git2::Error),
+    #[error("git error: {0}")]
+    Git(String),
     #[error("repository not found")]
     RepositoryNotFound,
     #[error("io error: {0}")]