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.

Sign up for GitHub

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

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

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f312b39
fix(cargo-package): sort dirty file report
weihanglo Dec 19, 2024
2386104
test(cargo-package): track dirty on workspace member granularity
weihanglo Dec 19, 2024
ca1060b
fix(cargo-package): show dirty filepaths relative to git workdir
weihanglo Dec 19, 2024
2bc27e1
perf(cargo-package): match certain path prefix with pathspec
weihanglo Dec 19, 2024
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
20 changes: 16 additions & 4 deletions src/cargo/ops/cargo_package.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -805,7 +805,7 @@ fn check_repo_state(
return Ok(Some(VcsInfo { git, path_in_vcs }));

fn git(
_pkg: &Package,
pkg: &Package,
src_files: &[PathBuf],
repo: &git2::Repository,
opts: &PackageOpts<'_>,
Expand All @@ -816,7 +816,8 @@ 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

cargo/src/cargo/ops/cargo_package.rs

Line 829 in 59b2ddd

.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 •
edited
Loading

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)?;
Expand Down Expand Up @@ -859,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 @@ -903,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
Loading