( )⚙ D7927 rust-status: add util for listing a directory

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D7927

rust-status: add util for listing a directory
ClosedPublic

Authored by Alphare on Jan 17 2020, 10:33 AM.

Details

Reviewers
marmoute
kevincox
Group Reviewers
hg-reviewers
Commits
rHG0d97bcb3cee9: rust-status: add util for listing a directory
Summary

I debated moving it to utils, but it is not used anywhere else for now, and
its skip behavior is pretty specific to status.

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.Jan 17 2020, 10:33 AM
marmoute accepted this revision.Feb 5 2020, 5:31 PM
marmoute added a subscriber: marmoute.

I am not a rust expert, but I don't see anything obviously wrong in there. I make a couple of minor comments.

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

note: took me some time to realize that skipping the full directory if there is a .hg make sense. Because we detected a another repository and that the wc is part of that repository.

Maybe follow up with a documentation update that clarify this.

74

Silly question: would it make sense to sort the filename before the loop ? It might result is less bytes to move around since the entry will not be attached yet.

Alphare marked an inline comment as done.Feb 6 2020, 10:02 AM
Alphare added inline comments.
rust/hg-core/src/dirstate/status.rs
74

I'm not entirely sure I understand your question.

marmoute added inline comments.Feb 6 2020, 2:46 PM
rust/hg-core/src/dirstate/status.rs
74

If I understand you code correctly you currently do:

filenames = listdir()
result = [(f, entry(f)) for f in filenames]
result.sort()

Would it make sense to do:

filenames = listdir()
filenames.sort()
result = [(f, entry(f)) for f in filenames]
Alphare added inline comments.Feb 10 2020, 11:11 AM
rust/hg-core/src/dirstate/status.rs
74

std::fs::read_dir returns an iterator, so I would have to collect it first, unsure if that would be more expensive... and also, a Vec of heap-allocated elements being just an array of pointers I'm not convinced that would help. Maybe we can do some benchmarking further down the line.

kevincox accepted this revision.Feb 12 2020, 5:09 AM
kevincox added inline comments.
rust/hg-core/src/dirstate/status.rs
76

It would be more clear to do .sort_by_key(|e| e.0)

And I don't think stability matters here so you could use sort_unstable_by_key.

Alphare marked an inline comment as done.Feb 12 2020, 8:53 AM
Alphare added inline comments.Feb 12 2020, 9:02 AM
rust/hg-core/src/dirstate/status.rs
76

I didn't realize that sort_unstable_by_key needed to return a T and not a &T, that forces a clone(). I'm not sure that matters, we'll go with the shortest code and see if that shows up in profiling later.

kevincox added inline comments.Feb 12 2020, 11:02 AM
rust/hg-core/src/dirstate/status.rs
76

sort_unstable_by_key doesn't require the function to return a T at all. It just needs to return an Ord type. Note that &Ord is Ord automatically.

https://doc.rust-lang.org/std/primitive.slice.html#method.sort_unstable_by_key

Alphare updated this revision to Diff 20183.Feb 13 2020, 12:36 PM
Alphare added a commit: rHG0d97bcb3cee9: rust-status: add util for listing a directory.Mar 11 2020, 12:23 PM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG0d97bcb3cee9: rust-status: add util for listing a directory (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 (31 lines)

Diff 20716

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

// status.rs // status.rs
// //
// Copyright 2019 Raphaël Gomès <rgomes@octobus.net> // Copyright 2019 Raphaël Gomès <rgomes@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.
//! Rust implementation of dirstate.status (dirstate.py). //! Rust implementation of dirstate.status (dirstate.py).
//! It is currently missing a lot of functionality compared to the Python one //! It is currently missing a lot of functionality compared to the Python one
//! and will only be triggered in narrow cases. //! and will only be triggered in narrow cases.
use crate::{ use crate::{
dirstate::SIZE_FROM_OTHER_PARENT, dirstate::SIZE_FROM_OTHER_PARENT,
matchers::Matcher, matchers::Matcher,
utils::{ utils::{
files::HgMetadata, files::HgMetadata,
hg_path::{hg_path_to_path_buf, HgPath}, hg_path::{
hg_path_to_path_buf, os_string_to_hg_path_buf, HgPath, HgPathBuf,
},
}, },
CopyMap, DirstateEntry, DirstateMap, EntryState, CopyMap, DirstateEntry, DirstateMap, EntryState,
}; };
use rayon::prelude::*; use rayon::prelude::*;
use std::collections::HashSet; use std::collections::HashSet;
use std::fs::{read_dir, DirEntry};
use std::path::Path; use std::path::Path;
/// Marker enum used to dispatch new status entries into the right collections. /// Marker enum used to dispatch new status entries into the right collections.
/// Is similar to `crate::EntryState`, but represents the transient state of /// Is similar to `crate::EntryState`, but represents the transient state of
/// entries during the lifetime of a command. /// entries during the lifetime of a command.
enum Dispatch { enum Dispatch {
Unsure, Unsure,
Modified, Modified,
/// changes. (issue2608) /// changes. (issue2608)
/// TODO I haven't found a way of having `b` be `Into<i32>`, since `From<u64>` /// 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 /// is not defined for `i32`, and there is no `As` trait. This forces the
/// caller to cast `b` as `i32`. /// caller to cast `b` as `i32`.
fn mod_compare(a: i32, b: i32) -> bool { fn mod_compare(a: i32, b: i32) -> bool {
a & i32::max_value() != b & i32::max_value() a & i32::max_value() != b & i32::max_value()
} }
/// Return a sorted list containing information about the entries
/// in the directory.
///
/// * `skip_dot_hg` - Return an empty vec if `path` contains a `.hg` directory
fn list_directory(
path: impl AsRef<Path>,
skip_dot_hg: bool,
) -> std::io::Result<Vec<(HgPathBuf, DirEntry)>> {
let mut results = vec![];
let entries = read_dir(path.as_ref())?;
for entry in entries {
let entry = entry?;
let filename = os_string_to_hg_path_buf(entry.file_name())?;
let file_type = entry.file_type()?;
marmouteUnsubmitted
Done

note: took me some time to realize that skipping the full directory if there is a .hg make sense. Because we detected a another repository and that the wc is part of that repository.

Maybe follow up with a documentation update that clarify this.

marmoute: note: took me some time to realize that skipping the full directory if there is a `.hg` make…
if skip_dot_hg && filename.as_bytes() == b".hg" && file_type.is_dir() {
return Ok(vec![]);
} else {
results.push((HgPathBuf::from(filename), entry))
}
}
marmouteUnsubmitted
Not Done

Silly question: would it make sense to sort the filename before the loop ? It might result is less bytes to move around since the entry will not be attached yet.

marmoute: Silly question: would it make sense to sort the filename before the loop ? It might result is…
AlphareAuthorUnsubmitted
Done

I'm not entirely sure I understand your question.

Alphare: I'm not entirely sure I understand your question.
marmouteUnsubmitted
Not Done

If I understand you code correctly you currently do:

filenames = listdir()
result = [(f, entry(f)) for f in filenames]
result.sort()

Would it make sense to do:

filenames = listdir()
filenames.sort()
result = [(f, entry(f)) for f in filenames]
marmoute: If I understand you code correctly you currently do: ``` filenames = listdir() result = [(f…
AlphareAuthorUnsubmitted
Done

std::fs::read_dir returns an iterator, so I would have to collect it first, unsure if that would be more expensive... and also, a Vec of heap-allocated elements being just an array of pointers I'm not convinced that would help. Maybe we can do some benchmarking further down the line.

Alphare: [[ https://doc.rust-lang.org/nightly/std/fs/fn.read_dir.html | std::fs::read_dir ]] returns an…
results.sort_unstable_by_key(|e| e.0.clone());
kevincoxUnsubmitted
Done

It would be more clear to do .sort_by_key(|e| e.0)

And I don't think stability matters here so you could use sort_unstable_by_key.

kevincox: It would be more clear to do `.sort_by_key(|e| e.0)` And I don't think stability matters here…
AlphareAuthorUnsubmitted
Done

I didn't realize that sort_unstable_by_key needed to return a T and not a &T, that forces a clone(). I'm not sure that matters, we'll go with the shortest code and see if that shows up in profiling later.

Alphare: I didn't realize that `sort_unstable_by_key` needed to return a `T` and not a `&T`, that forces…
kevincoxUnsubmitted
Not Done

sort_unstable_by_key doesn't require the function to return a T at all. It just needs to return an Ord type. Note that &Ord is Ord automatically.

https://doc.rust-lang.org/std/primitive.slice.html#method.sort_unstable_by_key

kevincox: `sort_unstable_by_key` doesn't require the function to return a T at all. It just needs to…
Ok(results)
}
/// 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,