This is an archive of the discontinued Mercurial Phabricator instance.

Differential D11889

rhg: Set second_ambiguous as needed in post-status fixup
ClosedPublic

Authored by SimonSapin on Dec 9 2021, 4:55 AM.

Details

Reviewers
Alphare
Group Reviewers
hg-reviewers
Commits
rHG112184713852: rhg: Set second_ambiguous as needed in post-status fixup
Summary

This fixes an intermittent bug that manifested only in test-revert.t,
and unfortunately not on CI. On a fast enough machine we could have:

  1. A file is modified
  2. rhg status writes an updated dirstate-v1
  3. The same file is modified again

… all within the same integer second. Because the dirstate-v1 file format
does not store sub-second precision, step 2 must write the file’s mtime
as "unknown" because of the possibility of step 3.

However, most of the code now handles timestamps with nanosecond precision
in order to take advantage of it in dirstate-v2. second_ambiguous must
be set for timestamps that become ambiguous if sub-second precision is dropped
(such as through serialization in dirstate-v1 format).

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

SimonSapin created this revision.Dec 9 2021, 4:55 AM
Alphare accepted this revision.Dec 10 2021, 5:38 AM
This revision is now accepted and ready to land.Dec 10 2021, 5:38 AM
Alphare added a comment.EditedDec 10 2021, 8:26 AM

Does not pass CI, I've removed it from the queue, please re-send. (oops, that was meant for D11890)

Revision Contents
Changeset List

Diff 31407

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

nanoseconds, nanoseconds,
second_ambiguous, second_ambiguous,
}) })
} else { } else {
Err(DirstateV2ParseError) Err(DirstateV2ParseError)
} }
} }
/// Returns a `TruncatedTimestamp` for the modification time of `metadata`.
///
/// Propagates errors from `std` on platforms where modification time
/// is not available at all.
pub fn for_mtime_of(metadata: &fs::Metadata) -> io::Result<Self> { pub fn for_mtime_of(metadata: &fs::Metadata) -> io::Result<Self> {
#[cfg(unix)] #[cfg(unix)]
{ {
use std::os::unix::fs::MetadataExt; use std::os::unix::fs::MetadataExt;
let seconds = metadata.mtime(); let seconds = metadata.mtime();
// i64!-> u32 with value always in the `0 .. NSEC_PER_SEC` range // i64!-> u32 with value always in the `0 .. NSEC_PER_SEC` range
let nanoseconds = metadata.mtime_nsec().try_into().unwrap(); let nanoseconds = metadata.mtime_nsec().try_into().unwrap();
Ok(Self::new_truncate(seconds, nanoseconds, false)) Ok(Self::new_truncate(seconds, nanoseconds, false))
} }
#[cfg(not(unix))] #[cfg(not(unix))]
{ {
metadata.modified().map(Self::from) metadata.modified().map(Self::from)
} }
} }
/// Returns whether this timestamp is reliable as the "mtime" of a file. /// Like `for_mtime_of`, but may return `None` or a value with
/// `second_ambiguous` set if the mtime is not "reliable".
/// ///
/// A modification time is reliable if it is older than `boundary` (or /// A modification time is reliable if it is older than `boundary` (or
/// sufficiently in the future). /// sufficiently in the future).
/// ///
/// Otherwise a concurrent modification might happens with the same mtime. /// Otherwise a concurrent modification might happens with the same mtime.
pub fn is_reliable_mtime(&self, boundary: &Self) -> bool { pub fn for_reliable_mtime_of(
metadata: &fs::Metadata,
boundary: &Self,
) -> io::Result<Option<Self>> {
let mut mtime = Self::for_mtime_of(metadata)?;
// If the mtime of the ambiguous file is younger (or equal) to the // If the mtime of the ambiguous file is younger (or equal) to the
// starting point of the `status` walk, we cannot garantee that // starting point of the `status` walk, we cannot garantee that
// another, racy, write will not happen right after with the same mtime // another, racy, write will not happen right after with the same mtime
// and we cannot cache the information. // and we cannot cache the information.
// //
// However if the mtime is far away in the future, this is likely some // However if the mtime is far away in the future, this is likely some
// mismatch between the current clock and previous file system // mismatch between the current clock and previous file system
// operation. So mtime more than one days in the future are considered // operation. So mtime more than one days in the future are considered
// fine. // fine.
if self.truncated_seconds == boundary.truncated_seconds { let reliable = if mtime.truncated_seconds == boundary.truncated_seconds
self.nanoseconds != 0 {
mtime.second_ambiguous = true;
mtime.nanoseconds != 0
&& boundary.nanoseconds != 0 && boundary.nanoseconds != 0
&& self.nanoseconds < boundary.nanoseconds && mtime.nanoseconds < boundary.nanoseconds
} else { } else {
// `truncated_seconds` is less than 2**31, // `truncated_seconds` is less than 2**31,
// so this does not overflow `u32`: // so this does not overflow `u32`:
let one_day_later = boundary.truncated_seconds + 24 * 3600; let one_day_later = boundary.truncated_seconds + 24 * 3600;
self.truncated_seconds < boundary.truncated_seconds mtime.truncated_seconds < boundary.truncated_seconds
|| self.truncated_seconds > one_day_later || mtime.truncated_seconds > one_day_later
};
if reliable {
Ok(Some(mtime))
} else {
Ok(None)
} }
} }
/// The lower 31 bits of the number of seconds since the epoch. /// The lower 31 bits of the number of seconds since the epoch.
pub fn truncated_seconds(&self) -> u32 { pub fn truncated_seconds(&self) -> u32 {
self.truncated_seconds self.truncated_seconds
} }

rust/rhg/src/commands/status.rs

.expect("HgPath conversion"); .expect("HgPath conversion");
// Specifically do not reuse `fs_metadata` from // Specifically do not reuse `fs_metadata` from
// `unsure_is_clean` which was needed before reading // `unsure_is_clean` which was needed before reading
// contents. Here we access metadata again after reading // contents. Here we access metadata again after reading
// content, in case it changed in the meantime. // content, in case it changed in the meantime.
let fs_metadata = repo let fs_metadata = repo
.working_directory_vfs() .working_directory_vfs()
.symlink_metadata(&fs_path)?; .symlink_metadata(&fs_path)?;
let mtime = TruncatedTimestamp::for_mtime_of(&fs_metadata) if let Some(mtime) =
.when_reading_file(&fs_path)?; TruncatedTimestamp::for_reliable_mtime_of(
if mtime.is_reliable_mtime(&mtime_boundary) { &fs_metadata,
&mtime_boundary,
)
.when_reading_file(&fs_path)?
{
let mode = fs_metadata.mode(); let mode = fs_metadata.mode();
let size = fs_metadata.len() as u32 & RANGE_MASK_31BIT; let size = fs_metadata.len() as u32 & RANGE_MASK_31BIT;
let mut entry = dmap let mut entry = dmap
.get(&hg_path)? .get(&hg_path)?
.expect("ambiguous file not in dirstate"); .expect("ambiguous file not in dirstate");
entry.set_clean(mode, size, mtime); entry.set_clean(mode, size, mtime);
dmap.add_file(&hg_path, entry)?; dmap.add_file(&hg_path, entry)?;
dirstate_write_needed = true dirstate_write_needed = true