In D6631#97887, @Alphare wrote:

@kevincox I've renamed the file since it no longer just contains macros.
I've moved whatever I could move to separate structs and used Cell instead of RefCell on those scalar types. I'm not so keen on using UnsafeCell, espacially since the target py_class!-resulting struct could be using RefCell-specific APIs.

It seems weird to add this argument without using it. I would much rather it was added as the implementation was added.

In D6631#97216, @kevincox wrote:

Is there any reason this can't be done using Rc<RefCell<DirsMultiset>>? I have an example here: https://rust.godbolt.org/z/MNNR_F

Is there any reason this can't be done using Rc<RefCell<DirsMultiset>>? I have an example here: https://rust.godbolt.org/z/MNNR_F

In D6594#96518, @yuja wrote:

Yes, so we'll probably want to minimize the data exchanged between Rust
and Python. This means we'll eventually move more logic to Rust side, which
hopefully reduce the number of elements to be exposed through the iterator
interface. Until then, it might be better to move iterator-heavy objects
to CPython/PyO3 layer. Just my guess.

Is the SliceExt change related to the 2018 change? If not could you split the two?

Sorry. I was busy. In general don't worry about blocking on me, I can't promise any sort of reasonable response time. Worst case I can review the changes after submission and changes can be made afterwards.

Thanks, looks good.

Wouldn't it be better to make add_missing_revisions work properly on empty inputs? It seems like it is too error prone to try and catch every caller. If possible I would like to push this check down as far as possible.

The code and rust style looks fine. I'm not sure that taking the N smallest elements is the best approach for testing though. I can imagine it hiding some issues with larger values.

Thanks for the changes.

In D6231#91102, @gracinet wrote:

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.

In D6231#90756, @gracinet wrote:

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.

In D6231#90744, @gracinet wrote:

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.

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.

In D5441#81222, @yuja wrote:

Anyway, I just thought using CamelCase in Rust would be slightly nicer and
simpler. If that makes things complicated, I'll drop my idea and stick to
anything that is simpler.

This is a nice improvement. It still seems odd to me to return a list of two parents when there could be 0, 1 or 2 parents. I guess this is for performance reasons?

I think sticking close to the python version makes sense for the initial version. Then improvements can be made in follow-ups.

Overall looks good to me. Just a couple of points.

The latest changes are looking really good. I have a couple more comments but I didn't have time for a full review. I'll try to get more reviewed tomorrow. It seems that you still have a lot of stuff still in-flight so I'll try to slowly review the changes as I have time. If you want input/feedback on any particular part just ask and I will prioritize it.

Ah, I forgot to consider the python interop. Now the need for that crate makes sense. Thanks for explaining.

I'm not a windows expert but it seems like the rust OsStr, Path and filesystem APIs should handle these conversions for you. I think the only place where you would need to do os-specific code is when doing serialization and serialization which I think should be handled by https://doc.rust-lang.org/std/os/unix/ffi/trait.OsStringExt.html and https://doc.rust-lang.org/std/os/windows/ffi/trait.OsStringExt.html.

Rust has platform independent types PathBuf and &Path for paths and OsString and &OsStr for strings (owned and references respectively. They do have os-specific extensions but as long as you don't use them it should be cross platform. That being said, if you are serializing and deserializing them you may need to write some platform dependant code.

In D2057#43987, @durin42 wrote:

Mercurial tries to be principled about always treating filenames as bytes. AIUI https://www.mercurial-scm.org/wiki/WindowsUTF8Plan is still the plan of record there?

I will try to finish the review later, but it might be quicker if you incorporate some of the changes first since a lot of them are repeated many times. Overall it looks good, there are a couple of things that i would highlight to make the code easier to read.

I agree with the splitting comments :) In fact there might already be a base85 crate which can be used: https://docs.rs/zero85. Either way I'll hold off on the review, feel free to ping me when you are ready for me to take a look.

Overall it looks great. I added a bunch of nitpicks. The most common suggestion was to add some contextual information to error messages. Other then that mostly minor style improvements. Feel free to push back on anything you don't agree with :)