@@ -245,8 +245,11 @@

    pycompat.fsencode(getattr(mainmod, '__file__', ''))) == 'hg'):
    _sethgexecutable(pycompat.fsencode(mainmod.__file__))
else:

• exe = findexe('hg') or os.path.basename(sys.argv[0])
• _sethgexecutable(pycompat.fsencode(exe))

+ exe = findexe('hg')
+ if exe:
+ _sethgexecutable(pycompat.fsencode(exe))
+ else:
+ _sethgexecutable(os.path.basename(pycompat.sysargv[0]))

> @kevincox said "it would be good to [...] add a debug_assert! on
> creation/modification to check that everything is as expected."
> IIUC, his idea is that the `debug_assert!()` condition must always be met,
> which is the guarantee that the HgPath type provides. In order to ensure
> that, we'll have to validate all paths read from dirstate, for example,
> prior to casting to HgPath type. Otherwise `debug_assert!()` would fail,
> meaning the implementation had a bug.
> So, my question is, do we really need such strong guarantee by default?
I indeed do not think we do, for the reasons I gave in my previous comment.

+/ TODO improve handling of utf8 file names. Our overall strategy for
+/ filenames has to be revisited anyway, since Windows is UTF-16.
+pub fn normalize_case(bytes: &[u8]) -> Vec<u8> {
+ #[cfg(windows)] // NTFS compares via upper()
+ return bytes.to_ascii_uppercase();
+ #[cfg(unix)]
+ bytes.to_ascii_lowercase()
+}

• a/mercurial/utils/procutil.py

+++ b/mercurial/utils/procutil.py
@@ -246,7 +246,7 @@

    _sethgexecutable(pycompat.fsencode(mainmod.__file__))
else:
    exe = findexe('hg') or os.path.basename(sys.argv[0])

• _sethgexecutable(exe)

+ _sethgexecutable(pycompat.fsencode(exe))

> I found `std::path::Path` has a well-written inline comment about
> the unsafe pointer cast. Let's copy it so we won't introduce a
> memory issue.
I'm not 100% sure which comment you're referring to, sorry.

(resend to get around Phabricator's bad email processing)

+#[derive(Eq, Ord, PartialEq, PartialOrd, Debug, Hash)]
+pub struct HgPath {
+ inner: [u8],
+}

`hg config -T '{defaultvalue}'` shows a bunch of entries like `<object object at 0x7fc4295c00f0>`, which is clearly not something we want to show the user. Also `hg config -T json` crashes for a similar reason (`mercurial.error.ProgrammingError: cannot encode <object object at 0x7fdd1a6a20f0>`).

+impl PySharedState {
+ pub fn borrow_mut<'a, T>(
+ &'a self,
+ py: Python<'a>,
+ pyrefmut: RefMut<'a, T>,
+ ) -> PyResult<PyRefMut<'a, T>> {
+ if self.mutably_borrowed.get() {
+ return Err(AlreadyBorrowed::new(
+ py,
+ "Cannot borrow mutably while there exists another \
+ mutable reference in a Python object",
+ ));
+ }
+ match self.leak_count.get() {
+ 0 => {
+ self.mutably_borrowed.replace(true);
+ Ok(PyRefMut::new(py, pyrefmut, self))
+ }
+ TODO
+ For now, this works differently than Python references
+ in the case of iterators.
+ Python does not complain when the data an iterator
+ points to is modified if the iterator is never used
+ afterwards.
+ Here, we are stricter than this by refusing to give a
+ mutable reference if it is already borrowed.
+ While the additional safety might be argued for, it
+ breaks valid programming patterns in Python and we need
+ to fix this issue down the line.
+ _ => Err(AlreadyBorrowed::new(
+ py,
+ "Cannot borrow mutably while there are \
+ immutable references in Python objects",
+ )),
+ }
+ }
+
+ / Return a reference to the wrapped data with an artificial static
+ / lifetime.
+ / We need to be protected by the GIL for thread-safety.
+ pub fn leak_immutable<T>(
+ &self,
+ py: Python,
+ data: &RefCell<T>,
+ ) -> PyResult<&'static T> {
+ if self.mutably_borrowed.get() {
+ return Err(AlreadyBorrowed::new(
+ py,
+ "Cannot borrow immutably while there is a \
+ mutable reference in Python objects",
+ ));
+ }
+ let ptr = data.as_ptr();
+ self.leak_count.replace(self.leak_count.get() + 1);
+ unsafe { Ok(&*ptr) }
+ }

+ if let Some(ref mut file_fold_map) = self.file_fold_map {
+ file_fold_map
+ .remove::<Vec<u8>>(filename.to_ascii_uppercase().as_ref());

+ def getdirs(&self) -> PyResult<Dirs> {
+ TODO don't copy, share the reference
+ self.inner(py).borrow_mut().set_dirs();
+ Dirs::from_inner(
+ py,
+ DirsMultiset::new(
+ DirsIterable::Dirstate(&self.inner(py).borrow()),
+ Some(EntryState::Removed),
+ ),
+ )
+ }
+ def getalldirs(&self) -> PyResult<Dirs> {
+ TODO don't copy, share the reference
+ self.inner(py).borrow_mut().set_all_dirs();
+ Dirs::from_inner(
+ py,
+ DirsMultiset::new(
+ DirsIterable::Dirstate(&self.inner(py).borrow()),
+ None,
+ ),
+ )
+ }

The pycompat.bytestr() was there for py3 compatibility,

+ def configdefault(self, section, name):
+ """returns the default value of the config item"""
+ item = self._knownconfig.get(section, {}).get(name)
+ itemdefault = None
+ if item is not None:
+ if callable(item.default):
+ itemdefault = item.default()
+ else:
+ itemdefault = item.default
+ return itemdefault

@@ -936,7 +942,7 @@

if opts.get("name"):
    shelved.append(opts["name"])

• if abortf or continuef and not interactive:

+ if abortf or continuef:

Might be better to rename next to 5.1, and recreate new next file.
The old next file in stable branch could theoretically be edited and
merged back to default.

It looks like pretty much this entire function is copies commands.graft()
and then all this code is left dead here (since cont = True above).
I'd prefer to de-queue this commit and let you clean this up and then
submit a new patch

While this assertion had good intentions, it broke existing behavior with a
nasty panic.

I fixed the renames. The amended changeset is available at, bitbucket.org/octobus/mercurial-devel, rev d4bbf54624e14ac2913d452a8a7cb32823227c16. If you want me to send you the patch some other way, please tell me.

diff --git a/rust/hg-core/src/utils/mod.rs b/rust/hg-core/src/utils/mod.rs
deleted file mode 100644

• a/mercurial/__init__.py

+++ b/mercurial/__init__.py
@@ -225,7 +225,9 @@

• 1. It changes iteritems/values to items/values as they are not
  2. present in Python 3 world.
• elif fn in ('iteritems', 'itervalues'):

+ elif (fn in ('iteritems', 'itervalues') and
+ not (tokens[i - 1].type == token.NAME and
+ tokens[i - 1].string == 'def')):

We trace copies from the 'modify x' commit to commit 4 by going via
the merge base (commit 0). When tracing file 'y' (_tracefile()) in the
first case, we immediately find the rename from 'x'. We check to see
if 'x' exists in the merge base, which it does, so we consider it a
valid copy. In the second case, 'x' does not exist in the merge base,
so it's not considered a valid copy. As a workaround, this patch makes
it so we also attempt the check in mergecopies's base commit (commit 1
in the second case). That feels pretty ugly to me, but I don't have
any better ideas.

if proc:

• proc.communicate()

+ try:
+ proc.communicate()
+ except ValueError:
+ # This happens if we started iterating src and then
+ # get a parse error on a line. It should be safe to ignore.
+ pass

@@ -16,6 +16,7 @@
import collections
import contextlib
import errno
+import io
import os
import struct
import zlib
@@ -2306,7 +2307,7 @@

try:
    with self._datafp() as f:

• f.seek(0, 2)

+ f.seek(0, io.SEEK_END)

> I heard boxing a PyObject has measurable cost,
Do you have a link to any benchmarks? I'd be interested.

sharing references between Rust and Python:
https://raphaelgomes.dev/blog/articles/2019-07-01-sharing-references-between-python-and-rust.html.

Using `keys()` will change the behavior, as I actually need to expose an `Iter<Vec<u8>, u32>`. Is your issue with the naming?

+ pub fn contains_key(&self, key: &[u8]) -> bool {
+ self.inner.contains_key(key)
+ }
+
+ pub fn iter(&self) -> Iter<Vec<u8>, u32> {
+ self.inner.iter()
+ }

impl DirsMultiset {

/// Initializes the multiset from a dirstate or a manifest.
///

@@ -132,6 +123,22 @@

    Ok(())
}

+
+ pub fn contains_key(&self, key: &[u8]) -> bool {
+ self.inner.contains_key(key)
+ }
+
+ pub fn iter(&self) -> Iter<Vec<u8>, u32> {
+ self.inner.iter()
+ }
+
+ pub fn len(&self) -> usize {
+ self.inner.len()
+ }
+
+ pub fn get(&self, key: &[u8]) -> Option<&u32> {
+ self.inner.get(key)
+ }

To my eyes, the point of `Deref` is to make the `struct` act as a "fat pointer" from the outer interface code, but maybe this does not really help that much?

Need to update extensions._builtin so the shelve "extension" is silently
ignored.

• a/rust/hg-core/src/dirstate/dirs_multiset.rs

+++ b/rust/hg-core/src/dirstate/dirs_multiset.rs
@@ -118,7 +118,9 @@

    entry.remove();
}
Entry::Vacant(_) => {

• return Err(DirstateMapError::PathNotFound(path.to_owned()))

+ return Err(DirstateMapError::PathNotFound(
+ path.to_owned(),
+ ))

    }
};

+ def contains(&self, item: PyObject) -> PyResult<bool> {
+ Ok(self
+ .dirs_map(py)
+ .borrow()
+ .get(&item.extract::<PyBytes>(py)?.data(py).to_owned())
+ .is_some())

+impl DirsMultiset {
+ / Initializes the multiset from a dirstate or a manifest.
+ /
+ / If skip_state is provided, skips dirstate entries with equal state.
+ pub fn new(iterable: DirsIterable, skip_state: Option<i8>) -> Self {
+ let mut multiset = DirsMultiset {
+ inner: HashMap::new(),
+ };
+
+ match iterable {
+ DirsIterable::Dirstate(vec) => {
+ for (ref filename, DirstateEntry { state, .. }) in vec {
+ This if is optimized out of the loop
+ if let Some(skip) = skip_state {
+ if skip != state {
+ multiset.add_path(filename);
+ }
+ } else {
+ multiset.add_path(filename);
+ }
+ }
+ }
+ DirsIterable::Manifest(vec) => {
+ for ref filename in vec {
+ multiset.add_path(filename);
+ }
+ }
+ }
+
+ multiset
+ }

while work:

• r, i1, copies1 = heapq.heappop(work)

+ r, i1, copies = heapq.heappop(work)

if work and work[0][0] == r:
    # We are tracing copies from both parents
    r, i2, copies2 = heapq.heappop(work)

• copies = {}
• allcopies = set(copies1) | set(copies2)
• for dst in allcopies:

+ for dst, src in copies2.items():

• 1. Unlike when copies are stored in the filelog, we consider
  2. it a copy even if the destination already existed on the
  3. other branch. It's simply too expensive to check if the
  4. file existed in the manifest.
• if dst in copies1:
• # If it was copied on the p1 side, mark it as copied from

+ if dst not in copies:
+ # If it was copied on the p1 side, leave it as copied from

• 1. that side, even if it was also copied on the p2 side.
• copies[dst] = copies1[dst]
• else: copies[dst] = copies2[dst]

-@vcrcommand(b'debugcallconduit', [], _(b'METHOD'))
-def debugcallconduit(ui, repo, name):
+@vcrcommand(b'debugcallconduit', [], _(b'METHOD'), norepo=True)
+def debugcallconduit(ui, name):

Amended this to reproduce the copy history. No content change.

> I can't really comment on the storage format. I'm not keen on using extras
>  for this kind of stuff (including copies), but that seems be okay for
>  experiment.
Do we have a better place for it?

Test failures:

I can't really comment on the storage format. I'm not keen on using extras
for this kind of stuff (including copies), but that seems be okay for
experiment.

> > @@ -1384,26 +1384,26 @@
> > 
> >   >>> _rootsdirsandparents(
> >   ...     [(b'glob', b'g/h/*', b''), (b'glob', b'g/h', b''),
> >   ...      (b'glob', b'g*', b'')])
> > 
> > - (['g/h', 'g/h', ''], [], ['', 'g']) +    (['g/h', 'g/h', ''], [], set(['', 'g']))
>
> Perhaps, this would break py3 doctests. The repr syntax changed.
Good point. However, it turns out they were already broken (b'' prefixes), so I'll leave it for the py3 folks to clean up.

Perhaps, this would break py3 doctests. The repr syntax changed.