( )⚙ D10969 dirstate: use a `merged` parameter to _addpath

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D10969

dirstate: use a `merged` parameter to _addpath
ClosedPublic

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

Details

Reviewers
SimonSapin
pulkit
Group Reviewers
hg-reviewers
Commits
rHGc6b91a9c242a: dirstate: use a `merged` parameter to _addpath

Diff Detail

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

Event Timeline

marmoute created this revision.Jul 4 2021, 5:51 PM
SimonSapin accepted this revision.Jul 5 2021, 1:01 AM
SimonSapin added a subscriber: SimonSapin.
SimonSapin added inline comments.
rust/hg-core/src/dirstate_tree/dispatch.rs
292–295

This can be a follow-up and you already have a comment elsewhere to that effect, but this is increasingly too many order-sensitive boolean parameters

marmoute added inline comments.Jul 5 2021, 4:42 AM
rust/hg-core/src/dirstate_tree/dispatch.rs
292–295

Once the "migration" is done, we should probably burn the whole dirstatemap.addfile API to the ground a rebuild something cleaner.

pulkit added a subscriber: pulkit.Jul 6 2021, 12:44 PM
pulkit added inline comments.
rust/hg-core/src/dirstate_tree/dispatch.rs
292–295

I agree with Simon here but yes a better API can wait for current refactoring to be finished.

pulkit accepted this revision.Jul 6 2021, 12:45 PM
This revision is now accepted and ready to land.Jul 6 2021, 12:45 PM
marmoute added inline comments.Jul 6 2021, 12:50 PM
rust/hg-core/src/dirstate_tree/dispatch.rs
292–295

I think everybody agree with Simon :-)

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

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

marmoute added a commit: rHGc6b91a9c242a: dirstate: use a `merged` parameter to _addpath.Jul 6 2021, 3:11 PM
Closed by commit rHGc6b91a9c242a: dirstate: use a `merged` parameter to _addpath (authored by marmoute). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 28790

mercurial/dirstate.py

def _addpath( def _addpath(
self, self,
f, f,
state=None, state=None,
mode=0, mode=0,
size=None, size=None,
mtime=None, mtime=None,
added=False, added=False,
merged=False,
from_p2=False, from_p2=False,
possibly_dirty=False, possibly_dirty=False,
): ):
entry = self._map.get(f) entry = self._map.get(f)
if added or entry is not None and entry.removed: if added or entry is not None and entry.removed:
scmutil.checkfilename(f) scmutil.checkfilename(f)
if self._map.hastrackeddir(f): if self._map.hastrackeddir(f):
msg = _(b'directory %r already in dirstate') msg = _(b'directory %r already in dirstate')
self._updatedfiles.add(f) self._updatedfiles.add(f)
self._map.addfile( self._map.addfile(
f, f,
state=state, state=state,
mode=mode, mode=mode,
size=size, size=size,
mtime=mtime, mtime=mtime,
added=added, added=added,
merged=merged,
from_p2=from_p2, from_p2=from_p2,
possibly_dirty=possibly_dirty, possibly_dirty=possibly_dirty,
) )
def normal(self, f, parentfiledata=None): def normal(self, f, parentfiledata=None):
"""Mark a file normal and clean. """Mark a file normal and clean.
parentfiledata: (mode, size, mtime) of the clean file parentfiledata: (mode, size, mtime) of the clean file
def otherparent(self, f): def otherparent(self, f):
'''Mark as coming from the other parent, always dirty.''' '''Mark as coming from the other parent, always dirty.'''
if not self.in_merge: if not self.in_merge:
msg = _(b"setting %r to other parent only allowed in merges") % f msg = _(b"setting %r to other parent only allowed in merges") % f
raise error.Abort(msg) raise error.Abort(msg)
if f in self and self[f] == b'n': if f in self and self[f] == b'n':
# merge-like # merge-like
self._addpath(f, b'm', 0, from_p2=True) self._addpath(f, merged=True)
else: else:
# add-like # add-like
self._addpath(f, b'n', 0, from_p2=True) self._addpath(f, b'n', 0, from_p2=True)
self._map.copymap.pop(f, None) self._map.copymap.pop(f, None)
def add(self, f): def add(self, f):
'''Mark a file added.''' '''Mark a file added.'''
self._addpath(f, added=True) self._addpath(f, added=True)

mercurial/dirstatemap.py

def addfile( def addfile(
self, self,
f, f,
state=None, state=None,
mode=0, mode=0,
size=None, size=None,
mtime=None, mtime=None,
added=False, added=False,
merged=False,
from_p2=False, from_p2=False,
possibly_dirty=False, possibly_dirty=False,
): ):
"""Add a tracked file to the dirstate.""" """Add a tracked file to the dirstate."""
if added: if added:
assert not merged
assert not possibly_dirty assert not possibly_dirty
assert not from_p2 assert not from_p2
state = b'a' state = b'a'
size = NONNORMAL size = NONNORMAL
mtime = AMBIGUOUS_TIME mtime = AMBIGUOUS_TIME
elif merged:
assert not possibly_dirty
assert not from_p2
state = b'm'
size = FROM_P2
mtime = AMBIGUOUS_TIME
elif from_p2: elif from_p2:
assert not possibly_dirty assert not possibly_dirty
size = FROM_P2 size = FROM_P2
mtime = AMBIGUOUS_TIME mtime = AMBIGUOUS_TIME
elif possibly_dirty: elif possibly_dirty:
size = NONNORMAL size = NONNORMAL
mtime = AMBIGUOUS_TIME mtime = AMBIGUOUS_TIME
else: else:
def addfile( def addfile(
self, self,
f, f,
state=None, state=None,
mode=0, mode=0,
size=None, size=None,
mtime=None, mtime=None,
added=False, added=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, state,
mode, mode,
size, size,
mtime, mtime,
added, added,
merged,
from_p2, from_p2,
possibly_dirty, possibly_dirty,
) )
def removefile(self, *args, **kwargs): def removefile(self, *args, **kwargs):
return self._rustmap.removefile(*args, **kwargs) return self._rustmap.removefile(*args, **kwargs)
def dropfile(self, *args, **kwargs): def dropfile(self, *args, **kwargs):

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

/// Add a tracked file to the dirstate /// Add a tracked file to the dirstate
pub fn add_file( pub fn add_file(
&mut self, &mut self,
filename: &HgPath, filename: &HgPath,
entry: DirstateEntry, entry: DirstateEntry,
// XXX once the dust settle this should probably become an enum // XXX once the dust settle this should probably become an enum
added: bool, added: bool,
merged: bool,
from_p2: bool, from_p2: bool,
possibly_dirty: bool, possibly_dirty: bool,
) -> Result<(), DirstateError> { ) -> Result<(), DirstateError> {
let mut entry = entry; let mut entry = entry;
if added { if added {
assert!(!merged);
assert!(!possibly_dirty); assert!(!possibly_dirty);
assert!(!from_p2); assert!(!from_p2);
entry.state = EntryState::Added; entry.state = EntryState::Added;
entry.size = SIZE_NON_NORMAL; entry.size = SIZE_NON_NORMAL;
entry.mtime = MTIME_UNSET; entry.mtime = MTIME_UNSET;
} else if merged {
assert!(!possibly_dirty);
assert!(!from_p2);
entry.state = EntryState::Merged;
entry.size = SIZE_FROM_OTHER_PARENT;
entry.mtime = MTIME_UNSET;
} else if from_p2 { } else if from_p2 {
assert!(!possibly_dirty); assert!(!possibly_dirty);
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.size = SIZE_NON_NORMAL; entry.size = SIZE_NON_NORMAL;
entry.mtime = MTIME_UNSET; entry.mtime = MTIME_UNSET;
} else { } else {
state: EntryState::Normal, state: EntryState::Normal,
mode: 1337, mode: 1337,
mtime: 1337, mtime: 1337,
size: 1337, size: 1337,
}, },
false, false,
false, false,
false, false,
false,
) )
.unwrap(); .unwrap();
assert_eq!(1, map.len()); assert_eq!(1, map.len());
assert_eq!(0, map.get_non_normal_other_parent_entries().0.len()); assert_eq!(0, map.get_non_normal_other_parent_entries().0.len());
assert_eq!(0, map.get_non_normal_other_parent_entries().1.len()); assert_eq!(0, map.get_non_normal_other_parent_entries().1.len());
} }

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

self.nodes_with_copy_source_count = 0; self.nodes_with_copy_source_count = 0;
} }
fn add_file( fn add_file(
&mut self, &mut self,
filename: &HgPath, filename: &HgPath,
entry: DirstateEntry, entry: DirstateEntry,
added: bool, added: bool,
merged: bool,
from_p2: bool, from_p2: bool,
possibly_dirty: bool, possibly_dirty: bool,
) -> Result<(), DirstateError> { ) -> Result<(), DirstateError> {
let mut entry = entry; let mut entry = entry;
if added { if added {
assert!(!possibly_dirty); assert!(!possibly_dirty);
assert!(!from_p2); assert!(!from_p2);
entry.state = EntryState::Added; entry.state = EntryState::Added;
entry.size = SIZE_NON_NORMAL; entry.size = SIZE_NON_NORMAL;
entry.mtime = MTIME_UNSET; entry.mtime = MTIME_UNSET;
} else if merged {
assert!(!possibly_dirty);
assert!(!from_p2);
entry.state = EntryState::Merged;
entry.size = SIZE_FROM_OTHER_PARENT;
entry.mtime = MTIME_UNSET;
} else if from_p2 { } else if from_p2 {
assert!(!possibly_dirty); assert!(!possibly_dirty);
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.size = SIZE_NON_NORMAL; entry.size = SIZE_NON_NORMAL;
entry.mtime = MTIME_UNSET; entry.mtime = MTIME_UNSET;
} else { } else {

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

/// before this call, or `EntryState::Unknown` if there was no such entry. /// before this call, or `EntryState::Unknown` if there was no such entry.
/// ///
/// `entry.state` should never be `EntryState::Unknown`. /// `entry.state` should never be `EntryState::Unknown`.
fn add_file( fn add_file(
&mut self, &mut self,
filename: &HgPath, filename: &HgPath,
entry: DirstateEntry, entry: DirstateEntry,
added: bool, added: bool,
merged: bool,
from_p2: bool, from_p2: bool,
possibly_dirty: bool, possibly_dirty: bool,
) -> Result<(), DirstateError>; ) -> Result<(), DirstateError>;
/// Mark a file as "removed" (as in `hg rm`). /// Mark a file as "removed" (as in `hg rm`).
/// ///
/// `old_state` is the state in the entry that `get` would have returned /// `old_state` is the state in the entry that `get` would have returned
/// before this call, or `EntryState::Unknown` if there was no such entry. /// before this call, or `EntryState::Unknown` if there was no such entry.
fn clear(&mut self) { fn clear(&mut self) {
self.clear() self.clear()
} }
fn add_file( fn add_file(
&mut self, &mut self,
filename: &HgPath, filename: &HgPath,
entry: DirstateEntry, entry: DirstateEntry,
added: bool, added: bool,
merged: bool,
from_p2: bool, from_p2: bool,
possibly_dirty: bool, possibly_dirty: bool,
SimonSapinUnsubmitted
Not Done

This can be a follow-up and you already have a comment elsewhere to that effect, but this is increasingly too many order-sensitive boolean parameters

SimonSapin: This can be a follow-up and you already have a comment elsewhere to that effect, but this is…
marmouteAuthorUnsubmitted
Done

Once the "migration" is done, we should probably burn the whole dirstatemap.addfile API to the ground a rebuild something cleaner.

marmoute: Once the "migration" is done, we should probably burn the whole `dirstatemap.addfile` API to…
pulkitUnsubmitted
Not Done

I agree with Simon here but yes a better API can wait for current refactoring to be finished.

pulkit: I agree with Simon here but yes a better API can wait for current refactoring to be finished.
marmouteAuthorUnsubmitted
Done

I think everybody agree with Simon :-)

marmoute: I think everybody agree with Simon :-)
) -> Result<(), DirstateError> { ) -> Result<(), DirstateError> {
self.add_file(filename, entry, added, from_p2, possibly_dirty) self.add_file(filename, entry, added, merged, from_p2, possibly_dirty)
} }
fn remove_file( fn remove_file(
&mut self, &mut self,
filename: &HgPath, filename: &HgPath,
in_merge: bool, in_merge: bool,
) -> Result<(), DirstateError> { ) -> Result<(), DirstateError> {
self.remove_file(filename, in_merge) self.remove_file(filename, in_merge)

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

def addfile( def addfile(
&self, &self,
f: PyObject, f: PyObject,
state: PyObject, state: PyObject,
mode: PyObject, mode: PyObject,
size: PyObject, size: PyObject,
mtime: PyObject, mtime: PyObject,
added: PyObject, added: 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) { let state = if state.is_none(py) {
// Arbitrary default value // Arbitrary default value
EntryState::Normal EntryState::Normal
}; };
let entry = DirstateEntry { let entry = DirstateEntry {
state: state, state: state,
mode: mode, mode: mode,
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 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(
filename, filename,
entry, entry,
added, added,
merged,
from_p2, from_p2,
possibly_dirty possibly_dirty
).and(Ok(py.None())).or_else(|e: DirstateError| { ).and(Ok(py.None())).or_else(|e: DirstateError| {
Err(PyErr::new::<exc::ValueError, _>(py, e.to_string())) Err(PyErr::new::<exc::ValueError, _>(py, e.to_string()))
}) })
} }
def removefile( def removefile(

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

self.get_mut().clear() self.get_mut().clear()
} }
fn add_file( fn add_file(
&mut self, &mut self,
filename: &HgPath, filename: &HgPath,
entry: DirstateEntry, entry: DirstateEntry,
added: bool, added: bool,
merged: bool,
from_p2: bool, from_p2: bool,
possibly_dirty: bool, possibly_dirty: bool,
) -> Result<(), DirstateError> { ) -> Result<(), DirstateError> {
self.get_mut().add_file( self.get_mut().add_file(
filename, filename,
entry, entry,
added, added,
merged,
from_p2, from_p2,
possibly_dirty, possibly_dirty,
) )
} }
fn remove_file( fn remove_file(
&mut self, &mut self,
filename: &HgPath, filename: &HgPath,