This is an archive of the discontinued Mercurial Phabricator instance.

Differential D10978

dirstate: no longer pass `oldstate` to the `dropfile`
ClosedPublic

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

Details

Reviewers
SimonSapin
pulkit
Group Reviewers
hg-reviewers
Commits
rHG6025353c9c55: dirstate: no longer pass `oldstate` to the `dropfile`
Summary

The oldstate value come literally from _map so we don't need to pass tha
information along.

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:52 PM
SimonSapin accepted this revision.Jul 5 2021, 1:26 AM
SimonSapin added a subscriber: SimonSapin.
SimonSapin added inline comments.
mercurial/dirstatemap.py
244–245

Nit: these two lines would be clearer to me in an else

marmoute added inline comments.Jul 5 2021, 4:47 AM
mercurial/dirstatemap.py
244–245

I don't disagree, but the "pre-assign the default case" have been the policy in the codebase. (Hoever, we could decide to change mpm era policy)

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

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

pulkit accepted this revision.Jul 6 2021, 3:11 PM
This revision is now accepted and ready to land.Jul 6 2021, 3:11 PM
marmoute added a commit: rHG6025353c9c55: dirstate: no longer pass `oldstate` to the `dropfile`.Jul 6 2021, 3:42 PM
Closed by commit rHG6025353c9c55: dirstate: no longer pass `oldstate` to the `dropfile` (authored by marmoute). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 28916

mercurial/dirstate.py

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):
'''Drop a file from the dirstate''' '''Drop a file from the dirstate'''
oldstate = self[f] if self._map.dropfile(f):
if self._map.dropfile(f, oldstate):
self._dirty = True self._dirty = True
self._updatedfiles.add(f) self._updatedfiles.add(f)
self._map.copymap.pop(f, None) self._map.copymap.pop(f, None)
def _discoverpath(self, path, normed, ignoremissing, exists, storemap): def _discoverpath(self, path, normed, ignoremissing, exists, storemap):
if exists is None: if exists is None:
exists = os.path.lexists(os.path.join(self._root, path)) exists = os.path.lexists(os.path.join(self._root, path))
if not exists: if not exists:
# This is equivalent to 'state, mode, size, time = dmap[fn]' but not # This is equivalent to 'state, mode, size, time = dmap[fn]' but not
# written like that for performance reasons. dmap[fn] is not a # written like that for performance reasons. dmap[fn] is not a
# Python tuple in compiled builds. The CPython UNPACK_SEQUENCE # Python tuple in compiled builds. The CPython UNPACK_SEQUENCE
# opcode has fast paths when the value to be unpacked is a tuple or # opcode has fast paths when the value to be unpacked is a tuple or
# a list, but falls back to creating a full-fledged iterator in # a list, but falls back to creating a full-fledged iterator in
# general. That is much slower than simply accessing and storing the # general. That is much slower than simply accessing and storing the
# tuple members one by one. # tuple members one by one.
t = dget(fn) t = dget(fn)
state = t.state
mode = t[1] mode = t[1]
size = t[2] size = t[2]
time = t[3] time = t[3]
if not st and t.tracked: if not st and t.tracked:
dadd(fn) dadd(fn)
elif t.merged: elif t.merged:
madd(fn) madd(fn)

mercurial/dirstatemap.py

if entry is None 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):
""" """
Remove a file from the dirstate. Returns True if the file was Remove a file from the dirstate. Returns True if the file was
previously recorded. previously recorded.
""" """
exists = self._map.pop(f, None) is not None old_entry = self._map.pop(f, None)
exists = False
oldstate = b'?'
SimonSapinUnsubmitted
Not Done

Nit: these two lines would be clearer to me in an else

SimonSapin: Nit: these two lines would be clearer to me in an `else`
marmouteAuthorUnsubmitted
Done

I don't disagree, but the "pre-assign the default case" have been the policy in the codebase. (Hoever, we could decide to change mpm era policy)

marmoute: I don't disagree, but the "pre-assign the default case" have been the policy in the codebase.
if old_entry is not None:
exists = True
oldstate = old_entry.state
if exists: if exists:
if oldstate != b"r" and "_dirs" in self.__dict__: if oldstate != b"r" and "_dirs" in self.__dict__:
self._dirs.delpath(f) self._dirs.delpath(f)
if "_alldirs" in self.__dict__: if "_alldirs" in self.__dict__:
self._alldirs.delpath(f) self._alldirs.delpath(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)

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

Ok(()) Ok(())
} }
/// Remove a file from the dirstate. /// Remove a file from the dirstate.
/// Returns `true` if the file was previously recorded. /// Returns `true` if the file was previously recorded.
pub fn drop_file( pub fn drop_file(
&mut self, &mut self,
filename: &HgPath, filename: &HgPath,
old_state: EntryState,
) -> Result<bool, DirstateError> { ) -> Result<bool, DirstateError> {
let old_state = match self.get(filename) {
Some(e) => e.state,
None => EntryState::Unknown,
};
let exists = self.state_map.remove(filename).is_some(); let exists = self.state_map.remove(filename).is_some();
if exists { if exists {
if old_state != EntryState::Removed { if 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)?;
} }
} }

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

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)?)
} }
fn drop_file( fn drop_file(&mut self, filename: &HgPath) -> Result<bool, DirstateError> {
&mut self, let old_state = match self.get(filename)? {
filename: &HgPath, Some(e) => e.state,
old_state: EntryState, None => EntryState::Unknown,
) -> Result<bool, DirstateError> { };
struct Dropped { struct Dropped {
was_tracked: bool, was_tracked: bool,
had_entry: bool, had_entry: bool,
had_copy_source: bool, had_copy_source: bool,
} }
/// If this returns `Ok(Some((dropped, removed)))`, then /// If this returns `Ok(Some((dropped, removed)))`, then
/// ///

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

use std::path::PathBuf; use std::path::PathBuf;
use crate::dirstate::parsers::Timestamp; use crate::dirstate::parsers::Timestamp;
use crate::dirstate_tree::on_disk::DirstateV2ParseError; use crate::dirstate_tree::on_disk::DirstateV2ParseError;
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::DirstateMap; use crate::DirstateMap;
use crate::DirstateParents; use crate::DirstateParents;
use crate::DirstateStatus; use crate::DirstateStatus;
use crate::EntryState;
use crate::PatternFileWarning; use crate::PatternFileWarning;
use crate::StateMapIter; use crate::StateMapIter;
use crate::StatusError; use crate::StatusError;
use crate::StatusOptions; use crate::StatusOptions;
/// `rust/hg-cpython/src/dirstate/dirstate_map.rs` implements in Rust a /// `rust/hg-cpython/src/dirstate/dirstate_map.rs` implements in Rust a
/// `DirstateMap` Python class that wraps `Box<dyn DirstateMapMethods + Send>`, /// `DirstateMap` Python class that wraps `Box<dyn DirstateMapMethods + Send>`,
/// a trait object of this trait. Except for constructors, this trait defines /// a trait object of this trait. Except for constructors, this trait defines
/// 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
/// 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 drop_file( fn drop_file(&mut self, filename: &HgPath) -> Result<bool, DirstateError>;
&mut self,
filename: &HgPath,
old_state: EntryState,
) -> Result<bool, DirstateError>;
/// Among given files, mark the stored `mtime` as ambiguous if there is one /// Among given files, mark the stored `mtime` as ambiguous if there is one
/// (if `state == EntryState::Normal`) equal to the given current Unix /// (if `state == EntryState::Normal`) equal to the given current Unix
/// timestamp. /// timestamp.
fn clear_ambiguous_times( fn clear_ambiguous_times(
&mut self, &mut self,
filenames: Vec<HgPathBuf>, filenames: Vec<HgPathBuf>,
now: i32, now: i32,
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)
} }
fn drop_file( fn drop_file(&mut self, filename: &HgPath) -> Result<bool, DirstateError> {
&mut self, self.drop_file(filename)
filename: &HgPath,
old_state: EntryState,
) -> Result<bool, DirstateError> {
self.drop_file(filename, old_state)
} }
fn clear_ambiguous_times( fn clear_ambiguous_times(
&mut self, &mut self,
filenames: Vec<HgPathBuf>, filenames: Vec<HgPathBuf>,
now: i32, now: i32,
) -> Result<(), DirstateV2ParseError> { ) -> Result<(), DirstateV2ParseError> {
Ok(self.clear_ambiguous_times(filenames, now)) Ok(self.clear_ambiguous_times(filenames, now))

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

parsers::dirstate_parents_to_pytuple, parsers::dirstate_parents_to_pytuple,
}; };
use hg::{ use hg::{
dirstate::parsers::Timestamp, dirstate::parsers::Timestamp,
dirstate::MTIME_UNSET, dirstate::MTIME_UNSET,
dirstate::SIZE_NON_NORMAL, dirstate::SIZE_NON_NORMAL,
dirstate_tree::dispatch::DirstateMapMethods, dirstate_tree::dispatch::DirstateMapMethods,
dirstate_tree::on_disk::DirstateV2ParseError, dirstate_tree::on_disk::DirstateV2ParseError,
errors::HgError,
revlog::Node, revlog::Node,
utils::files::normalize_case, utils::files::normalize_case,
utils::hg_path::{HgPath, HgPathBuf}, utils::hg_path::{HgPath, HgPathBuf},
DirstateEntry, DirstateError, DirstateMap as RustDirstateMap, DirstateEntry, DirstateError, DirstateMap as RustDirstateMap,
DirstateParents, EntryState, StateMapIter, DirstateParents, EntryState, StateMapIter,
}; };
// TODO // TODO
)) ))
})?; })?;
Ok(py.None()) Ok(py.None())
} }
def dropfile( def dropfile(
&self, &self,
f: PyObject, f: PyObject,
oldstate: PyObject
) -> PyResult<PyBool> { ) -> PyResult<PyBool> {
self.inner(py).borrow_mut() self.inner(py).borrow_mut()
.drop_file( .drop_file(
HgPath::new(f.extract::<PyBytes>(py)?.data(py)), HgPath::new(f.extract::<PyBytes>(py)?.data(py)),
oldstate.extract::<PyBytes>(py)?.data(py)[0]
.try_into()
.map_err(|e: HgError| {
PyErr::new::<exc::ValueError, _>(py, e.to_string())
})?,
) )
.and_then(|b| Ok(b.to_py_object(py))) .and_then(|b| Ok(b.to_py_object(py)))
.or_else(|e| { .or_else(|e| {
Err(PyErr::new::<exc::OSError, _>( Err(PyErr::new::<exc::OSError, _>(
py, py,
format!("Dirstate error: {}", e.to_string()), format!("Dirstate error: {}", e.to_string()),
)) ))
}) })

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

use crate::dirstate::owning::OwningDirstateMap; use crate::dirstate::owning::OwningDirstateMap;
use hg::dirstate::parsers::Timestamp; use hg::dirstate::parsers::Timestamp;
use hg::dirstate_tree::dispatch::DirstateMapMethods; use hg::dirstate_tree::dispatch::DirstateMapMethods;
use hg::dirstate_tree::on_disk::DirstateV2ParseError; use hg::dirstate_tree::on_disk::DirstateV2ParseError;
use hg::matchers::Matcher; use hg::matchers::Matcher;
use hg::utils::hg_path::{HgPath, HgPathBuf}; use hg::utils::hg_path::{HgPath, HgPathBuf};
use hg::CopyMapIter; use hg::CopyMapIter;
use hg::DirstateEntry; use hg::DirstateEntry;
use hg::DirstateError; use hg::DirstateError;
use hg::DirstateParents; use hg::DirstateParents;
use hg::DirstateStatus; use hg::DirstateStatus;
use hg::EntryState;
use hg::PatternFileWarning; use hg::PatternFileWarning;
use hg::StateMapIter; use hg::StateMapIter;
use hg::StatusError; use hg::StatusError;
use hg::StatusOptions; use hg::StatusOptions;
use std::path::PathBuf; use std::path::PathBuf;
impl DirstateMapMethods for OwningDirstateMap { impl DirstateMapMethods for OwningDirstateMap {
fn clear(&mut self) { fn clear(&mut self) {
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.get_mut().remove_file(filename, in_merge) self.get_mut().remove_file(filename, in_merge)
} }
fn drop_file( fn drop_file(&mut self, filename: &HgPath) -> Result<bool, DirstateError> {
&mut self, self.get_mut().drop_file(filename)
filename: &HgPath,
old_state: EntryState,
) -> Result<bool, DirstateError> {
self.get_mut().drop_file(filename, old_state)
} }
fn clear_ambiguous_times( fn clear_ambiguous_times(
&mut self, &mut self,
filenames: Vec<HgPathBuf>, filenames: Vec<HgPathBuf>,
now: i32, now: i32,
) -> Result<(), DirstateV2ParseError> { ) -> Result<(), DirstateV2ParseError> {
self.get_mut().clear_ambiguous_times(filenames, now) self.get_mut().clear_ambiguous_times(filenames, now)