Skip to content

Commit

Permalink
fix(affected): consider both source and destination as changed (#9422)
Browse files Browse the repository at this point in the history
### Description

Previously we would only consider the destination file moved as changed
and not the source. We use `diff-tree` instead of `diff` to track these
moves.

This PR also fixes an issue where `include_uncommitted: false` was not
respected if `to_commit` wasn't present i.e. we would always return
uncommitted changes. This did not affect users since this API isn't
exposed to users and there's no way to set `include_uncommitted: false`
without setting a `to_commit`.

### Testing Instructions

Added unit test where a file is moved between packages. Also added
additional checks for our `changed_files` tests
  • Loading branch information
chris-olszewski authored Nov 21, 2024
1 parent 5491695 commit 81b9260
Showing 1 changed file with 107 additions and 9 deletions.
116 changes: 107 additions & 9 deletions crates/turborepo-scm/src/git.rs
Original file line number Diff line number Diff line change
Expand Up @@ -285,11 +285,17 @@ impl Git {

let valid_from = self.resolve_base(from_commit, CIEnv::new())?;

let mut args = if let Some(to_commit) = to_commit {
vec!["diff", "--name-only", &valid_from, to_commit]
} else {
vec!["diff", "--name-only", &valid_from]
};
// If a to commit is not specified for `diff-tree` it will change the comparison
// to be between the provided commit and it's parent
let to_commit = to_commit.unwrap_or("HEAD");
let mut args = vec![
"diff-tree",
"-r",
"--name-only",
"--no-commit-id",
&valid_from,
to_commit,
];

if merge_base {
args.push("--merge-base");
Expand All @@ -301,10 +307,17 @@ impl Git {
// We only care about non-tracked files if we haven't specified both ends up the
// comparison
if include_uncommitted {
// Add untracked files, i.e. files that are not in git at all
let output = self
.execute_git_command(&["ls-files", "--others", "--exclude-standard"], pathspec)?;
self.add_files_from_stdout(&mut files, turbo_root, output);
// Add untracked files or unstaged changes, i.e. files that are not in git at
// all
let ls_files_output = self.execute_git_command(
&["ls-files", "--others", "--modified", "--exclude-standard"],
pathspec,
)?;
self.add_files_from_stdout(&mut files, turbo_root, ls_files_output);
// Include any files that have been staged, but not committed
let diff_output =
self.execute_git_command(&["diff", "--name-only", "--cached"], pathspec)?;
self.add_files_from_stdout(&mut files, turbo_root, diff_output);
}

Ok(files)
Expand Down Expand Up @@ -514,6 +527,26 @@ mod tests {
.unwrap()
}

fn commit_rename(repo: &Repository, source: &Path, dest: &Path, previous_commit: Oid) -> Oid {
let mut index = repo.index().unwrap();
index.remove_path(source).unwrap();
index.add_path(dest).unwrap();
let tree_oid = index.write_tree().unwrap();
index.write().unwrap();
let tree = repo.find_tree(tree_oid).unwrap();
let previous_commit = repo.find_commit(previous_commit).unwrap();

repo.commit(
Some("HEAD"),
&repo.signature().unwrap(),
&repo.signature().unwrap(),
"Commit",
&tree,
std::slice::from_ref(&&previous_commit),
)
.unwrap()
}

#[test]
fn test_shallow_clone() -> Result<(), Error> {
let tmp_dir = tempfile::tempdir()?;
Expand Down Expand Up @@ -579,6 +612,38 @@ mod tests {
Ok(())
}

#[test]
fn test_renamed_files() -> Result<(), Error> {
let (repo_root, repo) = setup_repository(None)?;

let file = repo_root.path().join("foo.js");
let file_path = Path::new("foo.js");
fs::write(&file, "let z = 0;")?;

let first_commit_oid = commit_file(&repo, file_path, None);

fs::rename(file, repo_root.path().join("bar.js")).unwrap();

let new_file_path = Path::new("bar.js");
let _second_commit_oid = commit_rename(&repo, file_path, new_file_path, first_commit_oid);

let first_commit_sha = first_commit_oid.to_string();
let git_root = repo_root.path().to_owned();
let turborepo_root = repo_root.path().to_owned();
let files = changed_files(
git_root,
turborepo_root,
Some(&first_commit_sha),
Some("HEAD"),
false,
)?;

assert_eq!(
files,
HashSet::from(["foo.js".to_string(), "bar.js".to_string()])
);
Ok(())
}
#[test]
fn test_merge_base() -> Result<(), Error> {
let (repo_root, repo) = setup_repository(None)?;
Expand Down Expand Up @@ -652,10 +717,43 @@ mod tests {
)?;
assert_eq!(files, HashSet::from(["bar.js".to_string()]));

// Test that uncommitted file in index is not marked as changed when not
// checking uncommitted
let files = changed_files(
repo_root.path().to_path_buf(),
turbo_root.to_path_buf(),
Some("HEAD"),
None,
false,
)?;
assert_eq!(files, HashSet::new());

// Test that uncommitted file in index is marked as changed when
// checking uncommitted
let files = changed_files(
repo_root.path().to_path_buf(),
turbo_root.to_path_buf(),
Some("HEAD"),
None,
true,
)?;
assert_eq!(files, HashSet::from(["bar.js".to_string()]));

// Add file to index
index.add_path(Path::new("bar.js")).unwrap();
index.write().unwrap();

// Test that uncommitted file in index is not marked as changed when not
// checking uncommitted
let files = changed_files(
repo_root.path().to_path_buf(),
turbo_root.to_path_buf(),
Some("HEAD"),
None,
false,
)?;
assert_eq!(files, HashSet::new());

// Test that uncommitted file in index is still marked as changed
let files = changed_files(
repo_root.path().to_path_buf(),
Expand Down

0 comments on commit 81b9260

Please sign in to comment.