Does this need to be public?

I'm completely new to Rust (just finished going through the tutorial), so please excuse naive questions...

What's the advantage of splitting up the definition of P like this? Why not constrain it as AsRef<Path> + Sync either on line 22 or line 28?

Looks like this will end up getting returned. Do we want that? It seems surprising to me.

Isn't this the same as vec!(HgPathBuf::new())? Do we dislike the vec! macro? Or macros in general?

What is this case about? If no files are specified, we want to return stat information for the (repo) root directory? That seems surprising. Does the caller depend on that? Does it have to?

It already is a vector, so no need to call .to_vec() here?

Please add a comment explaining what the type is. What does the outer Result mean? What does the Option mean? What are the two HgPathBufs? What does the inner Result mean?

Do we want to keep the trailing comma?

Do you think it would be easier to read and reason about this whole function if we did up to here in one step and the symlink_metadata() in a separate step after? By "step", I mean either in a separate .map() or in a separate par_iter(). I think it might be easier to follow if any errors from hg_path_to_path_buf() were checked earlier.

It doesn't matter yet, but should this be normalized instead of filename?

https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.collect talks about using a "turbofish". I don't know how specific you *want* to be about the type here (do you want all the details on line 44? If not, you can do .collect::<std::io::Result<Vec<_>>>(); here instead.

The tutorial seemed to put the ? where the expression was defined, i.e. at the end of line 63 here. Seems generally better to me so the variable gets a simpler type and so it's only evaluated once even if the variable is used in multiple places. (Maybe changing it requires the turbofish?)

Are we going to use the second value (the unnormalized filename) later? Otherwise I'd prefer to remove it from the tuple. Actually, I'd prefer to do that now anyway unless you already have patches that start using it.

This looks just like the is_dir() case. Should we merge them (i.e. just check if stat.is_file() { ... } else { ... })?

Are we going to do more here later? If not, we can just drop the Option-wrapping in stats_res (perhaps by adding a .filter() step to the iteration).

Are we going to care about the specific error type/message soon? Otherwise, could we replace it by an Option?

Call it normalized here too for consistency (and normalized is clearer than nf)

It does not, I used to export it to Python to test it.

I added the where clause well after defining the signature, it's indeed better to do either one of the two syntax.

It gets removed by the caller. I tried to replicate Python code to get close to feature-parity, but the more I dive into this, the less convinced I am that the Python implementation should really be trusted. I just removed it completely since it has no real effect on the current code.

It's the same and much cleaner, I just forgot about using this macro since most other collections don't have one.

I removed this block of code for the same reason as the sentinel above, it's not useful yet, and it might never be.

Although the inner type should be explained (which I did in my next patch), most of the time Result is easily identifiable by looking at the different ? within the associated scope, which - I think - makes it not really worth it to comment.

No. You would think that rustfmt should remove those, but apparently not.

For performance reasons I don't want to do any more loops than I need to. I'm already resorting to a sequential for loop right after because I haven't found an easy way of collecting into 3 different collections in parallel.
Overall, I think that the lack of comments is the real issue, this is not too complex.

Good catch!

I figured that having the full type written out was more helpful to understand the context. I still do, but I need comments as well. You're right that collecting into Result<Vec<_>> is cleaner and compiles just as well however. :)

I like to avoid the turbofish when possible because I think that it's harder to read that a normal variable type declaration.

It is indeed preferable to put the ? where the expression is defined, however in certain cases such as this one, the type inference is not good enough. Here, we're collecting into a Result; changing it to collect? into a Vec causes the compiler to ask us for type annotations.

I could also have a line right after to do let stats_res = stats_res?; but I don't think it's really useful since we're only using it once.

Collecting into a Result prevents us from doing a filter_map during collect. Implementing a ParallelIterator adapter that would do a filter_map with Result handling is possible, but would require more work.

In this specific case, as I eluded to above, I just removed the sentinel value so the question is not relevant anymore.

The symlink_metadata returns a Result, we don't care about the error here. It's the Python equivalent of a try except or try finally fallback.

Done with &[impl AsRef<HgPath> + Sync] which is more generic.

I could indeed probably simplify it. I'll update this part later today (Paris time) .

I had to add a Copy trait bound for it to work, otherwise it would be moved by walk_explicit before usage in stat_dmap_entries

It is indeed a much better idea. I'll also send a patch for the other uses of unimplemented!() in the rest of the codebase

Do we need to borrow it mutably?

Should this be stat.file_type().is_file() || stat.file_type().is_symlink() as below?

This nested match was hard for me to read. I think it would be simpler if you merged this one with the next one inside a simple None => match block, like this:

None => {
    match state {
        EntryState::Normal
        | EntryState::Merged
        | EntryState::Added => {
            deleted.push(filename);
        },
        EntryState::Removed => {
            removed.push(filename);
        },
        _ => {}
    }
}

We don't anymore, thanks. I'll be reworking the overly complicated match expression within the next hour.

What does this case mean? I.e. why would we get "NotADirectory"? Oh, I guess if e.g. dir/file is in the dirstate, but dir is now a file? Could you add a comment here saying that that is what this is about?