This is an archive of the discontinued Mercurial Phabricator instance.

Differential D12458

rust-dirstatemap: add `set_possibly_dirty` method
ClosedPublic

Authored by Alphare on Apr 6 2022, 10:11 AM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHGa2b1767790e9: rust-dirstatemap: add `set_possibly_dirty` method
rHG5ded54dd1572: rust-dirstatemap: add `set_possibly_dirty` method
Summary

This is the new API that Python has already migrated to.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

Alphare created this revision.Apr 6 2022, 10:11 AM
marmoute added a subscriber: marmoute.Apr 8 2022, 7:21 AM
marmoute added inline comments.
rust/hg-core/src/dirstate_tree/dirstate_map.rs
811

(Same question as in D12456)

Alphare added inline comments.Apr 8 2022, 8:24 AM
rust/hg-core/src/dirstate_tree/dirstate_map.rs
811

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
811

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
811

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
811

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 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.
baymax updated this revision to Diff 32930.Apr 8 2022, 12:29 PM

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

baymax updated this revision to Diff 33032.Apr 12 2022, 10:57 AM

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

baymax updated this revision to Diff 33114.Apr 12 2022, 12:06 PM

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

Revision Contents
Changeset List

Diff 33114

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,
}, },
)?; )?;
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

path, mode, size, timestamp, path, mode, size, timestamp,
); );
res.or_else(|_| { res.or_else(|_| {
Err(PyErr::new::<exc::OSError, _>(py, "Dirstate error".to_string())) Err(PyErr::new::<exc::OSError, _>(py, "Dirstate error".to_string()))
})?; })?;
Ok(PyNone) Ok(PyNone)
} }
def set_possibly_dirty(&self, f: PyObject) -> PyResult<PyNone> {
let bytes = f.extract::<PyBytes>(py)?;
let path = HgPath::new(bytes.data(py));
let res = self.inner(py).borrow_mut().set_possibly_dirty(path);
res.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)>)>,