Skip to content

Commit a7e0e44

Browse files
authored
Revert "fix(package): detect dirtiness for symlinks to submodule" (#15419)
### What does this PR try to resolve? This reverts commit 71ea2e5. `Repository::discover` and `Repository::status_file` are too expenstive to run inside a loop. And `cargo package` are doing a lot of duplicate works for checking submodule VCS status. Alternative fixes might look like * Let `status_submodules` function returns a path entry set, so Cargo can check whether a source file is dirty based on that. * When listing files in `PathSource`, attach the VCS status of a path entry assoicated with. Then subsequent operations can skip status check entirely. However, the above solutions are not trivial, and the dirtiness check is informational only based on T-cargo conclusion, so we should be good just reverting the change now. Again, the caveat of this is that we can't really detect dirty symlinks that link into a Git submodule. ### How should we test and review this PR? Should be good to merge. We still got #15384 fixed via d760263 ### Additional information See <#15384 (comment)>.
2 parents 3cc0b1a + 314df11 commit a7e0e44

File tree

2 files changed

+5
-27
lines changed

2 files changed

+5
-27
lines changed

src/cargo/ops/cargo_package/vcs.rs

+2-21
Original file line numberDiff line numberDiff line change
@@ -274,28 +274,9 @@ fn dirty_files_outside_pkg_root(
274274
dirty_files.insert(workdir.join(rel_path));
275275
}
276276
Err(e) => {
277-
if e.code() == git2::ErrorCode::NotFound {
278-
// Object not found means this file might be inside a subrepo/submodule.
279-
// Let's check its status from that repo.
280-
let abs_path = workdir.join(&rel_path);
281-
if let Ok(repo) = git2::Repository::discover(&abs_path) {
282-
let is_dirty = if repo.workdir() == Some(workdir) {
283-
false
284-
} else if let Ok(path) =
285-
paths::strip_prefix_canonical(&abs_path, repo.workdir().unwrap())
286-
{
287-
repo.status_file(&path) != Ok(git2::Status::CURRENT)
288-
} else {
289-
false
290-
};
291-
if is_dirty {
292-
dirty_files.insert(abs_path);
293-
}
294-
}
295-
}
296-
297277
// Dirtiness check for symlinks is mostly informational.
298-
// To avoid adding more complicated logic,
278+
// And changes in submodule would fail git-status as well (see #15384).
279+
// To avoid adding complicated logic to handle that,
299280
// for now we ignore the status check failure.
300281
debug!(
301282
"failed to get status from file `{}` in git repo at `{}`: {e}",

tests/testsuite/package.rs

+3-6
Original file line numberDiff line numberDiff line change
@@ -1463,16 +1463,13 @@ fn dirty_file_outside_pkg_root_inside_submodule() {
14631463
p.symlink("submodule/file.txt", "isengard/src/file.txt");
14641464
git::add(&repo);
14651465
git::commit(&repo);
1466+
// This dirtyness should be detected in the future.
14661467
p.change_file("submodule/file.txt", "changed");
14671468

14681469
p.cargo("package --workspace --no-verify")
1469-
.with_status(101)
14701470
.with_stderr_data(str![[r#"
1471-
[ERROR] 1 files in the working directory contain changes that were not yet committed into git:
1472-
1473-
submodule/file.txt
1474-
1475-
to proceed despite this and include the uncommitted changes, pass the `--allow-dirty` flag
1471+
[PACKAGING] isengard v0.0.0 ([ROOT]/foo/isengard)
1472+
[PACKAGED] 6 files, [FILE_SIZE]B ([FILE_SIZE]B compressed)
14761473
14771474
"#]])
14781475
.run();

0 commit comments

Comments
 (0)