I think future changes of DirstateItem internals will be easier if state == 'd' is never something we need to represent in DirstateItem. We can do that by changing directories to return a list of (path, mtime) instead of (path, DirstateItem), and updating the two callers (debugcommands.debugdirstate and dirstate.foldmap).

SimonSapin mentioned this in D10988: dirstate-item: use the v1_serialization method in debugstate.Jul 5 2021, 6:45 AM

In D10983#167824, @SimonSapin wrote:

I think future changes of DirstateItem internals will be easier if state == 'd' is never something we need to represent in DirstateItem. We can do that by changing directories to return a list of (path, mtime) instead of (path, DirstateItem), and updating the two callers (debugcommands.debugdirstate and dirstate.foldmap).

I am not too convinced that treating directory in a different way is win. What's your motivation there ?

Currently a dirstatetuple / dirstateitem only exists for paths that are present in dirstate-v1 and for which dirstate.__contains__ returns true. It contains a state whose value is always n, a, r, or m. However we want to move towards not having this state at all other than in dirstate-v1 parsing and serialization. Adding more cases to handle makes that more difficult and isn’t necessary.

In Rust code the DirstateEntry struct corresponds to dirstatetuple / dirstateitem. Current in the tree dirstate map, "directory" nodes don’t have a DirstateEntry at all. (And soon entry-less nodes won’t be just directories but will also potentially include ignored or unknown files.) When adding this directories iterator I only made it create tuples that look kinda like dirstatetuple as a shortcut because that’s what debugcommands.debugstate was already handling.

In D10983#167853, @SimonSapin wrote:

Currently a dirstatetuple / dirstateitem only exists for paths that are present in dirstate-v1 and for which dirstate.__contains__ returns true. It contains a state whose value is always n, a, r, or m. However we want to move towards not having this state at all other than in dirstate-v1 parsing and serialization. Adding more cases to handle makes that more difficult and isn’t necessary.
In Rust code the DirstateEntry struct corresponds to dirstatetuple / dirstateitem. Current in the tree dirstate map, "directory" nodes don’t have a DirstateEntry at all. (And soon entry-less nodes won’t be just directories but will also potentially include ignored or unknown files.) When adding this directories iterator I only made it create tuples that look kinda like dirstatetuple as a shortcut because that’s what debugcommands.debugstate was already handling.

Given that we are moving toward adding more thing in the dirstate(map) in general, having an explicit entry for all of them, instead of various special case, seems simpler. I expect us to introduce dedicated boolean property to differenciate the one Mercurial will directly care about (tracked in wc on in parent) and the other ones.

Alright, we’re not gonna decide right now the future internals of dirstateitem so let’s take this patch and we can always change things again later. We’ll probably need to separate the two users of this iterator anyway:

Now that tree-based hasdir is based on "the counter of descendants with an entry is zero" instead of just "this node does not have an entry", foldmap should be changed in a same way
debugdirstate wants all nodes with the maximum of information. That could be a single iterator instead of having it combine values from two iterators.

rust/hg-cpython/src/dirstate.rs
79	https://dgrunwald.github.io/rust-cpython/doc/cpython/macro.py_capsule.html says "with a layer of caching" so it should be fine.

✅ refresh by Heptapod after a successful CI run (🐙 💚)

marmoute edited parent revisions, added: D10977: dirstate: drop the last explicit `state` usage in the module; removed: D10982: dirstate-item: rename the class to DirstateItem.Jul 6 2021, 8:12 PM

✅ refresh by Heptapod after a successful CI run (🐙 💚)

baymax added a child revision: D10988: dirstate-item: use the v1_serialization method in debugstate.Jul 8 2021, 11:04 AM

✅ refresh by Heptapod after a successful CI run (🐙 💚)

Alphare accepted this revision.Jul 9 2021, 11:09 AM

Alphare added a subscriber: Alphare.

Alphare added inline comments.

rust/hg-cpython/src/dirstate.rs
79	Yeah, it's using a `OnceCell`, so this is at most a very tiny difference to performance.

This revision is now accepted and ready to land.Jul 9 2021, 11:09 AM

marmoute added a commit: rHG7a15dea6d303: dirstate-item: also build DistateItem in dirstate.directories().Jul 9 2021, 1:19 PM

Closed by commit rHG7a15dea6d303: dirstate-item: also build DistateItem in dirstate.directories() (authored by marmoute). · Explain Why

This revision was automatically updated to reflect the committed changes.

			Path	Packages
M			rust/hg-cpython/src/dirstate.rs (13 lines)
M			rust/hg-cpython/src/dirstate/dirstate_map.rs (4 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	28804		Jul 5 2021, 5:34 AM	★	★
Diff 2	28838		Jul 6 2021, 1:35 PM	★	★
Diff 3	29009		Jul 8 2021, 11:04 AM	★	★
Diff 4	29021		Jul 8 2021, 2:27 PM	★	★
Diff 5	29046	rHG7a15dea6d303421354b0527230cfbb2378d49263	Jul 5 2021, 4:32 AM	★	★


	let maybe_obj = unsafe {			let maybe_obj = unsafe {
	let ptr = make(state_code as c_char, mode, size, mtime);			let ptr = make(state_code as c_char, mode, size, mtime);
	PyObject::from_owned_ptr_opt(py, ptr)			PyObject::from_owned_ptr_opt(py, ptr)
	};			};
	maybe_obj.ok_or_else(\|\| PyErr::fetch(py))			maybe_obj.ok_or_else(\|\| PyErr::fetch(py))
	}			}

				// XXX a bit strange to have a dedicated function, but directory are not
				// treated as dirstate node by hg-core for now so…
				pub fn make_directory_item(py: Python, mtime: i32) -> PyResult<PyObject> {
				// might be silly to retrieve capsule function in hot loop
				SimonSapinUnsubmitted Not Done https://dgrunwald.github.io/rust-cpython/doc/cpython/macro.py_capsule.html says "with a layer of caching" so it should be fine. SimonSapin: https://dgrunwald.github.io/rust-cpython/doc/cpython/macro.py_capsule.html says "with a layer…
				AlphareUnsubmitted Not Done Yeah, it's using a `OnceCell`, so this is at most a very tiny difference to performance. Alphare: Yeah, it's using a `OnceCell`, so this is at most a very tiny difference to performance.
				let make = make_dirstate_item_capi::retrieve(py)?;

				let maybe_obj = unsafe {
				let ptr = make(b'd' as c_char, 0 as i32, 0 as i32, mtime);
				PyObject::from_owned_ptr_opt(py, ptr)
				};
				maybe_obj.ok_or_else(\|\| PyErr::fetch(py))
				}

	pub fn extract_dirstate(py: Python, dmap: &PyDict) -> Result<StateMap, PyErr> {			pub fn extract_dirstate(py: Python, dmap: &PyDict) -> Result<StateMap, PyErr> {
	dmap.items(py)			dmap.items(py)
	.iter()			.iter()
	.map(\|(filename, stats)\| {			.map(\|(filename, stats)\| {
	let stats = stats.extract::<PySequence>(py)?;			let stats = stats.extract::<PySequence>(py)?;
	let state = stats.get_item(py, 0)?.extract::<PyBytes>(py)?;			let state = stats.get_item(py, 0)?.extract::<PyBytes>(py)?;
	let state =			let state =
	EntryState::try_from(state.data(py)[0]).map_err(\|e\| {			EntryState::try_from(state.data(py)[0]).map_err(\|e\| {

	use cpython::{			use cpython::{
	exc, ObjectProtocol, PyBool, PyBytes, PyClone, PyDict, PyErr, PyList,			exc, ObjectProtocol, PyBool, PyBytes, PyClone, PyDict, PyErr, PyList,
	PyObject, PyResult, PySet, PyString, Python, PythonObject, ToPyObject,			PyObject, PyResult, PySet, PyString, Python, PythonObject, ToPyObject,
	UnsafePyLeaked,			UnsafePyLeaked,
	};			};

	use crate::{			use crate::{
	dirstate::copymap::{CopyMap, CopyMapItemsIterator, CopyMapKeysIterator},			dirstate::copymap::{CopyMap, CopyMapItemsIterator, CopyMapKeysIterator},
				dirstate::make_directory_item,
	dirstate::make_dirstate_item,			dirstate::make_dirstate_item,
	dirstate::non_normal_entries::{			dirstate::non_normal_entries::{
	NonNormalEntries, NonNormalEntriesIterator,			NonNormalEntries, NonNormalEntriesIterator,
	},			},
	dirstate::owning::OwningDirstateMap,			dirstate::owning::OwningDirstateMap,
	parsers::dirstate_parents_to_pytuple,			parsers::dirstate_parents_to_pytuple,
	};			};
	use hg::{			use hg::{
	}			}

	def directories(&self) -> PyResult<PyList> {			def directories(&self) -> PyResult<PyList> {
	let dirs = PyList::new(py, &[]);			let dirs = PyList::new(py, &[]);
	for item in self.inner(py).borrow().iter_directories() {			for item in self.inner(py).borrow().iter_directories() {
	let (path, mtime) = item.map_err(\|e\| v2_error(py, e))?;			let (path, mtime) = item.map_err(\|e\| v2_error(py, e))?;
	let path = PyBytes::new(py, path.as_bytes());			let path = PyBytes::new(py, path.as_bytes());
	let mtime = mtime.map(\|t\| t.0).unwrap_or(-1);			let mtime = mtime.map(\|t\| t.0).unwrap_or(-1);
	let tuple = (path, (b'd', 0, 0, mtime));			let item = make_directory_item(py, mtime as i32)?;
				let tuple = (path, item);
	dirs.append(py, tuple.to_py_object(py).into_object())			dirs.append(py, tuple.to_py_object(py).into_object())
	}			}
	Ok(dirs)			Ok(dirs)
	}			}

	});			});

	impl DirstateMap {			impl DirstateMap {

This is an archive of the discontinued Mercurial Phabricator instance.

dirstate-item: also build DistateItem in dirstate.directories()
ClosedPublic

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 28804

rust/hg-cpython/src/dirstate.rs

rust/hg-cpython/src/dirstate/dirstate_map.rs

This is an archive of the discontinued Mercurial Phabricator instance.

dirstate-item: also build DistateItem in dirstate.directories()ClosedPublic

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 28804

rust/hg-cpython/src/dirstate.rs

rust/hg-cpython/src/dirstate/dirstate_map.rs

dirstate-item: also build DistateItem in dirstate.directories()
ClosedPublic

Revision Contents
Changeset List