( )⚙ D10978 dirstate: no longer pass `oldstate` to the `dropfile`

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
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

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 28799

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)