( )⚙ D10953 dirstate: move most of the `remove` logic with dirstatemap `removefile`

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D10953

dirstate: move most of the `remove` logic with dirstatemap `removefile`
ClosedPublic

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

Details

Reviewers
SimonSapin
pulkit
Group Reviewers
hg-reviewers
Commits
rHGeaae39894312: dirstate: move most of the `remove` logic with dirstatemap `removefile`
Summary

This code deal with special logic to preserving "merged" and "from_p2" information when removing a file. These are implementation details that are more suitable for the dirstatemap layer. Since the dirstatemap layer alreaday have most of the information necessary to do so, the move is easy.

This move helps us to encapsulate more implementation details within the dirstatemap and its entry. Easing the use of a different storage for dirstate v2.

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:50 PM
SimonSapin accepted this revision.Jul 5 2021, 12:12 AM
SimonSapin added a subscriber: SimonSapin.
SimonSapin added inline comments.
rust/hg-cpython/src/dirstate/dirstate_map.rs
145

No need to go back and change it here, but I think rust-cpython supports specifying a PyBool or even bool type directly in the method signature, and takes of the conversion in that case

pulkit accepted this revision.Jul 6 2021, 12:24 PM
This revision is now accepted and ready to land.Jul 6 2021, 12:24 PM
baymax updated this revision to Diff 28855.Jul 6 2021, 2:08 PM

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

Revision Contents
Changeset List

Diff 28892

mercurial/dirstate.py

def add(self, f): def add(self, f):
'''Mark a file added.''' '''Mark a file added.'''
self._addpath(f, b'a', 0) self._addpath(f, b'a', 0)
self._map.copymap.pop(f, None) self._map.copymap.pop(f, None)
def remove(self, f): def remove(self, f):
'''Mark a file removed.''' '''Mark a file removed.'''
self._dirty = True self._dirty = True
oldstate = self[f]
size = 0
if self.in_merge:
entry = self._map.get(f)
if entry is not None:
# backup the previous state
if entry[0] == b'm': # merge
size = NONNORMAL
elif entry[0] == b'n' and entry[2] == FROM_P2: # other parent
size = FROM_P2
self._map.otherparentset.add(f)
self._updatedfiles.add(f) self._updatedfiles.add(f)
self._map.removefile(f, oldstate, size) self._map.removefile(f, in_merge=self.in_merge)
if size == 0:
self._map.copymap.pop(f, None)
def merge(self, f): def merge(self, f):
'''Mark a file merged.''' '''Mark a file merged.'''
if not self.in_merge: if not self.in_merge:
return self.normallookup(f) return self.normallookup(f)
return self.otherparent(f) return self.otherparent(f)
def drop(self, f): def drop(self, f):

mercurial/dirstatemap.py

if oldstate == b"?" and "_alldirs" in self.__dict__: if oldstate == b"?" and "_alldirs" in self.__dict__:
self._alldirs.addpath(f) self._alldirs.addpath(f)
self._map[f] = dirstatetuple(state, mode, size, mtime) self._map[f] = dirstatetuple(state, mode, size, mtime)
if state != b'n' or mtime == AMBIGUOUS_TIME: if state != b'n' or mtime == AMBIGUOUS_TIME:
self.nonnormalset.add(f) self.nonnormalset.add(f)
if size == FROM_P2: if size == FROM_P2:
self.otherparentset.add(f) self.otherparentset.add(f)
def removefile(self, f, oldstate, size): def removefile(self, f, in_merge=False):
""" """
Mark a file as removed in the dirstate. Mark a file as removed in the dirstate.
The `size` parameter is used to store sentinel values that indicate The `size` parameter is used to store sentinel values that indicate
the file's previous state. In the future, we should refactor this the file's previous state. In the future, we should refactor this
to be more explicit about what that state is. to be more explicit about what that state is.
""" """
if oldstate not in b"?r" and "_dirs" in self.__dict__: entry = self.get(f)
size = 0
if in_merge:
# XXX we should not be able to have 'm' state and 'FROM_P2' if not
# during a merge. So I (marmoute) am not sure we need the
# conditionnal at all. Adding double checking this with assert
# would be nice.
if entry is not None:
# backup the previous state
if entry[0] == b'm': # merge
size = NONNORMAL
elif entry[0] == b'n' and entry[2] == FROM_P2: # other parent
size = FROM_P2
self.otherparentset.add(f)
if size == 0:
self.copymap.pop(f, None)
if entry is not None and entry[0] != b'r' and "_dirs" in self.__dict__:
self._dirs.delpath(f) self._dirs.delpath(f)
if oldstate == b"?" and "_alldirs" in self.__dict__: if entry is None and "_alldirs" in self.__dict__:
self._alldirs.addpath(f) self._alldirs.addpath(f)
if "filefoldmap" in self.__dict__: if "filefoldmap" in self.__dict__:
normed = util.normcase(f) normed = util.normcase(f)
self.filefoldmap.pop(normed, None) self.filefoldmap.pop(normed, None)
self._map[f] = dirstatetuple(b'r', 0, size, 0) self._map[f] = dirstatetuple(b'r', 0, size, 0)
self.nonnormalset.add(f) self.nonnormalset.add(f)
def dropfile(self, f, oldstate): def dropfile(self, f, oldstate):

rust/hg-core/src/dirstate.rs

} }
const MTIME_UNSET: i32 = -1; const MTIME_UNSET: i32 = -1;
/// A `DirstateEntry` with a size of `-2` means that it was merged from the /// A `DirstateEntry` with a size of `-2` means that it was merged from the
/// other parent. This allows revert to pick the right status back during a /// other parent. This allows revert to pick the right status back during a
/// merge. /// merge.
pub const SIZE_FROM_OTHER_PARENT: i32 = -2; pub const SIZE_FROM_OTHER_PARENT: i32 = -2;
/// A special value used for internal representation of special case in
/// dirstate v1 format.
pub const SIZE_NON_NORMAL: i32 = -1;
pub type StateMap = FastHashMap<HgPathBuf, DirstateEntry>; pub type StateMap = FastHashMap<HgPathBuf, DirstateEntry>;
pub type StateMapIter<'a> = Box< pub type StateMapIter<'a> = Box<
dyn Iterator< dyn Iterator<
Item = Result<(&'a HgPath, DirstateEntry), DirstateV2ParseError>, Item = Result<(&'a HgPath, DirstateEntry), DirstateV2ParseError>,
> + Send > + Send
+ 'a, + 'a,
>; >;

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

// dirstate_map.rs // dirstate_map.rs
// //
// Copyright 2019 Raphaël Gomès <rgomes@octobus.net> // Copyright 2019 Raphaël Gomès <rgomes@octobus.net>
// //
// This software may be used and distributed according to the terms of the // This software may be used and distributed according to the terms of the
// GNU General Public License version 2 or any later version. // GNU General Public License version 2 or any later version.
use crate::dirstate::parsers::Timestamp; use crate::dirstate::parsers::Timestamp;
use crate::{ use crate::{
dirstate::EntryState, dirstate::EntryState,
dirstate::SIZE_FROM_OTHER_PARENT,
dirstate::SIZE_NON_NORMAL,
pack_dirstate, parse_dirstate, pack_dirstate, parse_dirstate,
utils::hg_path::{HgPath, HgPathBuf}, utils::hg_path::{HgPath, HgPathBuf},
CopyMap, DirsMultiset, DirstateEntry, DirstateError, DirstateParents, CopyMap, DirsMultiset, DirstateEntry, DirstateError, DirstateParents,
StateMap, StateMap,
}; };
use micro_timer::timed; use micro_timer::timed;
use std::collections::HashSet; use std::collections::HashSet;
use std::iter::FromIterator; use std::iter::FromIterator;
/// Mark a file as removed in the dirstate. /// Mark a file as removed in the dirstate.
/// ///
/// The `size` parameter is used to store sentinel values that indicate /// The `size` parameter is used to store sentinel values that indicate
/// the file's previous state. In the future, we should refactor this /// the file's previous state. In the future, we should refactor this
/// to be more explicit about what that state is. /// to be more explicit about what that state is.
pub fn remove_file( pub fn remove_file(
&mut self, &mut self,
filename: &HgPath, filename: &HgPath,
old_state: EntryState, in_merge: bool,
size: i32,
) -> Result<(), DirstateError> { ) -> Result<(), DirstateError> {
let old_entry_opt = self.get(filename);
let old_state = match old_entry_opt {
Some(e) => e.state,
None => EntryState::Unknown,
};
let mut size = 0;
if in_merge {
// XXX we should not be able to have 'm' state and 'FROM_P2' if not
// during a merge. So I (marmoute) am not sure we need the
// conditionnal at all. Adding double checking this with assert
// would be nice.
if let Some(old_entry) = old_entry_opt {
// backup the previous state
if old_entry.state == EntryState::Merged {
size = SIZE_NON_NORMAL;
} else if old_entry.state == EntryState::Normal
&& old_entry.size == SIZE_FROM_OTHER_PARENT
{
// other parent
size = SIZE_FROM_OTHER_PARENT;
self.get_non_normal_other_parent_entries()
.1
.insert(filename.to_owned());
}
}
}
if old_state != EntryState::Unknown && old_state != EntryState::Removed if old_state != EntryState::Unknown && old_state != EntryState::Removed
{ {
if let Some(ref mut dirs) = self.dirs { if let Some(ref mut dirs) = self.dirs {
dirs.delete_path(filename)?; dirs.delete_path(filename)?;
} }
} }
if old_state == EntryState::Unknown { if old_state == EntryState::Unknown {
if let Some(ref mut all_dirs) = self.all_dirs { if let Some(ref mut all_dirs) = self.all_dirs {
all_dirs.add_path(filename)?; all_dirs.add_path(filename)?;
} }
} }
if size == 0 {
self.copy_map.remove(filename);
}
self.state_map.insert( self.state_map.insert(
filename.to_owned(), filename.to_owned(),
DirstateEntry { DirstateEntry {
state: EntryState::Removed, state: EntryState::Removed,
mode: 0, mode: 0,
size, size,
mtime: 0, mtime: 0,

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

use bytes_cast::BytesCast; use bytes_cast::BytesCast;
use micro_timer::timed; use micro_timer::timed;
use std::borrow::Cow; use std::borrow::Cow;
use std::convert::TryInto; use std::convert::TryInto;
use std::path::PathBuf; use std::path::PathBuf;
use super::on_disk; use super::on_disk;
use super::on_disk::DirstateV2ParseError; use super::on_disk::DirstateV2ParseError;
use super::path_with_basename::WithBasename; use super::path_with_basename::WithBasename;
use crate::dirstate::parsers::pack_entry; use crate::dirstate::parsers::pack_entry;
use crate::dirstate::parsers::packed_entry_size; use crate::dirstate::parsers::packed_entry_size;
use crate::dirstate::parsers::parse_dirstate_entries; use crate::dirstate::parsers::parse_dirstate_entries;
use crate::dirstate::parsers::Timestamp; use crate::dirstate::parsers::Timestamp;
use crate::dirstate::SIZE_FROM_OTHER_PARENT;
use crate::dirstate::SIZE_NON_NORMAL;
use crate::matchers::Matcher; use crate::matchers::Matcher;
use crate::utils::hg_path::{HgPath, HgPathBuf}; use crate::utils::hg_path::{HgPath, HgPathBuf};
use crate::CopyMapIter; use crate::CopyMapIter;
use crate::DirstateEntry; use crate::DirstateEntry;
use crate::DirstateError; use crate::DirstateError;
use crate::DirstateParents; use crate::DirstateParents;
use crate::DirstateStatus; use crate::DirstateStatus;
use crate::EntryState; use crate::EntryState;
entry: DirstateEntry, entry: DirstateEntry,
) -> Result<(), DirstateError> { ) -> Result<(), DirstateError> {
Ok(self.add_or_remove_file(filename, old_state, entry)?) Ok(self.add_or_remove_file(filename, old_state, entry)?)
} }
fn remove_file( fn remove_file(
&mut self, &mut self,
filename: &HgPath, filename: &HgPath,
old_state: EntryState, in_merge: bool,
size: i32,
) -> Result<(), DirstateError> { ) -> Result<(), DirstateError> {
let old_entry_opt = self.get(filename)?;
let old_state = match old_entry_opt {
Some(e) => e.state,
None => EntryState::Unknown,
};
let mut size = 0;
if in_merge {
// XXX we should not be able to have 'm' state and 'FROM_P2' if not
// during a merge. So I (marmoute) am not sure we need the
// conditionnal at all. Adding double checking this with assert
// would be nice.
if let Some(old_entry) = old_entry_opt {
// backup the previous state
if old_entry.state == EntryState::Merged {
size = SIZE_NON_NORMAL;
} else if old_entry.state == EntryState::Normal
&& old_entry.size == SIZE_FROM_OTHER_PARENT
{
// other parent
size = SIZE_FROM_OTHER_PARENT;
}
}
}
if size == 0 {
self.copy_map_remove(filename)?;
}
let entry = DirstateEntry { let entry = DirstateEntry {
state: EntryState::Removed, state: EntryState::Removed,
mode: 0, mode: 0,
size, size,
mtime: 0, mtime: 0,
}; };
Ok(self.add_or_remove_file(filename, old_state, entry)?) Ok(self.add_or_remove_file(filename, old_state, entry)?)
} }

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

/// `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.
/// ///
/// `size` is not actually a size but the 0 or -1 or -2 value that would be /// `size` is not actually a size but the 0 or -1 or -2 value that would be
/// put in the size field in the dirstate-v1!format. /// put in the size field in the dirstate-v1!format.
fn remove_file( fn remove_file(
&mut self, &mut self,
filename: &HgPath, filename: &HgPath,
old_state: EntryState, in_merge: bool,
size: i32,
) -> Result<(), DirstateError>; ) -> Result<(), DirstateError>;
/// Drop information about this file from the map if any, and return /// Drop information about this file from the map if any, and return
/// whether there was any. /// whether there was any.
/// ///
/// `get` will now return `None` for this filename. /// `get` will now return `None` for this filename.
/// ///
/// `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
entry: DirstateEntry, entry: DirstateEntry,
) -> Result<(), DirstateError> { ) -> Result<(), DirstateError> {
self.add_file(filename, old_state, entry) self.add_file(filename, old_state, entry)
} }
fn remove_file( fn remove_file(
&mut self, &mut self,
filename: &HgPath, filename: &HgPath,
old_state: EntryState, in_merge: bool,
size: i32,
) -> Result<(), DirstateError> { ) -> Result<(), DirstateError> {
self.remove_file(filename, old_state, size) self.remove_file(filename, in_merge)
} }
fn drop_file( fn drop_file(
&mut self, &mut self,
filename: &HgPath, filename: &HgPath,
old_state: EntryState, old_state: EntryState,
) -> Result<bool, DirstateError> { ) -> Result<bool, DirstateError> {
self.drop_file(filename, old_state) self.drop_file(filename, old_state)

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

).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(
&self, &self,
f: PyObject, f: PyObject,
oldstate: PyObject, in_merge: PyObject
size: PyObject
) -> PyResult<PyObject> { ) -> PyResult<PyObject> {
self.inner(py).borrow_mut() self.inner(py).borrow_mut()
.remove_file( .remove_file(
HgPath::new(f.extract::<PyBytes>(py)?.data(py)), HgPath::new(f.extract::<PyBytes>(py)?.data(py)),
oldstate.extract::<PyBytes>(py)?.data(py)[0] in_merge.extract::<PyBool>(py)?.is_true(),
SimonSapinUnsubmitted
Not Done

No need to go back and change it here, but I think rust-cpython supports specifying a PyBool or even bool type directly in the method signature, and takes of the conversion in that case

SimonSapin: No need to go back and change it here, but I think rust-cpython supports specifying a `PyBool`…
.try_into()
.map_err(|e: HgError| {
PyErr::new::<exc::ValueError, _>(py, e.to_string())
})?,
size.extract(py)?,
) )
.or_else(|_| { .or_else(|_| {
Err(PyErr::new::<exc::OSError, _>( Err(PyErr::new::<exc::OSError, _>(
py, py,
"Dirstate error".to_string(), "Dirstate error".to_string(),
)) ))
})?; })?;
Ok(py.None()) Ok(py.None())

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

entry: DirstateEntry, entry: DirstateEntry,
) -> Result<(), DirstateError> { ) -> Result<(), DirstateError> {
self.get_mut().add_file(filename, old_state, entry) self.get_mut().add_file(filename, old_state, entry)
} }
fn remove_file( fn remove_file(
&mut self, &mut self,
filename: &HgPath, filename: &HgPath,
old_state: EntryState, in_merge: bool,
size: i32,
) -> Result<(), DirstateError> { ) -> Result<(), DirstateError> {
self.get_mut().remove_file(filename, old_state, size) self.get_mut().remove_file(filename, in_merge)
} }
fn drop_file( fn drop_file(
&mut self, &mut self,
filename: &HgPath, filename: &HgPath,
old_state: EntryState, old_state: EntryState,
) -> Result<bool, DirstateError> { ) -> Result<bool, DirstateError> {
self.get_mut().drop_file(filename, old_state) self.get_mut().drop_file(filename, old_state)