This is an archive of the discontinued Mercurial Phabricator instance.

rust-discovery: starting core implementation
ClosedPublic

Authored by gracinet on Apr 12 2019, 2:33 PM.

Details

Summary

Once exposed to the Python side, this core object will avoid
costly roundtrips with potentially big sets of revisions.

This changeset implements the core logic of the object only, i.e.,
manipulation of the missing, common and undefined set-like revision
attributes.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

gracinet created this revision.Apr 12 2019, 2:33 PM
kevincox accepted this revision.Apr 14 2019, 4:14 PM

I'm not a huge fan of the design of this class because it has a strong protocol to use correctly. It would probably be better to have 3 classes and when you apply new information each class transitions into the next one. This allows the type system to ensure that you are using the class correctly. However it appears that this is just re-implementing an API so improving that API is probably out of scope for the time being.

Otherwise the implementation looks good.

rust/hg-core/src/discovery.rs
58

You can replace the early return by an if let. This also removed the .unwrap() which is always nice.

if let Some(ref mut undecided) = self.undecided {
    self.common.remove_ancestors_from(undecided)?;
}
74

I believe this .into_iter() is redundant because the for loop calls it for you.

94

This this imply that this method shouldn't be called until then? If so you should probably add a debug_assert!(self.is_complete()).

@kevincox Thanks for the reviews !

It would probably be better to have 3 classes and when you apply new information each class transitions into the next one. This allows the type system to ensure that you are using the class correctly.

That's an interesting idea. I expect it to solve my gripes with ensure_undecided() neatly. I'm not sure how it'd fare on the Python side, and probably wouldn't do it in the bindings to maintain compatibility, but I'll play with it a bit.
Maybe this could be a good topic for a follow-up ?
Note: I still have the various sampling methods to submit today or tomorrow, but I think they would play well with that idea.

That's an interesting idea. I expect it to solve my gripes with ensure_undecided() neatly. I'm not sure how it'd fare on the Python side, and probably wouldn't do it in the bindings to maintain compatibility, but I'll play with it a bit.

Well the point is to force the caller to acknowledge the state change, but in python that isn't really possible because it is dynamically typed. What I would probably do is still implement the Rust part with multiple structs but then wrap it in an enum for the python API. Then each python method call turns into.

fn do_thing() {
  match self {
    HasInfo(state) => state.do_thing()
    other => unreachable!("Invalid state for do_thing(). Expected HasInfo got {:?}.", other);
  }
}

And yes, this should allow you to avoid all of the unwraping since it would all be done at the API boundary.

I think you could also propagate this to python by having the "state" transition methods return a new object. However because you can't ensure that the code drops the access to the previous object you would have to copy everything into the new object or ensure that you don't mutate shared state. The value is still lower here though because of the dynamic nature of python.

Maybe this could be a good topic for a follow-up ?

Yeah, sounds reasonable.

What I would probably do is still implement the Rust part with multiple structs but then wrap it in an enum for the python API.

Yes, that's what I had in mind. Not sure where that enum would be defined (hg-cpython or hg-core) but it doesn't matter so much.

Yes, that's what I had in mind. Not sure where that enum would be defined (hg-cpython or hg-core) but it doesn't matter so much.

I would probably stick it in hg-cpython because it is a compromise for the python binding. For anyone using hg-core from rust I would hope that they directly use the structs.

But yes, not to important either way.

gracinet marked 2 inline comments as done.Apr 15 2019, 12:49 PM
gracinet updated this revision to Diff 14744.
gracinet added inline comments.Apr 15 2019, 12:49 PM
rust/hg-core/src/discovery.rs
58

ah, yes the trick is the ref mut inside the if let, I see

94

I'm not sure. It wouldn't be a straight error to call this early, as long as the caller knows what this means, but I don't have any use-case before the discovery is complete.

On the other hand, I could go as far as consume the discovery object and use self.common.into_bases_heads(), but maybe that could make things more complicated for the Python bindings.

kevincox added inline comments.Apr 15 2019, 12:59 PM
rust/hg-core/src/discovery.rs
94

If it is allowed can you document what happens what happens if you call it before then?

gracinet updated this revision to Diff 14794.Apr 17 2019, 8:16 AM
gracinet marked an inline comment as done.Apr 17 2019, 8:32 AM

I believe to have addressed your immediate concerns.

I played with splitting in different structs for the various stages of the process. It is indeed cleaner and clearer Rust code.
However mutating in place of Enums exposed to Python is a bit of a headache, because of the wrapping in RefCell<Box<T>>, and I don't have a solution for that, so I'm gonna think about that in a follow-up.

I believe to have addressed your immediate concerns.
I played with splitting in different structs for the various stages of the process. It is indeed cleaner and clearer Rust code.
However mutating in place of Enums exposed to Python is a bit of a headache, because of the wrapping in RefCell<Box<T>>, and I don't have a solution for that, so I'm gonna think about that in a follow-up.

Thanks, it looks good for now :)

This revision was automatically updated to reflect the committed changes.