This is an archive of the discontinued Mercurial Phabricator instance.

Differential D11840

rhg: Update the dirstate on disk after status
ClosedPublic

Authored by SimonSapin on Dec 2 2021, 11:48 AM.

Details

Reviewers
Alphare
Group Reviewers
hg-reviewers
Commits
rHG000130cfafb6: rhg: Update the dirstate on disk after status

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

SimonSapin created this revision.Dec 2 2021, 11:48 AM
Alphare requested changes to this revision.Dec 3 2021, 9:12 AM
Alphare added a subscriber: Alphare.
Alphare added inline comments.
rust/rhg/src/commands/status.rs
443

I think we should add a comment here explaining why this racy code is not racy (because it's gated behind the mtime_boundary and any change that is caught in this boundary is discarded from this anyway).

This revision now requires changes to proceed.Dec 3 2021, 9:12 AM
SimonSapin updated this revision to Diff 31321.Dec 6 2021, 5:03 AM
SimonSapin updated this revision to Diff 31326.Dec 6 2021, 6:48 PM
SimonSapin added inline comments.Dec 7 2021, 4:07 AM
rust/rhg/src/commands/status.rs
443

This was actually missing all of the mtime_boundary logic since I had ported to Rust an earlier version of the Python code, without the recent changes that added mtime_boundary during ambiguous file resolution. This is fixed in the latest revision of this patch.

Alphare accepted this revision.Dec 7 2021, 9:59 AM
This revision is now accepted and ready to land.Dec 7 2021, 9:59 AM
SimonSapin added a commit: rHG000130cfafb6: rhg: Update the dirstate on disk after status.Dec 7 2021, 10:05 AM
Closed by commit rHG000130cfafb6: rhg: Update the dirstate on disk after status (authored by SimonSapin). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 31269

rust/hg-core/src/dirstate/entry.rs

} }
} }
}; };
Self::new_truncate(seconds, nanoseconds) Self::new_truncate(seconds, nanoseconds)
} }
} }
const NSEC_PER_SEC: u32 = 1_000_000_000; const NSEC_PER_SEC: u32 = 1_000_000_000;
const RANGE_MASK_31BIT: u32 = 0x7FFF_FFFF; pub const RANGE_MASK_31BIT: u32 = 0x7FFF_FFFF;
pub const MTIME_UNSET: i32 = -1; pub const MTIME_UNSET: i32 = -1;
/// A `DirstateEntry` with a size of `-2` means that it was merged from the /// A `DirstateEntry` with a size of `-2` means that it was merged from the
/// other parent. This allows revert to pick the right status back during a /// other parent. This allows revert to pick the right status back during a
/// merge. /// merge.
pub const SIZE_FROM_OTHER_PARENT: i32 = -2; pub const SIZE_FROM_OTHER_PARENT: i32 = -2;
/// A special value used for internal representation of special case in /// A special value used for internal representation of special case in

rust/rhg/src/commands/status.rs

// status.rs // status.rs
// //
// Copyright 2020, Georges Racinet <georges.racinets@octobus.net> // Copyright 2020, Georges Racinet <georges.racinets@octobus.net>
// //
// This software may be used and distributed according to the terms of the // This software may be used and distributed according to the terms of the
// GNU General Public License version 2 or any later version. // GNU General Public License version 2 or any later version.
use crate::error::CommandError; use crate::error::CommandError;
use crate::ui::Ui; use crate::ui::Ui;
use crate::utils::path_utils::relativize_paths; use crate::utils::path_utils::relativize_paths;
use clap::{Arg, SubCommand}; use clap::{Arg, SubCommand};
use format_bytes::format_bytes; use format_bytes::format_bytes;
use hg; use hg;
use hg::config::Config; use hg::config::Config;
use hg::dirstate::has_exec_bit; use hg::dirstate::has_exec_bit;
use hg::errors::HgError; use hg::dirstate::TruncatedTimestamp;
use hg::dirstate::RANGE_MASK_31BIT;
use hg::errors::{HgError, IoResultExt};
use hg::lock::LockError;
use hg::manifest::Manifest; use hg::manifest::Manifest;
use hg::matchers::AlwaysMatcher; use hg::matchers::AlwaysMatcher;
use hg::repo::Repo; use hg::repo::Repo;
use hg::utils::files::get_bytes_from_os_string; use hg::utils::files::get_bytes_from_os_string;
use hg::utils::hg_path::{hg_path_to_os_string, HgPath}; use hg::utils::hg_path::{hg_path_to_path_buf, HgPath};
use hg::{HgPathCow, StatusOptions}; use hg::{HgPathCow, StatusOptions};
use log::{info, warn}; use log::{info, warn};
use std::io;
pub const HELP_TEXT: &str = " pub const HELP_TEXT: &str = "
Show changed files in the working directory Show changed files in the working directory
This is a pure Rust version of `hg status`. This is a pure Rust version of `hg status`.
Some options might be missing, check the list below. Some options might be missing, check the list below.
"; ";
warn!("Bad matches {:?}", &(ds_status.bad)) warn!("Bad matches {:?}", &(ds_status.bad))
} }
if !ds_status.unsure.is_empty() { if !ds_status.unsure.is_empty() {
info!( info!(
"Files to be rechecked by retrieval from filelog: {:?}", "Files to be rechecked by retrieval from filelog: {:?}",
&ds_status.unsure &ds_status.unsure
); );
} }
let mut fixup = Vec::new();
if !ds_status.unsure.is_empty() if !ds_status.unsure.is_empty()
&& (display_states.modified || display_states.clean) && (display_states.modified || display_states.clean)
{ {
let p1 = repo.dirstate_parents()?.p1; let p1 = repo.dirstate_parents()?.p1;
let manifest = repo.manifest_for_node(p1).map_err(|e| { let manifest = repo.manifest_for_node(p1).map_err(|e| {
CommandError::from((e, &*format!("{:x}", p1.short()))) CommandError::from((e, &*format!("{:x}", p1.short())))
})?; })?;
for to_check in ds_status.unsure { for to_check in ds_status.unsure {
if unsure_is_modified(repo, &manifest, &to_check)? { if let Some((mode, size, mtime)) =
if display_states.modified { unsure_is_clean(repo, &manifest, &to_check)?
ds_status.modified.push(to_check); {
if display_states.clean {
ds_status.clean.push(to_check.clone());
} }
fixup.push((to_check.into_owned(), mode, size, mtime))
} else { } else {
if display_states.clean { if display_states.modified {
ds_status.clean.push(to_check); ds_status.modified.push(to_check);
} }
} }
} }
} }
if display_states.modified { if display_states.modified {
display_status_paths( display_status_paths(
ui, ui,
repo, repo,
ui, ui,
repo, repo,
config, config,
no_status, no_status,
&mut ds_status.clean, &mut ds_status.clean,
b"C", b"C",
)?; )?;
} }
if !fixup.is_empty() {
// Update the dirstate on disk if we can
let with_lock_result =
repo.try_with_wlock_no_wait(|| -> Result<(), CommandError> {
for (path, mode, size, mtime) in fixup {
let mut entry = dmap
.get(&path)?
.expect("ambiguous file not in dirstate");
entry.set_clean(mode, size, mtime);
dmap.add_file(&path, entry)?
}
drop(dmap); // Avoid "already mutably borrowed" RefCell panics
repo.write_dirstate()?;
Ok(())
});
match with_lock_result {
Ok(closure_result) => closure_result?,
Err(LockError::AlreadyHeld) => {
// Not updating the dirstate is not ideal but not critical:
// don’t keep our caller waiting until some other Mercurial process releases the lock.
}
Err(LockError::Other(HgError::IoError { error, .. }))
if error.kind() == io::ErrorKind::PermissionDenied =>
{
// `hg status` on a read-only repository is fine
}
Err(LockError::Other(error)) => {
// Report other I/O errors
Err(error)?
}
}
}
Ok(()) Ok(())
} }
// Probably more elegant to use a Deref or Borrow trait rather than // Probably more elegant to use a Deref or Borrow trait rather than
// harcode HgPathBuf, but probably not really useful at this point // harcode HgPathBuf, but probably not really useful at this point
fn display_status_paths( fn display_status_paths(
ui: &Ui, ui: &Ui,
repo: &Repo, repo: &Repo,
} }
Ok(()) Ok(())
} }
/// Check if a file is modified by comparing actual repo store and file system. /// Check if a file is modified by comparing actual repo store and file system.
/// ///
/// This meant to be used for those that the dirstate cannot resolve, due /// This meant to be used for those that the dirstate cannot resolve, due
/// to time resolution limits. /// to time resolution limits.
fn unsure_is_modified( ///
/// If the file is clean, return `Some((mode, size, mtime))` to update its dirstate entry.
fn unsure_is_clean(
repo: &Repo, repo: &Repo,
manifest: &Manifest, manifest: &Manifest,
hg_path: &HgPath, hg_path: &HgPath,
) -> Result<bool, HgError> { ) -> Result<Option<(u32, u32, TruncatedTimestamp)>, HgError> {
let vfs = repo.working_directory_vfs(); let vfs = repo.working_directory_vfs();
let fs_path = hg_path_to_os_string(hg_path).expect("HgPath conversion"); let fs_path = hg_path_to_path_buf(hg_path).expect("HgPath conversion");
let fs_metadata = vfs.symlink_metadata(&fs_path)?; let fs_metadata = vfs.symlink_metadata(&fs_path)?;
let is_symlink = fs_metadata.file_type().is_symlink(); let is_symlink = fs_metadata.file_type().is_symlink();
// TODO: Also account for `FALLBACK_SYMLINK` and `FALLBACK_EXEC` from the // TODO: Also account for `FALLBACK_SYMLINK` and `FALLBACK_EXEC` from the
// dirstate // dirstate
let fs_flags = if is_symlink { let fs_flags = if is_symlink {
Some(b'l') Some(b'l')
} else if has_exec_bit(&fs_metadata) { } else if has_exec_bit(&fs_metadata) {
Some(b'x') Some(b'x')
} else { } else {
None None
}; };
let entry = manifest let entry = manifest
.find_file(hg_path)? .find_file(hg_path)?
.expect("ambgious file not in p1"); .expect("ambgious file not in p1");
if entry.flags != fs_flags { if entry.flags != fs_flags {
return Ok(true); return Ok(None);
} }
let filelog = repo.filelog(hg_path)?; let filelog = repo.filelog(hg_path)?;
let filelog_entry = let filelog_entry =
filelog.data_for_node(entry.node_id()?).map_err(|_| { filelog.data_for_node(entry.node_id()?).map_err(|_| {
HgError::corrupted("filelog missing node from manifest") HgError::corrupted("filelog missing node from manifest")
})?; })?;
let contents_in_p1 = filelog_entry.data()?; let contents_in_p1 = filelog_entry.data()?;
let fs_contents = if is_symlink { let fs_contents = if is_symlink {
get_bytes_from_os_string(vfs.read_link(fs_path)?.into_os_string()) get_bytes_from_os_string(vfs.read_link(&fs_path)?.into_os_string())
} else { } else {
vfs.read(fs_path)? vfs.read(&fs_path)?
}; };
return Ok(contents_in_p1 != &*fs_contents); if contents_in_p1 != &*fs_contents {
Ok(None)
} else {
use std::os::unix::fs::MetadataExt;
let mode = fs_metadata.mode();
let size = fs_contents.len() as u32 & RANGE_MASK_31BIT;
let mtime = TruncatedTimestamp::for_mtime_of(&fs_metadata)
AlphareUnsubmitted
Not Done

I think we should add a comment here explaining why this racy code is not racy (because it's gated behind the mtime_boundary and any change that is caught in this boundary is discarded from this anyway).

Alphare: I think we should add a comment here explaining why this racy code is not racy (because it's…
SimonSapinAuthorUnsubmitted
Done

This was actually missing all of the mtime_boundary logic since I had ported to Rust an earlier version of the Python code, without the recent changes that added mtime_boundary during ambiguous file resolution. This is fixed in the latest revision of this patch.

SimonSapin: This was actually missing all of the `mtime_boundary` logic since I had ported to Rust an…
.when_reading_file(&fs_path)?;
Ok(Some((mode, size, mtime)))
}
} }

tests/test-backout.t

$ echo 'newly added' > c $ echo 'newly added' > c
$ hg add c $ hg add c
$ hg remove b $ hg remove b
$ hg commit -d '5 0' -m 'prepare for subsequent backout' $ hg commit -d '5 0' -m 'prepare for subsequent backout'
$ touch -t 200001010000 c $ touch -t 200001010000 c
$ hg status -A $ hg status -A
C c C c
$ hg debugstate --no-dates $ hg debugstate --no-dates
n 644 12 set c (no-rhg !) n 644 12 set c
n 0 -1 unset c (rhg known-bad-output !)
$ hg backout -d '6 0' -m 'to be rollback-ed soon' -r . $ hg backout -d '6 0' -m 'to be rollback-ed soon' -r .
removing c removing c
adding b adding b
changeset 6:4bfec048029d backs out changeset 5:fac0b729a654 changeset 6:4bfec048029d backs out changeset 5:fac0b729a654
$ hg rollback -q $ hg rollback -q
$ hg status -A $ hg status -A
A b A b
R c R c

tests/test-dirstate-race2.t

#testcases dirstate-v1 dirstate-v2 #testcases dirstate-v1 dirstate-v2
#if dirstate-v2 #if dirstate-v2
$ cat >> $HGRCPATH << EOF $ cat >> $HGRCPATH << EOF
> [format] > [format]
> exp-rc-dirstate-v2=1 > exp-rc-dirstate-v2=1
> [storage] > [storage]
> dirstate-v2.slow-path=allow > dirstate-v2.slow-path=allow
> EOF > EOF
#endif #endif
TODO: fix rhg bugs that make this test fail when status is enabled
$ unset RHG_STATUS
Checking the size/permissions/file-type of files stored in the Checking the size/permissions/file-type of files stored in the
dirstate after an update where the files are changed concurrently dirstate after an update where the files are changed concurrently
outside of hg's control. outside of hg's control.
$ hg init repo $ hg init repo
$ cd repo $ cd repo
$ echo a > a $ echo a > a
$ hg commit -qAm _ $ hg commit -qAm _

tests/test-dirstate.t

#testcases dirstate-v1 dirstate-v2 #testcases dirstate-v1 dirstate-v2
#if dirstate-v2 #if dirstate-v2
$ cat >> $HGRCPATH << EOF $ cat >> $HGRCPATH << EOF
> [format] > [format]
> exp-rc-dirstate-v2=1 > exp-rc-dirstate-v2=1
> [storage] > [storage]
> dirstate-v2.slow-path=allow > dirstate-v2.slow-path=allow
> EOF > EOF
#endif #endif
TODO: fix rhg bugs that make this test fail when status is enabled
$ unset RHG_STATUS
------ Test dirstate._dirs refcounting ------ Test dirstate._dirs refcounting
$ hg init t $ hg init t
$ cd t $ cd t
$ mkdir -p a/b/c/d $ mkdir -p a/b/c/d
$ touch a/b/c/d/x $ touch a/b/c/d/x
$ touch a/b/c/d/y $ touch a/b/c/d/y
$ touch a/b/c/d/z $ touch a/b/c/d/z