( )⚙ D7300 rust-status: refactor dispatch case for normal files

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D7300

rust-status: refactor dispatch case for normal files
ClosedPublic

Authored by Alphare on Nov 7 2019, 5:18 AM.

Details

Reviewers
kevincox
Group Reviewers
hg-reviewers
Commits
rHG51cd86735608: rust-status: refactor dispatch case for normal files
Summary

This should make the code easier to read and more idiomatic.

Diff Detail

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

Event Timeline

Alphare created this revision.Nov 7 2019, 5:18 AM
kevincox accepted this revision.Nov 7 2019, 1:24 PM
kevincox added inline comments.
rust/hg-core/src/dirstate/status.rs
31

I see a lot of these and in general I would recommend against it. The compiler is smart. Unless profiling shows an advantage in a specific case I would leave them out.

Alphare updated this revision to Diff 17741.Nov 8 2019, 7:51 AM
Alphare marked an inline comment as done.Nov 8 2019, 7:54 AM
Alphare added a commit: rHG51cd86735608: rust-status: refactor dispatch case for normal files.Nov 11 2019, 4:08 PM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG51cd86735608: rust-status: refactor dispatch case for normal files (authored by Alphare). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mrust/hg-core/src/dirstate/status.rs (35 lines)

Diff 17713

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

Modified, Modified,
Added, Added,
Removed, Removed,
Deleted, Deleted,
Clean, Clean,
Unknown, Unknown,
} }
#[inline] #[inline]
kevincoxUnsubmitted
Done

I see a lot of these and in general I would recommend against it. The compiler is smart. Unless profiling shows an advantage in a specific case I would leave them out.

kevincox: I see a lot of these and in general I would recommend against it. The compiler is smart. Unless…
/// Dates and times that are outside the 31-bit signed range are compared
/// modulo 2^31. This should prevent hg from behaving badly with very large
/// files or corrupt dates while still having a high probability of detecting
/// changes. (issue2608)
/// TODO I haven't found a way of having `b` be `Into<i32>`, since `From<u64>`
/// is not defined for `i32`, and there is no `As` trait. This forces the
/// caller to cast `b` as `i32`.
fn mod_compare(a: i32, b: i32) -> bool {
a & i32::max_value() != b & i32::max_value()
}
#[inline]
/// The file corresponding to the dirstate entry was found on the filesystem. /// The file corresponding to the dirstate entry was found on the filesystem.
fn dispatch_found( fn dispatch_found(
filename: impl AsRef<HgPath>, filename: impl AsRef<HgPath>,
entry: DirstateEntry, entry: DirstateEntry,
metadata: HgMetadata, metadata: HgMetadata,
copy_map: &CopyMap, copy_map: &CopyMap,
check_exec: bool, check_exec: bool,
list_clean: bool, list_clean: bool,
st_mode, st_mode,
st_size, st_size,
st_mtime, st_mtime,
.. ..
} = metadata; } = metadata;
match state { match state {
EntryState::Normal => { EntryState::Normal => {
// Dates and times that are outside the 31-bit signed let size_changed = mod_compare(size, st_size as i32);
// range are compared modulo 2^31. This should prevent
// it from behaving badly with very large files or
// corrupt dates while still having a high probability
// of detecting changes. (issue2608)
let range_mask = 0x7fffffff;
let size_changed = (size != st_size as i32)
&& size != (st_size as i32 & range_mask);
let mode_changed = let mode_changed =
(mode ^ st_mode as i32) & 0o100 != 0o000 && check_exec; (mode ^ st_mode as i32) & 0o100 != 0o000 && check_exec;
if size >= 0 let metadata_changed = size >= 0 && (size_changed || mode_changed);
&& (size_changed || mode_changed) let other_parent = size == -2;
|| size == -2 // other parent if metadata_changed
|| other_parent
|| copy_map.contains_key(filename.as_ref()) || copy_map.contains_key(filename.as_ref())
{ {
Dispatch::Modified Dispatch::Modified
} else if mtime != st_mtime as i32 } else if mod_compare(mtime, st_mtime as i32) {
&& mtime != (st_mtime as i32 & range_mask)
{
Dispatch::Unsure Dispatch::Unsure
} else if st_mtime == last_normal_time { } else if st_mtime == last_normal_time {
// the file may have just been marked as normal and // the file may have just been marked as normal and
// it may have changed in the same second without // it may have changed in the same second without
// changing its size. This can happen if we quickly // changing its size. This can happen if we quickly
// do multiple commits. Force lookup, so we don't // do multiple commits. Force lookup, so we don't
// miss such a racy file change. // miss such a racy file change.
Dispatch::Unsure Dispatch::Unsure