# HG changeset patch # User Pierre-Yves David # Date 2021-11-19 02:04:42 # Node ID 434de12918fd5256634896869d53b604a0a59035 # Parent b80e5e75d51e3a81731d254826cc02beb9b6ec5d dirstate: remove need_delay logic Now that all¹ stored mtime are non ambiguous, we no longer need to apply the `need_delay` step. The need delay logic was not great are mtime gathered during longer operation could be ambiguous but younger than the `dirstate.write` call time. So, we don't need that logic anymore and can drop it This make the code much simpler. The code related to the test extension faking the dirstate write is now obsolete and associated test will be migrated as follow up. They currently do not break. [1] except the ones from `hg update`, but `need_delay` no longer help for them either. Differential Revision: https://phab.mercurial-scm.org/D11796 diff --git a/mercurial/cext/parsers.c b/mercurial/cext/parsers.c --- a/mercurial/cext/parsers.c +++ b/mercurial/cext/parsers.c @@ -320,21 +320,6 @@ static PyObject *dirstate_item_v1_mtime( return PyInt_FromLong(dirstate_item_c_v1_mtime(self)); }; -static PyObject *dirstate_item_need_delay(dirstateItemObject *self, - PyObject *now) -{ - int now_s; - int now_ns; - if (!PyArg_ParseTuple(now, "ii", &now_s, &now_ns)) { - return NULL; - } - if (dirstate_item_c_v1_state(self) == 'n' && self->mtime_s == now_s) { - Py_RETURN_TRUE; - } else { - Py_RETURN_FALSE; - } -}; - static PyObject *dirstate_item_mtime_likely_equal_to(dirstateItemObject *self, PyObject *other) { @@ -548,8 +533,6 @@ static PyMethodDef dirstate_item_methods "return a \"size\" suitable for v1 serialization"}, {"v1_mtime", (PyCFunction)dirstate_item_v1_mtime, METH_NOARGS, "return a \"mtime\" suitable for v1 serialization"}, - {"need_delay", (PyCFunction)dirstate_item_need_delay, METH_O, - "True if the stored mtime would be ambiguous with the current time"}, {"mtime_likely_equal_to", (PyCFunction)dirstate_item_mtime_likely_equal_to, METH_O, "True if the stored mtime is likely equal to the given mtime"}, {"from_v1_data", (PyCFunction)dirstate_item_from_v1_meth, @@ -922,12 +905,9 @@ static PyObject *pack_dirstate(PyObject Py_ssize_t nbytes, pos, l; PyObject *k, *v = NULL, *pn; char *p, *s; - int now_s; - int now_ns; - if (!PyArg_ParseTuple(args, "O!O!O!(ii):pack_dirstate", &PyDict_Type, - &map, &PyDict_Type, ©map, &PyTuple_Type, &pl, - &now_s, &now_ns)) { + if (!PyArg_ParseTuple(args, "O!O!O!:pack_dirstate", &PyDict_Type, &map, + &PyDict_Type, ©map, &PyTuple_Type, &pl)) { return NULL; } @@ -996,21 +976,6 @@ static PyObject *pack_dirstate(PyObject mode = dirstate_item_c_v1_mode(tuple); size = dirstate_item_c_v1_size(tuple); mtime = dirstate_item_c_v1_mtime(tuple); - if (state == 'n' && tuple->mtime_s == now_s) { - /* See pure/parsers.py:pack_dirstate for why we do - * this. */ - mtime = -1; - mtime_unset = (PyObject *)dirstate_item_from_v1_data( - state, mode, size, mtime); - if (!mtime_unset) { - goto bail; - } - if (PyDict_SetItem(map, k, mtime_unset) == -1) { - goto bail; - } - Py_DECREF(mtime_unset); - mtime_unset = NULL; - } *p++ = state; putbe32((uint32_t)mode, p); putbe32((uint32_t)size, p + 4); diff --git a/mercurial/dirstate.py b/mercurial/dirstate.py --- a/mercurial/dirstate.py +++ b/mercurial/dirstate.py @@ -779,25 +779,6 @@ class dirstate(object): # filesystem's notion of 'now' now = timestamp.mtime_of(util.fstat(st)) - # enough 'delaywrite' prevents 'pack_dirstate' from dropping - # timestamp of each entries in dirstate, because of 'now > mtime' - delaywrite = self._ui.configint(b'debug', b'dirstate.delaywrite') - if delaywrite > 0: - # do we have any files to delay for? - for f, e in pycompat.iteritems(self._map): - if e.need_delay(now): - import time # to avoid useless import - - # rather than sleep n seconds, sleep until the next - # multiple of n seconds - clock = time.time() - start = int(clock) - (int(clock) % delaywrite) - end = start + delaywrite - time.sleep(end - clock) - # trust our estimate that the end is near now - now = timestamp.timestamp((end, 0)) - break - self._map.write(tr, st, now) self._dirty = False diff --git a/mercurial/dirstatemap.py b/mercurial/dirstatemap.py --- a/mercurial/dirstatemap.py +++ b/mercurial/dirstatemap.py @@ -446,11 +446,11 @@ class dirstatemap(_dirstatemapcommon): def write(self, tr, st, now): if self._use_dirstate_v2: - packed, meta = v2.pack_dirstate(self._map, self.copymap, now) + packed, meta = v2.pack_dirstate(self._map, self.copymap) self.write_v2_no_append(tr, st, meta, packed) else: packed = parsers.pack_dirstate( - self._map, self.copymap, self.parents(), now + self._map, self.copymap, self.parents() ) st.write(packed) st.close() @@ -658,7 +658,7 @@ if rustmod is not None: def write(self, tr, st, now): if not self._use_dirstate_v2: p1, p2 = self.parents() - packed = self._map.write_v1(p1, p2, now) + packed = self._map.write_v1(p1, p2) st.write(packed) st.close() self._dirtyparents = False @@ -666,7 +666,7 @@ if rustmod is not None: # We can only append to an existing data file if there is one can_append = self.docket.uuid is not None - packed, meta, append = self._map.write_v2(now, can_append) + packed, meta, append = self._map.write_v2(can_append) if append: docket = self.docket data_filename = docket.data_filename() diff --git a/mercurial/dirstateutils/v2.py b/mercurial/dirstateutils/v2.py --- a/mercurial/dirstateutils/v2.py +++ b/mercurial/dirstateutils/v2.py @@ -174,12 +174,10 @@ class Node(object): ) -def pack_dirstate(map, copy_map, now): +def pack_dirstate(map, copy_map): """ Pack `map` and `copy_map` into the dirstate v2 binary format and return the bytearray. - `now` is a timestamp of the current filesystem time used to detect race - conditions in writing the dirstate to disk, see inline comment. The on-disk format expects a tree-like structure where the leaves are written first (and sorted per-directory), going up levels until the root @@ -284,17 +282,6 @@ def pack_dirstate(map, copy_map, now): stack.append(current_node) for index, (path, entry) in enumerate(sorted_map, 1): - if entry.need_delay(now): - # The file was last modified "simultaneously" with the current - # write to dirstate (i.e. within the same second for file- - # systems with a granularity of 1 sec). This commonly happens - # for at least a couple of files on 'update'. - # The user could change the file without changing its size - # within the same second. Invalidate the file's mtime in - # dirstate, forcing future 'status' calls to compare the - # contents of the file if the size is the same. This prevents - # mistakenly treating such files as clean. - entry.set_possibly_dirty() nodes_with_entry_count += 1 if path in copy_map: nodes_with_copy_source_count += 1 diff --git a/mercurial/pure/parsers.py b/mercurial/pure/parsers.py --- a/mercurial/pure/parsers.py +++ b/mercurial/pure/parsers.py @@ -536,10 +536,6 @@ class DirstateItem(object): else: return self._mtime_s - def need_delay(self, now): - """True if the stored mtime would be ambiguous with the current time""" - return self.v1_state() == b'n' and self._mtime_s == now[0] - def gettype(q): return int(q & 0xFFFF) @@ -905,23 +901,11 @@ def parse_dirstate(dmap, copymap, st): return parents -def pack_dirstate(dmap, copymap, pl, now): +def pack_dirstate(dmap, copymap, pl): cs = stringio() write = cs.write write(b"".join(pl)) for f, e in pycompat.iteritems(dmap): - if e.need_delay(now): - # The file was last modified "simultaneously" with the current - # write to dirstate (i.e. within the same second for file- - # systems with a granularity of 1 sec). This commonly happens - # for at least a couple of files on 'update'. - # The user could change the file without changing its size - # within the same second. Invalidate the file's mtime in - # dirstate, forcing future 'status' calls to compare the - # contents of the file if the size is the same. This prevents - # mistakenly treating such files as clean. - e.set_possibly_dirty() - if f in copymap: f = b"%s\0%s" % (f, copymap[f]) e = _pack( diff --git a/rust/hg-core/src/dirstate/entry.rs b/rust/hg-core/src/dirstate/entry.rs --- a/rust/hg-core/src/dirstate/entry.rs +++ b/rust/hg-core/src/dirstate/entry.rs @@ -590,16 +590,6 @@ impl DirstateEntry { pub fn debug_tuple(&self) -> (u8, i32, i32, i32) { (self.state().into(), self.mode(), self.size(), self.mtime()) } - - /// True if the stored mtime would be ambiguous with the current time - pub fn need_delay(&self, now: TruncatedTimestamp) -> bool { - if let Some(mtime) = self.mtime { - self.state() == EntryState::Normal - && mtime.truncated_seconds() == now.truncated_seconds() - } else { - false - } - } } impl EntryState { diff --git a/rust/hg-core/src/dirstate_tree/dirstate_map.rs b/rust/hg-core/src/dirstate_tree/dirstate_map.rs --- a/rust/hg-core/src/dirstate_tree/dirstate_map.rs +++ b/rust/hg-core/src/dirstate_tree/dirstate_map.rs @@ -677,25 +677,6 @@ impl<'on_disk> DirstateMap<'on_disk> { }) } - fn clear_known_ambiguous_mtimes( - &mut self, - paths: &[impl AsRef], - ) -> Result<(), DirstateV2ParseError> { - for path in paths { - if let Some(node) = Self::get_node_mut( - self.on_disk, - &mut self.unreachable_bytes, - &mut self.root, - path.as_ref(), - )? { - if let NodeData::Entry(entry) = &mut node.data { - entry.set_possibly_dirty(); - } - } - } - Ok(()) - } - fn count_dropped_path(unreachable_bytes: &mut u32, path: &Cow) { if let Cow::Borrowed(path) = path { *unreachable_bytes += path.len() as u32 @@ -930,29 +911,20 @@ impl OwningDirstateMap { pub fn pack_v1( &mut self, parents: DirstateParents, - now: TruncatedTimestamp, ) -> Result, DirstateError> { let map = self.get_map_mut(); - let mut ambiguous_mtimes = Vec::new(); // Optizimation (to be measured?): pre-compute size to avoid `Vec` // reallocations let mut size = parents.as_bytes().len(); for node in map.iter_nodes() { let node = node?; - if let Some(entry) = node.entry()? { + if node.entry()?.is_some() { size += packed_entry_size( node.full_path(map.on_disk)?, node.copy_source(map.on_disk)?, ); - if entry.need_delay(now) { - ambiguous_mtimes.push( - node.full_path_borrowed(map.on_disk)? - .detach_from_tree(), - ) - } } } - map.clear_known_ambiguous_mtimes(&ambiguous_mtimes)?; let mut packed = Vec::with_capacity(size); packed.extend(parents.as_bytes()); @@ -978,26 +950,9 @@ impl OwningDirstateMap { #[timed] pub fn pack_v2( &mut self, - now: TruncatedTimestamp, can_append: bool, ) -> Result<(Vec, Vec, bool), DirstateError> { let map = self.get_map_mut(); - let mut paths = Vec::new(); - for node in map.iter_nodes() { - let node = node?; - if let Some(entry) = node.entry()? { - if entry.need_delay(now) { - paths.push( - node.full_path_borrowed(map.on_disk)? - .detach_from_tree(), - ) - } - } - } - // Borrow of `self` ends here since we collect cloned paths - - map.clear_known_ambiguous_mtimes(&paths)?; - on_disk::write(map, can_append) } diff --git a/rust/hg-cpython/src/dirstate/dirstate_map.rs b/rust/hg-cpython/src/dirstate/dirstate_map.rs --- a/rust/hg-cpython/src/dirstate/dirstate_map.rs +++ b/rust/hg-cpython/src/dirstate/dirstate_map.rs @@ -18,7 +18,7 @@ use cpython::{ use crate::{ dirstate::copymap::{CopyMap, CopyMapItemsIterator, CopyMapKeysIterator}, - dirstate::item::{timestamp, DirstateItem}, + dirstate::item::DirstateItem, pybytes_deref::PyBytesDeref, }; use hg::{ @@ -194,16 +194,13 @@ py_class!(pub class DirstateMap |py| { &self, p1: PyObject, p2: PyObject, - now: (u32, u32) ) -> PyResult { - let now = timestamp(py, now)?; - let mut inner = self.inner(py).borrow_mut(); let parents = DirstateParents { p1: extract_node_id(py, &p1)?, p2: extract_node_id(py, &p2)?, }; - let result = inner.pack_v1(parents, now); + let result = inner.pack_v1(parents); match result { Ok(packed) => Ok(PyBytes::new(py, &packed)), Err(_) => Err(PyErr::new::( @@ -218,13 +215,10 @@ py_class!(pub class DirstateMap |py| { /// instead of written to a new data file (False). def write_v2( &self, - now: (u32, u32), can_append: bool, ) -> PyResult { - let now = timestamp(py, now)?; - let mut inner = self.inner(py).borrow_mut(); - let result = inner.pack_v2(now, can_append); + let result = inner.pack_v2(can_append); match result { Ok((packed, tree_metadata, append)) => { let packed = PyBytes::new(py, &packed); diff --git a/rust/hg-cpython/src/dirstate/item.rs b/rust/hg-cpython/src/dirstate/item.rs --- a/rust/hg-cpython/src/dirstate/item.rs +++ b/rust/hg-cpython/src/dirstate/item.rs @@ -194,11 +194,6 @@ py_class!(pub class DirstateItem |py| { Ok(mtime) } - def need_delay(&self, now: (u32, u32)) -> PyResult { - let now = timestamp(py, now)?; - Ok(self.entry(py).get().need_delay(now)) - } - def mtime_likely_equal_to(&self, other: (u32, u32)) -> PyResult { if let Some(mtime) = self.entry(py).get().truncated_mtime() { Ok(mtime.likely_equal(timestamp(py, other)?)) diff --git a/tests/fakedirstatewritetime.py b/tests/fakedirstatewritetime.py --- a/tests/fakedirstatewritetime.py +++ b/tests/fakedirstatewritetime.py @@ -37,14 +37,8 @@ parsers = policy.importmod('parsers') has_rust_dirstate = policy.importrust('dirstate') is not None -def pack_dirstate(fakenow, orig, dmap, copymap, pl, now): - # execute what original parsers.pack_dirstate should do actually - # for consistency - for f, e in dmap.items(): - if e.need_delay(now): - e.set_possibly_dirty() - - return orig(dmap, copymap, pl, fakenow) +def pack_dirstate(orig, dmap, copymap, pl): + return orig(dmap, copymap, pl) def fakewrite(ui, func): @@ -67,19 +61,19 @@ def fakewrite(ui, func): # The Rust implementation does not use public parse/pack dirstate # to prevent conversion round-trips orig_dirstatemap_write = dirstatemapmod.dirstatemap.write - wrapper = lambda self, tr, st, now: orig_dirstatemap_write( - self, tr, st, fakenow - ) + wrapper = lambda self, tr, st: orig_dirstatemap_write(self, tr, st) dirstatemapmod.dirstatemap.write = wrapper orig_get_fs_now = timestamp.get_fs_now - wrapper = lambda *args: pack_dirstate(fakenow, orig_pack_dirstate, *args) + wrapper = lambda *args: pack_dirstate(orig_pack_dirstate, *args) orig_module = parsers orig_pack_dirstate = parsers.pack_dirstate orig_module.pack_dirstate = wrapper - timestamp.get_fs_now = lambda *args: fakenow + timestamp.get_fs_now = ( + lambda *args: fakenow + ) # XXX useless for this purpose now try: return func() finally: diff --git a/tests/test-merge1.t b/tests/test-merge1.t --- a/tests/test-merge1.t +++ b/tests/test-merge1.t @@ -349,6 +349,10 @@ Test that updated files are treated as " aren't changed), even if none of mode, size and timestamp of them isn't changed on the filesystem (see also issue4583). +This test is now "best effort" as the mechanism to prevent such race are +getting better, it get more complicated to test a specific scenario that would +trigger it. If you see flakyness here, there is a race. + $ cat > $TESTTMP/abort.py < from __future__ import absolute_import > # emulate aborting before "recordupdates()". in this case, files @@ -365,13 +369,6 @@ isn't changed on the filesystem (see als > extensions.wrapfunction(merge, "applyupdates", applyupdates) > EOF - $ cat >> .hg/hgrc < [fakedirstatewritetime] - > # emulate invoking dirstate.write() via repo.status() - > # at 2000-01-01 00:00 - > fakenow = 200001010000 - > EOF - (file gotten from other revision) $ hg update -q -C 2 @@ -381,12 +378,8 @@ isn't changed on the filesystem (see als $ hg update -q -C 3 $ cat b This is file b1 - $ touch -t 200001010000 b - $ hg debugrebuildstate - $ cat >> .hg/hgrc < [extensions] - > fakedirstatewritetime = $TESTDIR/fakedirstatewritetime.py > abort = $TESTTMP/abort.py > EOF $ hg merge 5 @@ -394,13 +387,11 @@ isn't changed on the filesystem (see als [255] $ cat >> .hg/hgrc < [extensions] - > fakedirstatewritetime = ! > abort = ! > EOF $ cat b THIS IS FILE B5 - $ touch -t 200001010000 b $ hg status -A b M b @@ -413,12 +404,10 @@ isn't changed on the filesystem (see als $ cat b this is file b6 - $ touch -t 200001010000 b - $ hg debugrebuildstate + $ hg status $ cat >> .hg/hgrc < [extensions] - > fakedirstatewritetime = $TESTDIR/fakedirstatewritetime.py > abort = $TESTTMP/abort.py > EOF $ hg merge --tool internal:other 5 @@ -426,13 +415,11 @@ isn't changed on the filesystem (see als [255] $ cat >> .hg/hgrc < [extensions] - > fakedirstatewritetime = ! > abort = ! > EOF $ cat b THIS IS FILE B5 - $ touch -t 200001010000 b $ hg status -A b M b diff --git a/tests/test-subrepo.t b/tests/test-subrepo.t --- a/tests/test-subrepo.t +++ b/tests/test-subrepo.t @@ -1021,37 +1021,21 @@ Issue1977: multirepo push should fail if test if untracked file is not overwritten -(this also tests that updated .hgsubstate is treated as "modified", -when 'merge.update()' is aborted before 'merge.recordupdates()', even -if none of mode, size and timestamp of it isn't changed on the -filesystem (see also issue4583)) +(this tests also has a change to update .hgsubstate and merge it within the +same second. It should mark is are modified , even if none of mode, size and +timestamp of it isn't changed on the filesystem (see also issue4583)) $ echo issue3276_ok > repo/s/b $ hg -R repo2 push -f -q - $ touch -t 200001010000 repo/.hgsubstate - $ cat >> repo/.hg/hgrc < [fakedirstatewritetime] - > # emulate invoking dirstate.write() via repo.status() - > # at 2000-01-01 00:00 - > fakenow = 200001010000 - > - > [extensions] - > fakedirstatewritetime = $TESTDIR/fakedirstatewritetime.py - > EOF $ hg -R repo update b: untracked file differs abort: untracked files in working directory differ from files in requested revision (in subrepository "s") [255] - $ cat >> repo/.hg/hgrc < [extensions] - > fakedirstatewritetime = ! - > EOF $ cat repo/s/b issue3276_ok $ rm repo/s/b - $ touch -t 200001010000 repo/.hgsubstate $ hg -R repo revert --all reverting repo/.hgsubstate reverting subrepo s