From b04fb21197e2c10016a7abd0400d7fed4243b093 Mon Sep 17 00:00:00 2001 From: --global Date: Mon, 17 Apr 2023 15:39:31 -0400 Subject: [PATCH] Fixing tests and specifying paths more carefully --- cli/internal/scm/git_go.go | 5 +- cli/internal/scm/git_rust.go | 7 +- cli/internal/scm/scm.go | 11 +-- crates/turborepo-scm/Cargo.toml | 2 +- crates/turborepo-scm/src/git.rs | 126 +++++++++++++++++++------------- crates/turborepo-scm/src/lib.rs | 4 +- 6 files changed, 92 insertions(+), 63 deletions(-) diff --git a/cli/internal/scm/git_go.go b/cli/internal/scm/git_go.go index 1e5060895f0bc..0dac2bfdb074b 100644 --- a/cli/internal/scm/git_go.go +++ b/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} diff --git a/cli/internal/scm/git_rust.go b/cli/internal/scm/git_rust.go index bce51a87916d8..4b4cd2dedc1b0 100644 --- a/cli/internal/scm/git_rust.go +++ b/cli/internal/scm/git_rust.go @@ -12,16 +12,17 @@ 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) { @@ -29,5 +30,5 @@ func (g *git) PreviousContent(fromCommit string, filePath string) ([]byte, error 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) } diff --git a/cli/internal/scm/scm.go b/cli/internal/scm/scm.go index b491cd8d382ff..e7f17c8b7374a 100644 --- a/cli/internal/scm/scm.go +++ b/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,8 +24,8 @@ 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 @@ -36,7 +33,7 @@ func newGitSCM(repoRoot string) SCM { // 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()) } diff --git a/crates/turborepo-scm/Cargo.toml b/crates/turborepo-scm/Cargo.toml index 01e3f7df07fc7..9c40f507ce844 100644 --- a/crates/turborepo-scm/Cargo.toml +++ b/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 } diff --git a/crates/turborepo-scm/src/git.rs b/crates/turborepo-scm/src/git.rs index 9b0241fd296b0..e6676757b4043 100644 --- a/crates/turborepo-scm/src/git.rs +++ b/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, 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, git_root: &AbsoluteSystemPathBuf, @@ -123,20 +139,42 @@ pub fn previous_content( from_commit: &str, file_path: PathBuf, ) -> Result, 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(), diff --git a/crates/turborepo-scm/src/lib.rs b/crates/turborepo-scm/src/lib.rs index 60456ffe4542d..a93e84721a25f 100644 --- a/crates/turborepo-scm/src/lib.rs +++ b/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}")]