This is an archive of the discontinued Mercurial Phabricator instance.

Differential D10972

dirstate: drop `state` to `_addpath`
ClosedPublic

Authored by marmoute on Jul 4 2021, 5:51 PM.

Details

Reviewers
SimonSapin
pulkit
Group Reviewers
hg-reviewers
Commits
rHGa1745a292885: dirstate: drop `state` to `_addpath`
Summary

All its value are inferred within the dirstatemap now.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

marmoute created this revision.Jul 4 2021, 5:51 PM
SimonSapin accepted this revision.Jul 5 2021, 1:08 AM
SimonSapin added a subscriber: SimonSapin.
SimonSapin added inline comments.
rust/hg-cpython/src/dirstate/dirstate_map.rs
140–142

This strongly suggests that DirstateMapMethods::add_file should no longer take a DirstateEntry parameter. Let’s refactor that later together with its boolean parameters.

marmoute added inline comments.Jul 5 2021, 4:43 AM
rust/hg-cpython/src/dirstate/dirstate_map.rs
140–142

Yep, we can build something better dirstatemap side and that should lead use to something simpler on the hg-core side too.

pulkit added a subscriber: pulkit.Jul 6 2021, 12:47 PM
pulkit added inline comments.
mercurial/dirstatemap.py
184

Hm, is this an intended change?

marmoute added inline comments.Jul 6 2021, 12:52 PM
mercurial/dirstatemap.py
184

Yes, since we dropped the argument we still need it defined in all code path. That code path is the the 'n' case.

rust/hg-core/src/dirstate/dirstate_map.rs
102

See the similar change in the rust code.

baymax updated this revision to Diff 28874.Jul 6 2021, 2:09 PM

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

pulkit accepted this revision.Jul 6 2021, 2:50 PM
This revision is now accepted and ready to land.Jul 6 2021, 2:50 PM
marmoute added a commit: rHGa1745a292885: dirstate: drop `state` to `_addpath`.Jul 6 2021, 3:12 PM
Closed by commit rHGa1745a292885: dirstate: drop `state` to `_addpath` (authored by marmoute). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 28911

mercurial/dirstate.py

return self._map.copymap.get(file, None) return self._map.copymap.get(file, None)
def copies(self): def copies(self):
return self._map.copymap return self._map.copymap
def _addpath( def _addpath(
self, self,
f, f,
state=None,
mode=0, mode=0,
size=None, size=None,
mtime=None, mtime=None,
added=False, added=False,
merged=False, merged=False,
from_p2=False, from_p2=False,
possibly_dirty=False, possibly_dirty=False,
): ):
if entry is not None and not entry.removed: if entry is not None and not entry.removed:
msg = _(b'file %r in dirstate clashes with %r') msg = _(b'file %r in dirstate clashes with %r')
msg %= (pycompat.bytestr(d), pycompat.bytestr(f)) msg %= (pycompat.bytestr(d), pycompat.bytestr(f))
raise error.Abort(msg) raise error.Abort(msg)
self._dirty = True self._dirty = True
self._updatedfiles.add(f) self._updatedfiles.add(f)
self._map.addfile( self._map.addfile(
f, f,
state=state,
mode=mode, mode=mode,
size=size, size=size,
mtime=mtime, mtime=mtime,
added=added, added=added,
merged=merged, merged=merged,
from_p2=from_p2, from_p2=from_p2,
possibly_dirty=possibly_dirty, possibly_dirty=possibly_dirty,
) )
moment where the file was determined to be clean and now.""" moment where the file was determined to be clean and now."""
if parentfiledata: if parentfiledata:
(mode, size, mtime) = parentfiledata (mode, size, mtime) = parentfiledata
else: else:
s = os.lstat(self._join(f)) s = os.lstat(self._join(f))
mode = s.st_mode mode = s.st_mode
size = s.st_size size = s.st_size
mtime = s[stat.ST_MTIME] mtime = s[stat.ST_MTIME]
self._addpath(f, b'n', mode, size, mtime) self._addpath(f, mode=mode, size=size, mtime=mtime)
self._map.copymap.pop(f, None) self._map.copymap.pop(f, None)
if f in self._map.nonnormalset: if f in self._map.nonnormalset:
self._map.nonnormalset.remove(f) self._map.nonnormalset.remove(f)
if mtime > self._lastnormaltime: if mtime > self._lastnormaltime:
# Remember the most recent modification timeslot for status(), # Remember the most recent modification timeslot for status(),
# to make sure we won't miss future size-preserving file content # to make sure we won't miss future size-preserving file content
# modifications that happen within the same timeslot. # modifications that happen within the same timeslot.
self._lastnormaltime = mtime self._lastnormaltime = mtime

mercurial/dirstatemap.py

def preload(self): def preload(self):
"""Loads the underlying data, if it's not already loaded""" """Loads the underlying data, if it's not already loaded"""
self._map self._map
def addfile( def addfile(
self, self,
f, f,
state=None,
mode=0, mode=0,
size=None, size=None,
mtime=None, mtime=None,
added=False, added=False,
merged=False, merged=False,
from_p2=False, from_p2=False,
possibly_dirty=False, possibly_dirty=False,
): ):
state = b'n' state = b'n'
size = FROM_P2 size = FROM_P2
mtime = AMBIGUOUS_TIME mtime = AMBIGUOUS_TIME
elif possibly_dirty: elif possibly_dirty:
state = b'n' state = b'n'
size = NONNORMAL size = NONNORMAL
mtime = AMBIGUOUS_TIME mtime = AMBIGUOUS_TIME
else: else:
assert state != b'a'
assert size != FROM_P2 assert size != FROM_P2
assert size != NONNORMAL assert size != NONNORMAL
state = b'n'
pulkitUnsubmitted
Not Done

Hm, is this an intended change?

pulkit: Hm, is this an intended change?
marmouteAuthorUnsubmitted
Done

Yes, since we dropped the argument we still need it defined in all code path. That code path is the the 'n' case.

marmoute: Yes, since we dropped the argument we still need it defined in all code path. That code path is…
size = size & rangemask size = size & rangemask
mtime = mtime & rangemask mtime = mtime & rangemask
assert state is not None assert state is not None
assert size is not None assert size is not None
assert mtime is not None assert mtime is not None
old_entry = self.get(f) old_entry = self.get(f)
if ( if (
old_entry is None or old_entry.removed old_entry is None or old_entry.removed
b"experimental", b"experimental",
b"dirstate-tree.in-memory", b"dirstate-tree.in-memory",
False, False,
) )
def addfile( def addfile(
self, self,
f, f,
state=None,
mode=0, mode=0,
size=None, size=None,
mtime=None, mtime=None,
added=False, added=False,
merged=False, merged=False,
from_p2=False, from_p2=False,
possibly_dirty=False, possibly_dirty=False,
): ):
return self._rustmap.addfile( return self._rustmap.addfile(
f, f,
state,
mode, mode,
size, size,
mtime, mtime,
added, added,
merged, merged,
from_p2, from_p2,
possibly_dirty, possibly_dirty,
) )

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

entry.state = EntryState::Normal; entry.state = EntryState::Normal;
entry.size = SIZE_FROM_OTHER_PARENT; entry.size = SIZE_FROM_OTHER_PARENT;
entry.mtime = MTIME_UNSET; entry.mtime = MTIME_UNSET;
} else if possibly_dirty { } else if possibly_dirty {
entry.state = EntryState::Normal; entry.state = EntryState::Normal;
entry.size = SIZE_NON_NORMAL; entry.size = SIZE_NON_NORMAL;
entry.mtime = MTIME_UNSET; entry.mtime = MTIME_UNSET;
} else { } else {
entry.state = EntryState::Normal;
marmouteAuthorUnsubmitted
Done

See the similar change in the rust code.

marmoute: See the similar change in the rust code.
entry.size = entry.size & V1_RANGEMASK; entry.size = entry.size & V1_RANGEMASK;
entry.mtime = entry.mtime & V1_RANGEMASK; entry.mtime = entry.mtime & V1_RANGEMASK;
} }
let old_state = match self.get(filename) { let old_state = match self.get(filename) {
Some(e) => e.state, Some(e) => e.state,
None => EntryState::Unknown, None => EntryState::Unknown,
}; };
if old_state == EntryState::Unknown || old_state == EntryState::Removed if old_state == EntryState::Unknown || old_state == EntryState::Removed

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

entry.state = EntryState::Normal; entry.state = EntryState::Normal;
entry.size = SIZE_FROM_OTHER_PARENT; entry.size = SIZE_FROM_OTHER_PARENT;
entry.mtime = MTIME_UNSET; entry.mtime = MTIME_UNSET;
} else if possibly_dirty { } else if possibly_dirty {
entry.state = EntryState::Normal; entry.state = EntryState::Normal;
entry.size = SIZE_NON_NORMAL; entry.size = SIZE_NON_NORMAL;
entry.mtime = MTIME_UNSET; entry.mtime = MTIME_UNSET;
} else { } else {
entry.state = EntryState::Normal;
entry.size = entry.size & V1_RANGEMASK; entry.size = entry.size & V1_RANGEMASK;
entry.mtime = entry.mtime & V1_RANGEMASK; entry.mtime = entry.mtime & V1_RANGEMASK;
} }
let old_state = match self.get(filename)? { let old_state = match self.get(filename)? {
Some(e) => e.state, Some(e) => e.state,
None => EntryState::Unknown, None => EntryState::Unknown,
}; };

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

}, },
None => Ok(default) None => Ok(default)
} }
} }
def addfile( def addfile(
&self, &self,
f: PyObject, f: PyObject,
state: PyObject,
mode: PyObject, mode: PyObject,
size: PyObject, size: PyObject,
mtime: PyObject, mtime: PyObject,
added: PyObject, added: PyObject,
merged: PyObject, merged: PyObject,
from_p2: PyObject, from_p2: PyObject,
possibly_dirty: PyObject, possibly_dirty: PyObject,
) -> PyResult<PyObject> { ) -> PyResult<PyObject> {
let f = f.extract::<PyBytes>(py)?; let f = f.extract::<PyBytes>(py)?;
let filename = HgPath::new(f.data(py)); let filename = HgPath::new(f.data(py));
let state = if state.is_none(py) {
// Arbitrary default value
EntryState::Normal
} else {
state.extract::<PyBytes>(py)?.data(py)[0]
.try_into()
.map_err(|e: HgError| {
PyErr::new::<exc::ValueError, _>(py, e.to_string())
})?
};
let mode = if mode.is_none(py) { let mode = if mode.is_none(py) {
// fallback default value // fallback default value
0 0
} else { } else {
mode.extract(py)? mode.extract(py)?
}; };
let size = if size.is_none(py) { let size = if size.is_none(py) {
// fallback default value // fallback default value
SIZE_NON_NORMAL SIZE_NON_NORMAL
} else { } else {
size.extract(py)? size.extract(py)?
}; };
let mtime = if mtime.is_none(py) { let mtime = if mtime.is_none(py) {
// fallback default value // fallback default value
MTIME_UNSET MTIME_UNSET
} else { } else {
mtime.extract(py)? mtime.extract(py)?
}; };
let entry = DirstateEntry { let entry = DirstateEntry {
state: state, // XXX Arbitrary default value since the value is determined later
state: EntryState::Normal,
mode: mode, mode: mode,
SimonSapinUnsubmitted
Not Done

This strongly suggests that DirstateMapMethods::add_file should no longer take a DirstateEntry parameter. Let’s refactor that later together with its boolean parameters.

SimonSapin: This strongly suggests that `DirstateMapMethods::add_file` should no longer take a…
marmouteAuthorUnsubmitted
Done

Yep, we can build something better dirstatemap side and that should lead use to something simpler on the hg-core side too.

marmoute: Yep, we can build something better dirstatemap side and that should lead use to something…
size: size, size: size,
mtime: mtime, mtime: mtime,
}; };
let added = added.extract::<PyBool>(py)?.is_true(); let added = added.extract::<PyBool>(py)?.is_true();
let merged = merged.extract::<PyBool>(py)?.is_true(); let merged = merged.extract::<PyBool>(py)?.is_true();
let from_p2 = from_p2.extract::<PyBool>(py)?.is_true(); let from_p2 = from_p2.extract::<PyBool>(py)?.is_true();
let possibly_dirty = possibly_dirty.extract::<PyBool>(py)?.is_true(); let possibly_dirty = possibly_dirty.extract::<PyBool>(py)?.is_true();
self.inner(py).borrow_mut().add_file( self.inner(py).borrow_mut().add_file(