Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf(cargo-package): match certain path prefix with pathspec #14962

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 19 additions & 5 deletions src/cargo/ops/cargo_package.rs
Original file line number Diff line number Diff line change
Expand Up @@ -816,19 +816,21 @@ fn check_repo_state(
// - ignored (in case the user has an `include` directive that
// conflicts with .gitignore).
let mut dirty_files = Vec::new();
collect_statuses(repo, &mut dirty_files)?;
let pathspec = relative_pathspec(repo, pkg.root());
collect_statuses(repo, &[pathspec.as_str()], &mut dirty_files)?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the readme is ../README.md?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this PR changed anything for relative paths. It is a bug itself.

They will be filtered out later in

.filter(|src_file| dirty_files.iter().any(|path| src_file.starts_with(path)))

That said, I don't think either dirty symlinks or relative paths were correctly handled on master.

Copy link
Member Author

@weihanglo weihanglo Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me create a test and verify it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is saying if a src_file should start with a dirty_file path, else filter it out. I'm not seeing how thats relevant to a ../README.md. It would if Cargo.toml is in the repo root but it is most likely to be in a sub-directory within the repo, so that README.md could be among dirty_files.

Or is src_files not including these ../README.md files?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either way, isn't this cementing the bug in further, making it harder for us to fix?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src_files includes symlink files as ordinary files. By the time we get src_files, file metadata has been lost.

I don't think this change worsen the situation, unless we consider rewriting list_files to do more as a fix to #14955.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the non-symlink case?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #14967.

Going to close this and discuss the performance issue in #14955

// Include each submodule so that the error message can provide
// specifically *which* files in a submodule are modified.
status_submodules(repo, &mut dirty_files)?;

// Find the intersection of dirty in git, and the src_files that would
// be packaged. This is a lazy n^2 check, but seems fine with
// thousands of files.
let dirty_src_files: Vec<String> = src_files
let workdir = repo.workdir().unwrap();
let mut dirty_src_files: Vec<_> = src_files
.iter()
.filter(|src_file| dirty_files.iter().any(|path| src_file.starts_with(path)))
.map(|path| {
path.strip_prefix(pkg.root())
path.strip_prefix(workdir)
.unwrap_or(path)
.display()
.to_string()
Expand All @@ -847,6 +849,7 @@ fn check_repo_state(
dirty,
}))
} else {
dirty_src_files.sort_unstable();
anyhow::bail!(
"{} files in the working directory contain changes that were \
not yet committed into git:\n\n{}\n\n\
Expand All @@ -857,16 +860,27 @@ fn check_repo_state(
}
}

/// Use pathspec so git only matches a certain path prefix
fn relative_pathspec(repo: &git2::Repository, pkg_root: &Path) -> String {
let workdir = repo.workdir().unwrap();
let relpath = pkg_root.strip_prefix(workdir).unwrap_or(Path::new(""));
// to unix separators
relpath.to_str().unwrap().replace('\\', "/")
}

// Helper to collect dirty statuses for a single repo.
fn collect_statuses(
repo: &git2::Repository,
pathspecs: &[&str],
dirty_files: &mut Vec<PathBuf>,
) -> CargoResult<()> {
let mut status_opts = git2::StatusOptions::new();
// Exclude submodules, as they are being handled manually by recursing
// into each one so that details about specific files can be
// retrieved.
status_opts
pathspecs
.iter()
.fold(&mut status_opts, git2::StatusOptions::pathspec)
.exclude_submodules(true)
.include_ignored(true)
.include_untracked(true);
Expand Down Expand Up @@ -901,7 +915,7 @@ fn check_repo_state(
// If its files are required, then the verification step should fail.
if let Ok(sub_repo) = submodule.open() {
status_submodules(&sub_repo, dirty_files)?;
collect_statuses(&sub_repo, dirty_files)?;
collect_statuses(&sub_repo, &[], dirty_files)?;
}
}
Ok(())
Expand Down
135 changes: 135 additions & 0 deletions tests/testsuite/package.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1156,6 +1156,141 @@ src/lib.rs
.run();
}

#[cargo_test]
fn dirty_workspace_package() {
// Cargo see them as clean if not in any local package's directory.
let (p, repo) = git::new_repo("foo", |p| {
p.file(
"Cargo.toml",
r#"
[workspace]
members = ["isengard", "mordor"]
"#,
)
.file("not-belong-to-any-mortal-man", "...")
.file(
"isengard/Cargo.toml",
r#"
[package]
name = "isengard"
edition = "2015"
homepage = "saruman"
description = "saruman"
license = "MIT"
"#,
)
.file("isengard/src/lib.rs", "")
.file(
"mordor/Cargo.toml",
r#"
[package]
name = "mordor"
edition = "2015"
homepage = "sauron"
description = "sauron"
license = "MIT"
"#,
)
.file("mordor/src/lib.rs", "")
});
git::commit(&repo);

p.change_file(
"Cargo.toml",
r#"
[workspace]
members = ["isengard", "mordor"]
[workspace.package]
edition = "2021"
"#,
);
// Dirty file outside won't affect packaging.
p.change_file("not-belong-to-any-mortal-man", "changed!");
p.change_file("mordor/src/lib.rs", "changed!");
p.change_file("mordor/src/main.rs", "fn main() {}");

// Ensure dirty files be reported.
p.cargo("package --workspace --no-verify")
.with_status(101)
.with_stderr_data(str![[r#"
[PACKAGING] isengard v0.0.0 ([ROOT]/foo/isengard)
[PACKAGED] 5 files, [FILE_SIZE]B ([FILE_SIZE]B compressed)
[ERROR] 2 files in the working directory contain changes that were not yet committed into git:

mordor/src/lib.rs
mordor/src/main.rs

to proceed despite this and include the uncommitted changes, pass the `--allow-dirty` flag

"#]])
.run();

// Ensure only dirty package mordor be dirty.
p.cargo("package --workspace --no-verify --allow-dirty")
.with_stderr_data(str![[r#"
[PACKAGING] isengard v0.0.0 ([ROOT]/foo/isengard)
[PACKAGED] 5 files, [FILE_SIZE]B ([FILE_SIZE]B compressed)
[PACKAGING] mordor v0.0.0 ([ROOT]/foo/mordor)
[PACKAGED] 6 files, [FILE_SIZE]B ([FILE_SIZE]B compressed)

"#]])
.run();

let f = File::open(&p.root().join("target/package/isengard-0.0.0.crate")).unwrap();
validate_crate_contents(
f,
"isengard-0.0.0.crate",
&[
".cargo_vcs_info.json",
"Cargo.toml",
"Cargo.toml.orig",
"src/lib.rs",
"Cargo.lock",
],
[(
".cargo_vcs_info.json",
// No change within `isengard/`, so not dirty at all.
str![[r#"
{
"git": {
"sha1": "[..]"
},
"path_in_vcs": "isengard"
}
"#]]
.is_json(),
)],
);

let f = File::open(&p.root().join("target/package/mordor-0.0.0.crate")).unwrap();
validate_crate_contents(
f,
"mordor-0.0.0.crate",
&[
".cargo_vcs_info.json",
"Cargo.toml",
"Cargo.toml.orig",
"src/lib.rs",
"src/main.rs",
"Cargo.lock",
],
[(
".cargo_vcs_info.json",
// Dirty bit is recorded.
str![[r#"
{
"git": {
"dirty": true,
"sha1": "[..]"
},
"path_in_vcs": "mordor"
}
"#]]
.is_json(),
)],
);
}

#[cargo_test]
fn issue_13695_allow_dirty_vcs_info() {
let p = project()
Expand Down
Loading