( )⚙ D12202 status: fix hg status race against file deletion

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D12202

status: fix hg status race against file deletion
ClosedPublic

Authored by aalekseyev on Feb 17 2022, 3:51 PM.

Details

Reviewers
Alphare
Group Reviewers
hg-reviewers
Commits
rHGdcec16e799dd: status: fix hg status race against file deletion

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

aalekseyev created this revision.Feb 17 2022, 3:51 PM
aalekseyev added a comment.Feb 17 2022, 3:58 PM

This is a bug that leads to hg status wrongly saying that the whole directory is deleted even though only some of the files in it was deleted (potentially one of the ignored files, in which case the status could even be clean).
I tested it using the following command:

(mkdir r; cd r; hg init; echo 'd/x*' > .hgignore; mkdir d; touch d/x{1..10000}; hg status --config 'extensions.sparse=!' & rm d/*; wait; cd ..; rm -r r)

Without this patch, this prints "d: No such file or directory". With this patch it does not.

aalekseyev added a comment.Feb 17 2022, 4:00 PM

The condition to skip just the ENOENT errors is equivalent to the one in osutil.c (line 393). Unfortunately, I believe the pure code is still buggy: there's still the same bug at mercurial/pure/osutil.py line 62.

Alphare accepted this revision.Feb 18 2022, 5:27 AM
Alphare added a subscriber: Alphare.

Good find, thanks!

This revision is now accepted and ready to land.Feb 18 2022, 5:27 AM
aalekseyev added a commit: rHGdcec16e799dd: status: fix hg status race against file deletion.Feb 18 2022, 5:55 AM
Closed by commit rHGdcec16e799dd: status: fix hg status race against file deletion (authored by aalekseyev). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 32266

rust/hg-core/src/dirstate_tree/status.rs

/// ///
/// * At the repository root, ignore that sub-directory /// * At the repository root, ignore that sub-directory
/// * Elsewhere, we’re listing the content of a sub-repo. Return an empty /// * Elsewhere, we’re listing the content of a sub-repo. Return an empty
/// list instead. /// list instead.
fn read_dir(path: &Path, is_at_repo_root: bool) -> io::Result<Vec<Self>> { fn read_dir(path: &Path, is_at_repo_root: bool) -> io::Result<Vec<Self>> {
let mut results = Vec::new(); let mut results = Vec::new();
for entry in path.read_dir()? { for entry in path.read_dir()? {
let entry = entry?; let entry = entry?;
let metadata = entry.metadata()?; let metadata = match entry.metadata() {
Ok(v) => v,
Err(e) => {
// race with file deletion?
if e.kind() == std::io::ErrorKind::NotFound {
continue;
} else {
return Err(e);
}
}
};
let name = get_bytes_from_os_string(entry.file_name()); let name = get_bytes_from_os_string(entry.file_name());
// FIXME don't do this when cached // FIXME don't do this when cached
if name == b".hg" { if name == b".hg" {
if is_at_repo_root { if is_at_repo_root {
// Skip the repo’s own .hg (might be a symlink) // Skip the repo’s own .hg (might be a symlink)
continue; continue;
} else if metadata.is_dir() { } else if metadata.is_dir() {
// A .hg sub-directory at another location means a subrepo, // A .hg sub-directory at another location means a subrepo,