This is an archive of the discontinued Mercurial Phabricator instance.

rust-status: add util for listing a directory
ClosedPublic

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

Download Raw Diff

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

Branch

default

Lint

No Linters Available

Unit

No Unit Test Coverage

Event Timeline

Alphare created this revision.Jan 17 2020, 10:33 AM

Herald added a reviewer: hg-reviewers. · View Herald TranscriptJan 17 2020, 10:33 AM

Herald added subscribers: mercurial-devel, kevincox, durin42. · View Herald Transcript

Alphare added a child revision: D7928: rust-status: add function for sequential traversal of the working directory.Jan 17 2020, 10:33 AM

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.

Alphare edited parent revisions, added: D7925: rust-matchers: add `IgnoreMatcher`; removed: D7926: rust-dependencies: update rayon.Feb 6 2020, 10:15 AM

Alphare added a child revision: D8086: rust-status: refactor options into a `StatusOptions` struct.

Alphare removed a child revision: D7928: rust-status: add function for sequential traversal of the working directory.

Alphare updated this revision to Diff 19942.

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

			Path	Packages
M			rust/hg-core/src/dirstate/status.rs (31 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	19416		Jan 17 2020, 10:33 AM	★	★
Diff 2	19942		Feb 6 2020, 10:15 AM	★	★
Diff 3	20183		Feb 13 2020, 12:36 PM	★	★
Diff 4	20716	rHG0d97bcb3cee9e608481967d8cb67f472cb867304	Jan 17 2020, 9:37 AM	★	★

Commit	Parents	Author	Summary	Date
84150a86d35f	af55ef6e575f	Raphaël Gomès		Jan 17 2020, 9:37 AM

Status	Author	Revision
Closed	Alphare	D8253 hg-core: add function timing information
Closed	Alphare	D8252 rust: add logging utils
Closed	Alphare	D8251 rust-status: traverse working directory in parallel
Closed	Alphare	D8250 rust-status: wrap `stat_dmap_entries` to ease profiling
Closed	Alphare	D8249 rust-status: refactor handling of unknown files
Closed	Alphare	D8215 rust-status: move to recursive traversal to prepare for parallel traversal
Closed	Alphare	D8213 rust-pathauditor: use interior mutability for use in multi-threaded contexts
Closed	Alphare	D8212 rust-pathauditor: actually populate the `audited_dirs` cache
Closed	Alphare	D8211 rust-pathauditor: actually split Windows shortname aliases at `~`
Closed	Alphare	D8226 debuginstall: add entry about re2 Rust bindings when applicable
Closed	Alphare	D8225 rust-cpython: add `debug` module to expose debug information to Python
Closed	Alphare	D7931 rust-status: use bare hg status fastpath from Python
Closed	Alphare	D7930 rust-status: update rust-cpython bridge to account for the changes in core
Closed	Alphare	D7929 rust-status: add bare `hg status` support in hg-core
Closed	Alphare	D7928 rust-status: add function for sequential traversal of the working directory
Closed	Alphare	D8088 rust-status: add missing variants to `Dispatch` enum
Closed	Alphare	D8087 rust-status: rename `StatusResult` to `DirstateStatus`
Closed	Alphare	D8086 rust-status: refactor options into a `StatusOptions` struct
Closed	Alphare	D7927 rust-status: add util for listing a directory
Closed	Alphare	D7926 rust-dependencies: update rayon
Closed	Alphare	D7925 rust-matchers: add `IgnoreMatcher`
Closed	Alphare	D7924 rust-matchers: add `build_regex_match` function
Closed	Alphare	D7923 rust-matchers: add functions to get roots, dirs and parents from patterns
Closed	Alphare	D7922 rust-matchers: add function to generate a regex matcher function
Closed	Alphare	D8224 rust-format: cleanup ancestors.rs to make rustfmt happy

Diff 19942

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_by(\|a, b\| a.0.cmp(&b.0));
				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,