rust-dirstatemap: add `set_possibly_dirty` method
ClosedPublic

rust/hg-core/src/dirstate_tree/dirstate_map.rs
808	I don't think this should affect anything. Setting an entry as `possibly_dirty` does not have any effect on its being tracked or having a copy source, or an entry. The next call to `dirstate.status` will use some other method to reset its state. Or am I missing something?

marmoute added inline comments.Apr 8 2022, 8:47 AM

rust/hg-core/src/dirstate_tree/dirstate_map.rs
808	My question is more "it seems like this call could insert a node, yet nothing is done to adjust the counter associated to this insertion". Is this a legitimate concerns ?

Alphare added inline comments.Apr 8 2022, 8:50 AM

rust/hg-core/src/dirstate_tree/dirstate_map.rs
808	This can't insert a node because of the check in the public function. There is a strong case to use `get_node_mut` instead, but it would have to be adjusted to have an ancestors callback as well. I propose to fix D12456 in the state that it is, and then add that change on top of the stack.

marmoute added inline comments.Apr 8 2022, 9:00 AM

rust/hg-core/src/dirstate_tree/dirstate_map.rs
808	It seems like we are in a "This function is correct because unrelated code never call it with argument that makes it wrong" That seems dangerous, and either and assert or a narrower function use would be safer. Doing that fix at the top of the stack up seems fines if that helps.

Alphare mentioned this in D12460: rust-dirstatemap: add `set_untracked` method.Apr 8 2022, 9:08 AM

Alphare added a commit: rHG5ded54dd1572: rust-dirstatemap: add `set_possibly_dirty` method.Apr 8 2022, 11:56 AM

This revision was not accepted when it landed; it landed in state Needs Review.

Closed by commit rHG5ded54dd1572: rust-dirstatemap: add `set_possibly_dirty` method (authored by Alphare). · Explain Why

This revision was automatically updated to reflect the committed changes.

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

Alphare added a commit: rHGa2b1767790e9: rust-dirstatemap: add `set_possibly_dirty` method.Apr 8 2022, 12:50 PM

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

Revision Contents
Changeset List

			Path	Packages
M			rust/hg-core/src/dirstate_tree/dirstate_map.rs (35 lines)
M			rust/hg-cpython/src/dirstate/dirstate_map.rs (14 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	32833		Apr 6 2022, 10:11 AM	★	★
Diff 2	32895	rHG5ded54dd157227391305f6270620529ecc14b42f	Mar 28 2022, 12:09 PM	★	★
Diff 3	32930		Apr 8 2022, 12:29 PM	★	★
Diff 4	33032		Apr 12 2022, 10:57 AM	★	★
Diff 5	33114		Apr 12 2022, 12:06 PM	★	★

Status	Author	Revision
Closed	Alphare	D12478 rust-dirstatemap: add unit tests
Closed	Alphare	D12477 rust: add `Debug` trait to a bunch of structs
Closed	Alphare	D12476 rust-dirstatemap: use `&HgPath` instead of `HgPathBuf` in `copy_map_insert`
Closed	Alphare	D12475 rust-dirstatemap: use `DirstateEntry::tracked` directly
Closed	Alphare	D12474 rust-cpython: remove unused API to `drop_entry_and_copy_source`
Closed	Alphare	D12473 rust-dirstatemap: remove `set_dirstate_entry`/`set_entry` methods
Closed	Alphare	D12472 rust-dirstatemap: implement part of the `setparents` logic
Closed	Alphare	D12471 dirstate-item: add missing bit of docstring
Closed	Alphare	D12470 dirstatemap: move `_dirs_incr` and `_dirs_decr` methods out of the common
Closed	Alphare	D12469 dirstatemap: move `_refresh_entry` out of the common methods
Closed	Alphare	D12468 dirstatemap: move `_drop_entry` out of the common methods
Closed	Alphare	D12467 rust-dirstatemap: remove `_drop_entry`
Closed	Alphare	D12466 rust-dirstatemap: remove `__settitem__`
Closed	Alphare	D12465 rust-dirstatemap: remove unused `_refresh_entry` implementation
Closed	Alphare	D12464 rust-distatemap: remove `addfile` API
Closed	Alphare	D12463 rust-dirstatemap: remove `removefile` API
Closed	Alphare	D12462 rhg: use the new `set_clean` API
Closed	Alphare	D12461 dirstatemap: move `set_untracked` out of the common methods
Closed	Alphare	D12460 rust-dirstatemap: add `set_untracked` method
Closed	Alphare	D12459 dirstatemap: move `set_possibly_dirty` out of the common methods
Closed	Alphare	D12458 rust-dirstatemap: add `set_possibly_dirty` method
Closed	Alphare	D12457 dirstatemap: move `set_clean` out of common methods
Closed	Alphare	D12456 rust-dirstatemap: add `set_clean` method
Closed	Alphare	D12455 dirstatemap: remove `_insert_entry`
Closed	Alphare	D12454 dirstatemap: move `reset_state` out of common methods
Closed	Alphare	D12453 rust-dirstatemap: add Rust implementation of `reset_state`
Closed	Alphare	D12452 rust-dirstate: introduce intermediate struct for dirstate-v2 data
Closed	Alphare	D12451 dirstatemap: remove unused parameter from `reset_state`
Closed	Alphare	D12450 dirstatemap: move `set_tracked` out of common methods and plug in Rust
Closed	Alphare	D12449 rust-dirstatemap: add `set_tracked` method
Closed	Alphare	D12448 rust-dirstate: don't return a state for untracked entries
Closed	Alphare	D12447 dirstate: remove v1_* methods from Python/C/Rust shared API
Closed	Alphare	D12446 rust-dirstate-entry: fix typo in panic message
Closed	Alphare	D12445 test-issue660: add dirstate-v2 variant
Closed	Alphare	D12444 test-issue660: test inside a repository, not the test dir
Closed	Alphare	D12443 dirstate: fix some typos in docstrings

Diff 32895

rust/hg-core/src/dirstate_tree/dirstate_map.rs

	}			}

	fn as_entry(&self) -> Option<&DirstateEntry> {			fn as_entry(&self) -> Option<&DirstateEntry> {
	match self {			match self {
	NodeData::Entry(entry) => Some(entry),			NodeData::Entry(entry) => Some(entry),
	_ => None,			_ => None,
	}			}
	}			}

				fn as_entry_mut(&mut self) -> Option<&mut DirstateEntry> {
				match self {
				NodeData::Entry(entry) => Some(entry),
				_ => None,
				}
				}
	}			}

	impl<'on_disk> DirstateMap<'on_disk> {			impl<'on_disk> DirstateMap<'on_disk> {
	pub(super) fn empty(on_disk: &'on_disk [u8]) -> Self {			pub(super) fn empty(on_disk: &'on_disk [u8]) -> Self {
	Self {			Self {
	on_disk,			on_disk,
	root: ChildNodes::default(),			root: ChildNodes::default(),
	nodes_with_entry_count: 0,			nodes_with_entry_count: 0,
	\|_ancestor\| {},			\|_ancestor\| {},
	)?;			)?;
	let mut new_entry = old_entry.clone();			let mut new_entry = old_entry.clone();
	new_entry.set_clean(mode, size, mtime);			new_entry.set_clean(mode, size, mtime);
	node.data = NodeData::Entry(new_entry);			node.data = NodeData::Entry(new_entry);
	Ok(())			Ok(())
	}			}

				fn set_possibly_dirty(
				&mut self,
				filename: &HgPath,
				) -> Result<(), DirstateError> {
				let node = Self::get_or_insert_node(
				self.on_disk,
				&mut self.unreachable_bytes,
				&mut self.root,
				filename,
				WithBasename::to_cow_owned,
				\|_ancestor\| {},
				marmouteUnsubmitted Not Done (Same question as in D12456) marmoute: (Same question as in D12456)
				AlphareAuthorUnsubmitted Done I don't think this should affect anything. Setting an entry as `possibly_dirty` does not have any effect on its being tracked or having a copy source, or an entry. The next call to `dirstate.status` will use some other method to reset its state. Or am I missing something? Alphare: I don't think this should affect anything. Setting an entry as `possibly_dirty` does not have…
				marmouteUnsubmitted Not Done My question is more "it seems like this call could insert a node, yet nothing is done to adjust the counter associated to this insertion". Is this a legitimate concerns ? marmoute: My question is more "it seems like this call could insert a node, yet nothing is done to adjust…
				AlphareAuthorUnsubmitted Done This can't insert a node because of the check in the public function. There is a strong case to use `get_node_mut` instead, but it would have to be adjusted to have an ancestors callback as well. I propose to fix D12456 in the state that it is, and then add that change on top of the stack. Alphare: This can't insert a node because of the check in the public function. There is a strong case to…
				marmouteUnsubmitted Not Done It seems like we are in a "This function is correct because unrelated code never call it with argument that makes it wrong" That seems dangerous, and either and assert or a narrower function use would be safer. Doing that fix at the top of the stack up seems fines if that helps. marmoute: It seems like we are in a "This function is correct because unrelated code never call it with…
				)?;
				let entry = node.data.as_entry_mut().expect("entry should exist");
				entry.set_possibly_dirty();
				node.data = NodeData::Entry(*entry);
				Ok(())
				}

	fn iter_nodes<'tree>(			fn iter_nodes<'tree>(
	&'tree self,			&'tree self,
	) -> impl Iterator<			) -> impl Iterator<
	Item = Result<NodeRef<'tree, 'on_disk>, DirstateV2ParseError>,			Item = Result<NodeRef<'tree, 'on_disk>, DirstateV2ParseError>,
	> + 'tree {			> + 'tree {
	// Depth first tree traversal.			// Depth first tree traversal.
	//			//
	// If we could afford internal iteration and recursion,			// If we could afford internal iteration and recursion,
	Some(e) => e,			Some(e) => e,
	};			};
	self.copy_map_remove(filename)?;			self.copy_map_remove(filename)?;
	self.with_dmap_mut(\|map\| {			self.with_dmap_mut(\|map\| {
	map.set_clean(filename, old_entry, mode, size, mtime)			map.set_clean(filename, old_entry, mode, size, mtime)
	})			})
	}			}

				pub fn set_possibly_dirty(
				&mut self,
				filename: &HgPath,
				) -> Result<(), DirstateError> {
				if self.get(filename)?.is_none() {
				return Err(DirstateMapError::PathNotFound(filename.into()).into());
				}
				self.with_dmap_mut(\|map\| map.set_possibly_dirty(filename))
				}

	pub fn reset_state(			pub fn reset_state(
	&mut self,			&mut self,
	filename: &HgPath,			filename: &HgPath,
	wc_tracked: bool,			wc_tracked: bool,
	p1_tracked: bool,			p1_tracked: bool,
	p2_info: bool,			p2_info: bool,
	has_meaningful_mtime: bool,			has_meaningful_mtime: bool,
	parent_file_data_opt: Option<ParentFileData>,			parent_file_data_opt: Option<ParentFileData>,

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

	Err(PyErr::new::<exc::OSError, _>(			Err(PyErr::new::<exc::OSError, _>(
	py,			py,
	"Dirstate error".to_string(),			"Dirstate error".to_string(),
	))			))
	})?;			})?;
	Ok(PyNone)			Ok(PyNone)
	}			}

				def set_possibly_dirty(&self, f: PyObject) -> PyResult<PyNone> {
				self.inner(py).borrow_mut()
				.set_possibly_dirty(
				HgPath::new(f.extract::<PyBytes>(py)?.data(py)),
				)
				.or_else(\|_\| {
				Err(PyErr::new::<exc::OSError, _>(
				py,
				"Dirstate error".to_string(),
				))
				})?;
				Ok(PyNone)
				}

	def reset_state(			def reset_state(
	&self,			&self,
	f: PyObject,			f: PyObject,
	wc_tracked: bool,			wc_tracked: bool,
	p1_tracked: bool,			p1_tracked: bool,
	p2_info: bool,			p2_info: bool,
	has_meaningful_mtime: bool,			has_meaningful_mtime: bool,
	parentfiledata: Option<(u32, u32, Option<(i64, u32, bool)>)>,			parentfiledata: Option<(u32, u32, Option<(i64, u32, bool)>)>,

This is an archive of the discontinued Mercurial Phabricator instance.

rust-dirstatemap: add `set_possibly_dirty` methodClosedPublic

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 32895

rust/hg-core/src/dirstate_tree/dirstate_map.rs

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

rust-dirstatemap: add `set_possibly_dirty` method
ClosedPublic

Revision Contents
Changeset List