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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

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
30–42

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 (34 lines)

Diff 18019

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

Unsure, Unsure,
Modified, Modified,
Added, Added,
Removed, Removed,
Deleted, Deleted,
Clean, Clean,
Unknown, Unknown,
} }
/// 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()
}
/// The file corresponding to the dirstate entry was found on the filesystem. /// The file corresponding to the dirstate entry was found on the filesystem.
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…
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,
last_normal_time: i64, last_normal_time: i64,
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