Submitter | phabricator |
---|---|
Date | Oct. 16, 2019, 1:48 p.m. |
Message ID | <differential-rev-PHID-DREV-g2tzrcjpqvl43cznhomd-req@mercurial-scm.org> |
Download | mbox | patch |
Permalink | /patch/42384/ |
State | Superseded |
Headers | show |
Comments
> +++ b/mercurial/dirstate.py > @@ -919,6 +919,9 @@ > matchalways = match.always() > matchtdir = match.traversedir > dmap = self._map > + if rustmod is not None: > + dmap = self._map._rustmap If it's the same trick, can't it be abstracted away? `if rustmod` seems weird.
yuja added a comment. > +++ b/mercurial/dirstate.py > @@ -919,6 +919,9 @@ > > matchalways = match.always() > matchtdir = match.traversedir > dmap = self._map > > + if rustmod is not None: > + dmap = self._map._rustmap If it's the same trick, can't it be abstracted away? `if rustmod` seems weird. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7118/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7118 To: Alphare, #hg-reviewers Cc: yuja, mercurial-devel
Alphare added a comment. In D7118#104834 <https://phab.mercurial-scm.org/D7118#104834>, @yuja wrote: >> +++ b/mercurial/dirstate.py >> @@ -919,6 +919,9 @@ >> >> matchalways = match.always() >> matchtdir = match.traversedir >> dmap = self._map >> >> + if rustmod is not None: >> + dmap = self._map._rustmap > > If it's the same trick, can't it be abstracted away? `if rustmod` seems weird. The main issue I faced is that Python classes built from `rust-cpython` cannot be subclassed, so we have to resort to an additional level of indirection. IIRC, the main reason was that there was no way of enforcing a subclass to call `super()` which would result in the Rust class not being initialized properly. Or maybe I'm missing the point? REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7118/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7118 To: Alphare, #hg-reviewers Cc: yuja, mercurial-devel
> >> +++ b/mercurial/dirstate.py > >> @@ -919,6 +919,9 @@ > >> > >> matchalways = match.always() > >> matchtdir = match.traversedir > >> dmap = self._map > >> > >> + if rustmod is not None: > >> + dmap = self._map._rustmap > > > > If it's the same trick, can't it be abstracted away? `if rustmod` seems weird. > > The main issue I faced is that Python classes built from `rust-cpython` cannot be subclassed, so we have to resort to an additional level of indirection. IIRC, the main reason was that there was no way of enforcing a subclass to call `super()` which would result in the Rust class not being initialized properly. Or maybe I'm missing the point? I mean there could be a unified `self._map._map` call if we need to take the internal map from both py dirstatemap and rust dirstatemap.
yuja added a comment. > >> +++ b/mercurial/dirstate.py > >> @@ -919,6 +919,9 @@ > >> > >> matchalways = match.always() > >> matchtdir = match.traversedir > >> dmap = self._map > >> > >> + if rustmod is not None: > >> + dmap = self._map._rustmap > > > > If it's the same trick, can't it be abstracted away? `if rustmod` seems weird. > The main issue I faced is that Python classes built from `rust-cpython` cannot be subclassed, so we have to resort to an additional level of indirection. IIRC, the main reason was that there was no way of enforcing a subclass to call `super()` which would result in the Rust class not being initialized properly. Or maybe I'm missing the point? I mean there could be a unified `self._map._map` call if we need to take the internal map from both py dirstatemap and rust dirstatemap. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7118/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7118 To: Alphare, #hg-reviewers Cc: yuja, mercurial-devel
martinvonz added a comment. What's the state of this patch? The description makes it sound like it's a Rust patch, but it only modifies Python code. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7118/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7118 To: Alphare, #hg-reviewers Cc: martinvonz, yuja, mercurial-devel
Alphare added a comment.
In D7118#111735 <https://phab.mercurial-scm.org/D7118#111735>, @martinvonz wrote:
> What's the state of this patch? The description makes it sound like it's a Rust patch, but it only modifies Python code.
It only impacts the rust module policy, I figured the name was not a bad idea. Maybe I'm wrong?
I also did not find Yuya's suggestion to work, but I may have understood it wrong.
REPOSITORY
rHG Mercurial
CHANGES SINCE LAST ACTION
https://phab.mercurial-scm.org/D7118/new/
REVISION DETAIL
https://phab.mercurial-scm.org/D7118
To: Alphare, #hg-reviewers
Cc: martinvonz, yuja, mercurial-devel
martinvonz added a comment. In D7118#111784 <https://phab.mercurial-scm.org/D7118#111784>, @Alphare wrote: > In D7118#111735 <https://phab.mercurial-scm.org/D7118#111735>, @martinvonz wrote: > >> What's the state of this patch? The description makes it sound like it's a Rust patch, but it only modifies Python code. > > It only impacts the rust module policy, I figured the name was not a bad idea. Maybe I'm wrong? > I also did not find Yuya's suggestion to work, but I may have understood it wrong. I think Yuya's suggestion was to make the diff be this: diff --git a/mercurial/dirstate.py b/mercurial/dirstate.py --- a/mercurial/dirstate.py +++ b/mercurial/dirstate.py @@ -909,7 +909,7 @@ class dirstate(object): matchfn = match.matchfn matchalways = match.always() matchtdir = match.traversedir - dmap = self._map + dmap = self._map._map listdir = util.listdir lstat = os.lstat dirkind = stat.S_IFDIR Obviously, you'd need to make some adjustments to allow that, at least renaming `_rustmap` to `_map`. The commit message should also change then. I don't remember what the point of the nested `_map`s was (perhaps to make it possible to replace the inner `_map`?). Hopefully we're not breaking that by directly accessing the inner `_map`. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7118/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7118 To: Alphare, #hg-reviewers Cc: martinvonz, yuja, mercurial-devel
Patch
diff --git a/mercurial/dirstate.py b/mercurial/dirstate.py --- a/mercurial/dirstate.py +++ b/mercurial/dirstate.py @@ -919,6 +919,9 @@ matchalways = match.always() matchtdir = match.traversedir dmap = self._map + if rustmod is not None: + dmap = self._map._rustmap + listdir = util.listdir lstat = os.lstat dirkind = stat.S_IFDIR