| hg-reviewers |
For now we’re still writing the entire data every time, so appending is not
useful yet. Later we’ll have new nodes pointing to some existing data for
nodes and paths that haven’t changed.
The decision whether to append is pseudo-random in order to make tests exercise
both code paths. This will be replaced by a heuristic based on the amount
of unused existing data.
| No Linters Available |
| No Unit Test Coverage |
| mercurial/dirstatemap.py | ||
|---|---|---|
| 672 | I inlined append from vfs.py in order to be able to add the assert with .tell(). Is vfs.py also buggy? Silently overwriting extra data seems very wrong to me. If someone else wrote it, overwriting it would interfere with them. However due to the combination of repofilecache + wlock, my understanding is that there should never be extra data. | |
| mercurial/dirstatemap.py | ||
|---|---|---|
| 672 | I am refering to the issue pointed here: https://www.mercurial-scm.org/repo/hg/file/stable/mercurial/revlog.py#l2427 So vfs.append is possibly buggy :-/ There should never be extra data, except if we decide this is a good idea in some case to being friendlier to mmap user. For example, we might decide to do so in hg rollback or decide that truncate less transaction rollback is useful. | |
| rust/hg-core/src/dirstate_tree/on_disk.rs | ||
| 666 | spotted on the go "curent_offset" instead of "current_offset" | |
On top of what @marmoute said and just so it's said somewhere, I think it'd be nice to be explicit in documenting the new format by saying that the "append-only" nature of the format, even when looking at a single file, must not be relied on by code outside of the read/write core dirstate, since it's not always append-only.
| Path | Packages | |||
|---|---|---|---|---|
| M | mercurial/dirstatemap.py (34 lines) | |||
| M | rust/hg-core/src/dirstate_tree/dirstate_map.rs (29 lines) | |||
| M | rust/hg-core/src/dirstate_tree/dispatch.rs (18 lines) | |||
| M | rust/hg-core/src/dirstate_tree/on_disk.rs (213 lines) | |||
| M | rust/hg-cpython/src/dirstate/dirstate_map.rs (15 lines) | |||
| M | rust/hg-cpython/src/dirstate/dispatch.rs (8 lines) |
| Commit | Parents | Author | Summary | Date |
|---|---|---|---|---|
| 160d94faad1a | 6264fbce9c1f | Simon Sapin | Jul 13 2021, 11:18 AM |
| Status | Author | Revision | |
|---|---|---|---|
| Closed | SimonSapin | ||
| Closed | SimonSapin | ||
| Closed | SimonSapin | ||
| Closed | SimonSapin | ||
| Closed | SimonSapin | ||
| Closed | SimonSapin | ||
| Closed | SimonSapin | ||
| Closed | SimonSapin | ||
| Closed | SimonSapin | ||
| Closed | SimonSapin | ||
| Closed | SimonSapin |
| self.setparents(*parents) | self.setparents(*parents) | ||||
| self.__contains__ = self._rustmap.__contains__ | self.__contains__ = self._rustmap.__contains__ | ||||
| self.__getitem__ = self._rustmap.__getitem__ | self.__getitem__ = self._rustmap.__getitem__ | ||||
| self.get = self._rustmap.get | self.get = self._rustmap.get | ||||
| return self._rustmap | return self._rustmap | ||||
| def write(self, tr, st, now): | def write(self, tr, st, now): | ||||
| if self._use_dirstate_v2: | if not self._use_dirstate_v2: | ||||
| packed = self._rustmap.write_v2(now) | p1, p2 = self.parents() | ||||
| packed = self._rustmap.write_v1(p1, p2, now) | |||||
| st.write(packed) | |||||
| st.close() | |||||
| return | |||||
| # We can only append to an existing data file if there is one | |||||
| can_append = self.docket.uuid is not None | |||||
| packed, append = self._rustmap.write_v2(now, can_append) | |||||
| if append: | |||||
| docket = self.docket | |||||
| with self._opener(docket.data_filename(), b'ab') as fp: | |||||
marmoute: I don't think we should use 'a' here:
1) it is buggy on some version of python // os and still… | |||||
I inlined append from vfs.py in order to be able to add the assert with .tell(). Is vfs.py also buggy? Silently overwriting extra data seems very wrong to me. If someone else wrote it, overwriting it would interfere with them. However due to the combination of repofilecache + wlock, my understanding is that there should never be extra data. SimonSapin: I inlined `append` from `vfs.py` in order to be able to add the assert with `.tell()`. Is `vfs. | |||||
I am refering to the issue pointed here: https://www.mercurial-scm.org/repo/hg/file/stable/mercurial/revlog.py#l2427 So vfs.append is possibly buggy :-/ There should never be extra data, except if we decide this is a good idea in some case to being friendlier to mmap user. For example, we might decide to do so in hg rollback or decide that truncate less transaction rollback is useful. marmoute: I am refering to the issue pointed here: https://www.mercurial-scm. | |||||
| assert fp.tell() == docket.data_size, ( | |||||
| fp.tell(), | |||||
| docket.data_size, | |||||
| ) | |||||
| written = fp.write(packed) | |||||
| if written is not None: # py2 return None | |||||
| assert written == len(packed), (written, len(packed)) | |||||
| docket.data_size += len(packed) | |||||
| docket.parents = self.parents() | |||||
| st.write(docket.serialize()) | |||||
| st.close() | |||||
| else: | |||||
| old_docket = self.docket | old_docket = self.docket | ||||
| new_docket = docketmod.DirstateDocket.with_new_uuid( | new_docket = docketmod.DirstateDocket.with_new_uuid( | ||||
| self.parents(), len(packed) | self.parents(), len(packed) | ||||
| ) | ) | ||||
| self._opener.write(new_docket.data_filename(), packed) | self._opener.write(new_docket.data_filename(), packed) | ||||
| # Write the new docket after the new data file has been | # Write the new docket after the new data file has been | ||||
| # written. Because `st` was opened with `atomictemp=True`, | # written. Because `st` was opened with `atomictemp=True`, | ||||
| # the actual `.hg/dirstate` file is only affected on close. | # the actual `.hg/dirstate` file is only affected on close. | ||||
| st.write(new_docket.serialize()) | st.write(new_docket.serialize()) | ||||
| st.close() | st.close() | ||||
| # Remove the old data file after the new docket pointing to | # Remove the old data file after the new docket pointing to | ||||
| # the new data file was written. | # the new data file was written. | ||||
| if old_docket.uuid: | if old_docket.uuid: | ||||
| self._opener.unlink(old_docket.data_filename()) | self._opener.unlink(old_docket.data_filename()) | ||||
| self._docket = new_docket | self._docket = new_docket | ||||
| else: | # Reload from the newly-written file | ||||
| p1, p2 = self.parents() | util.clearcachedproperty(self, b"_rustmap") | ||||
| packed = self._rustmap.write_v1(p1, p2, now) | |||||
| st.write(packed) | |||||
| st.close() | |||||
| self._dirtyparents = False | self._dirtyparents = False | ||||
| @propertycache | @propertycache | ||||
| def filefoldmap(self): | def filefoldmap(self): | ||||
| """Returns a dictionary mapping normalized case paths to their | """Returns a dictionary mapping normalized case paths to their | ||||
| non-normalized versions. | non-normalized versions. | ||||
| """ | """ | ||||
| return self._rustmap.filefoldmapasdict() | return self._rustmap.filefoldmapasdict() | ||||
| Ok(()) | Ok(()) | ||||
| }, | }, | ||||
| )?; | )?; | ||||
| let parents = Some(parents.clone()); | let parents = Some(parents.clone()); | ||||
| Ok((map, parents)) | Ok((map, parents)) | ||||
| } | } | ||||
| /// Assuming dirstate-v2 format, returns whether the next write should | |||||
| /// append to the existing data file that contains `self.on_disk` (true), | |||||
| /// or create a new data file from scratch (false). | |||||
| pub(super) fn write_should_append(&self) -> bool { | |||||
| // Soon this will be a heuristic based on the amount of unreachable | |||||
| // data. For now it’s pseudo-random in order to make tests exercise | |||||
| // both code paths. | |||||
| fn bad_rng() -> u32 { | |||||
| std::time::SystemTime::now() | |||||
| .duration_since(std::time::UNIX_EPOCH) | |||||
| .unwrap() | |||||
| .subsec_millis() | |||||
| } | |||||
| bad_rng() % 2 == 0 | |||||
| } | |||||
| fn get_node<'tree>( | fn get_node<'tree>( | ||||
| &'tree self, | &'tree self, | ||||
| path: &HgPath, | path: &HgPath, | ||||
| ) -> Result<Option<NodeRef<'tree, 'on_disk>>, DirstateV2ParseError> { | ) -> Result<Option<NodeRef<'tree, 'on_disk>>, DirstateV2ParseError> { | ||||
| let mut children = self.root.as_ref(); | let mut children = self.root.as_ref(); | ||||
| let mut components = path.components(); | let mut components = path.components(); | ||||
| let mut component = | let mut component = | ||||
| components.next().expect("expected at least one components"); | components.next().expect("expected at least one components"); | ||||
| node.copy_source(self.on_disk)?, | node.copy_source(self.on_disk)?, | ||||
| &mut packed, | &mut packed, | ||||
| ); | ); | ||||
| } | } | ||||
| } | } | ||||
| Ok(packed) | Ok(packed) | ||||
| } | } | ||||
| /// Returns new data together with whether that data should be appended to | |||||
| /// the existing data file whose content is at `self.on_disk` (true), | |||||
| /// instead of written to a new data file (false). | |||||
| #[timed] | #[timed] | ||||
| fn pack_v2(&mut self, now: Timestamp) -> Result<Vec<u8>, DirstateError> { | fn pack_v2( | ||||
| &mut self, | |||||
| now: Timestamp, | |||||
| can_append: bool, | |||||
| ) -> Result<(Vec<u8>, bool), DirstateError> { | |||||
| // TODO:!how do we want to handle this in 2038? | // TODO:!how do we want to handle this in 2038? | ||||
| let now: i32 = now.0.try_into().expect("time overflow"); | let now: i32 = now.0.try_into().expect("time overflow"); | ||||
| let mut paths = Vec::new(); | let mut paths = Vec::new(); | ||||
| for node in self.iter_nodes() { | for node in self.iter_nodes() { | ||||
| let node = node?; | let node = node?; | ||||
| if let Some(entry) = node.entry()? { | if let Some(entry) = node.entry()? { | ||||
| if entry.mtime_is_ambiguous(now) { | if entry.mtime_is_ambiguous(now) { | ||||
| paths.push( | paths.push( | ||||
| node.full_path_borrowed(self.on_disk)? | node.full_path_borrowed(self.on_disk)? | ||||
| .detach_from_tree(), | .detach_from_tree(), | ||||
| ) | ) | ||||
| } | } | ||||
| } | } | ||||
| } | } | ||||
| // Borrow of `self` ends here since we collect cloned paths | // Borrow of `self` ends here since we collect cloned paths | ||||
| self.clear_known_ambiguous_mtimes(&paths)?; | self.clear_known_ambiguous_mtimes(&paths)?; | ||||
| on_disk::write(self) | on_disk::write(self, can_append) | ||||
| } | } | ||||
| fn status<'a>( | fn status<'a>( | ||||
| &'a mut self, | &'a mut self, | ||||
| matcher: &'a (dyn Matcher + Sync), | matcher: &'a (dyn Matcher + Sync), | ||||
| root_dir: PathBuf, | root_dir: PathBuf, | ||||
| ignore_files: Vec<PathBuf>, | ignore_files: Vec<PathBuf>, | ||||
| options: StatusOptions, | options: StatusOptions, | ||||
| fn pack_v1( | fn pack_v1( | ||||
| &mut self, | &mut self, | ||||
| parents: DirstateParents, | parents: DirstateParents, | ||||
| now: Timestamp, | now: Timestamp, | ||||
| ) -> Result<Vec<u8>, DirstateError>; | ) -> Result<Vec<u8>, DirstateError>; | ||||
| /// Clear mtimes that are ambigous with `now` (similar to | /// Clear mtimes that are ambigous with `now` (similar to | ||||
| /// `clear_ambiguous_times` but for all files in the dirstate map), and | /// `clear_ambiguous_times` but for all files in the dirstate map), and | ||||
| /// serialize bytes to write the `.hg/dirstate` file to disk in dirstate-v2 | /// serialize bytes to write a dirstate data file to disk in dirstate-v2 | ||||
| /// format. | /// format. | ||||
| /// | /// | ||||
| /// Returns new data together with whether that data should be appended to | |||||
| /// the existing data file whose content is at `self.on_disk` (true), | |||||
| /// instead of written to a new data file (false). | |||||
| /// | |||||
| /// Note: this is only supported by the tree dirstate map. | /// Note: this is only supported by the tree dirstate map. | ||||
| fn pack_v2(&mut self, now: Timestamp) -> Result<Vec<u8>, DirstateError>; | fn pack_v2( | ||||
| &mut self, | |||||
| now: Timestamp, | |||||
| can_append: bool, | |||||
| ) -> Result<(Vec<u8>, bool), DirstateError>; | |||||
| /// Run the status algorithm. | /// Run the status algorithm. | ||||
| /// | /// | ||||
| /// This is not sematically a method of the dirstate map, but a different | /// This is not sematically a method of the dirstate map, but a different | ||||
| /// algorithm is used for the flat v.s. tree dirstate map so having it in | /// algorithm is used for the flat v.s. tree dirstate map so having it in | ||||
| /// this trait enables the same dynamic dispatch as with other methods. | /// this trait enables the same dynamic dispatch as with other methods. | ||||
| fn status<'a>( | fn status<'a>( | ||||
| &'a mut self, | &'a mut self, | ||||
| fn pack_v1( | fn pack_v1( | ||||
| &mut self, | &mut self, | ||||
| parents: DirstateParents, | parents: DirstateParents, | ||||
| now: Timestamp, | now: Timestamp, | ||||
| ) -> Result<Vec<u8>, DirstateError> { | ) -> Result<Vec<u8>, DirstateError> { | ||||
| self.pack(parents, now) | self.pack(parents, now) | ||||
| } | } | ||||
| fn pack_v2(&mut self, _now: Timestamp) -> Result<Vec<u8>, DirstateError> { | fn pack_v2( | ||||
| &mut self, | |||||
| _now: Timestamp, | |||||
| _can_append: bool, | |||||
| ) -> Result<(Vec<u8>, bool), DirstateError> { | |||||
| panic!( | panic!( | ||||
| "should have used dirstate_tree::DirstateMap to use the v2 format" | "should have used dirstate_tree::DirstateMap to use the v2 format" | ||||
| ) | ) | ||||
| } | } | ||||
| fn status<'a>( | fn status<'a>( | ||||
| &'a mut self, | &'a mut self, | ||||
| matcher: &'a (dyn Matcher + Sync), | matcher: &'a (dyn Matcher + Sync), | ||||
| Ok(packed) => Ok(PyBytes::new(py, &packed)), | Ok(packed) => Ok(PyBytes::new(py, &packed)), | ||||
| Err(_) => Err(PyErr::new::<exc::OSError, _>( | Err(_) => Err(PyErr::new::<exc::OSError, _>( | ||||
| py, | py, | ||||
| "Dirstate error".to_string(), | "Dirstate error".to_string(), | ||||
| )), | )), | ||||
| } | } | ||||
| } | } | ||||
| /// Returns new data together with whether that data should be appended to | |||||
| /// the existing data file whose content is at `self.on_disk` (True), | |||||
| /// instead of written to a new data file (False). | |||||
| def write_v2( | def write_v2( | ||||
| &self, | &self, | ||||
| now: PyObject | now: PyObject, | ||||
| ) -> PyResult<PyBytes> { | can_append: bool, | ||||
| ) -> PyResult<PyObject> { | |||||
| let now = Timestamp(now.extract(py)?); | let now = Timestamp(now.extract(py)?); | ||||
| let mut inner = self.inner(py).borrow_mut(); | let mut inner = self.inner(py).borrow_mut(); | ||||
| let result = inner.pack_v2(now); | let result = inner.pack_v2(now, can_append); | ||||
| match result { | match result { | ||||
| Ok(packed) => Ok(PyBytes::new(py, &packed)), | Ok((packed, append)) => { | ||||
| let packed = PyBytes::new(py, &packed); | |||||
| Ok((packed, append).to_py_object(py).into_object()) | |||||
| }, | |||||
| Err(_) => Err(PyErr::new::<exc::OSError, _>( | Err(_) => Err(PyErr::new::<exc::OSError, _>( | ||||
| py, | py, | ||||
| "Dirstate error".to_string(), | "Dirstate error".to_string(), | ||||
| )), | )), | ||||
| } | } | ||||
| } | } | ||||
| def filefoldmapasdict(&self) -> PyResult<PyDict> { | def filefoldmapasdict(&self) -> PyResult<PyDict> { | ||||
| fn pack_v1( | fn pack_v1( | ||||
| &mut self, | &mut self, | ||||
| parents: DirstateParents, | parents: DirstateParents, | ||||
| now: Timestamp, | now: Timestamp, | ||||
| ) -> Result<Vec<u8>, DirstateError> { | ) -> Result<Vec<u8>, DirstateError> { | ||||
| self.get_mut().pack_v1(parents, now) | self.get_mut().pack_v1(parents, now) | ||||
| } | } | ||||
| fn pack_v2(&mut self, now: Timestamp) -> Result<Vec<u8>, DirstateError> { | fn pack_v2( | ||||
| self.get_mut().pack_v2(now) | &mut self, | ||||
| now: Timestamp, | |||||
| can_append: bool, | |||||
| ) -> Result<(Vec<u8>, bool), DirstateError> { | |||||
| self.get_mut().pack_v2(now, can_append) | |||||
| } | } | ||||
| fn status<'a>( | fn status<'a>( | ||||
| &'a mut self, | &'a mut self, | ||||
| matcher: &'a (dyn Matcher + Sync), | matcher: &'a (dyn Matcher + Sync), | ||||
| root_dir: PathBuf, | root_dir: PathBuf, | ||||
| ignore_files: Vec<PathBuf>, | ignore_files: Vec<PathBuf>, | ||||
| options: StatusOptions, | options: StatusOptions, | ||||
I don't think we should use 'a' here: