D7866: rust-pathauditor: add Rust implementation of the `pathauditor`

[Alphare (Raphaël Gomès)]

Alphare added inline comments.
Alphare marked an inline comment as done.

INLINE COMMENTS

> martinvonz wrote in path_auditor.rs:55
> As @yuja pointed out on D7864 <https://phab.mercurial-scm.org/D7864>, it's weird to have `split_drive()` on `HgPath`, because that is supposed to be a repo-relative path. I think it would be better to move it into this file (or elsewhere). Can be done in a follow-up.

Yes, it should just act on bytes and be a separate function in `utils/files.rs`.

> martinvonz wrote in path_auditor.rs:72
> or is `u8::is_ascii_digit` preferred? (just a question; i don't know which is preferred)

I like the point-free approach, I don't often remember to use it, since most of the type iterator adapters are not that trivial.

> martinvonz wrote in path_auditor.rs:73
> Is this line equivalent to `&& (first == b"HG" || first == b"HG8B6C")`? If so, I would strongly prefer that.
> 
> Same comment on line 56, actually (I reviewed out of of order). Would probably be helpful to extract `&lower_clean(parts[0]).as_ref()` to a variable anyway.

Much clearer indeed, I don't know what I was thinking.

> martinvonz wrote in path_auditor.rs:83-84
> Would we ever get different results from `lower_clean(path).split()` and `split(path).map(lower_clean)`? If not, shouldn't we reuse the `lower_clean()`ed result from above and call `split()` on that?

I don't think that could ever happen, since the `/` is left intact `lower-clean` and no weird multi-byte shenanigans would arise.

> martinvonz wrote in path_auditor.rs:86
> What's the `1` about? It seems to say it's okay if the path has `.hg` as its first component. Why?

It would be valid to have a path inside the root `.hg` wouldn't it?

> martinvonz wrote in path_auditor.rs:87-90
> Can we eliminate this by using `.enumerate()` on line 85?

Used pattern matching instead of unwrapping.

> martinvonz wrote in path_auditor.rs:91-95
> Will it be confusing that this is not necessarily a prefix of `path`? Specifically, it seems like it's going to be lower-case and with possibly different path separators.

I wondered when looking at the Python implementation, but I think this has the added value of telling the user what the path was transformed to. Maybe in a follow-up we could change add the original path in the error message... but I'm not sure it's worth the hassle.

> martinvonz wrote in path_auditor.rs:114
> Should this be a `PathBuf`? It's weird to have a `HgPath` containing `std::path::MAIN_SEPARATOR`.

It has to be an `HgPath`, but I agree that I should just use a plain `b'/'`.

> martinvonz wrote in path_auditor.rs:123-124
> It looks like we check the prefixes in order, so first "foo", then "foo/bar", then "foo/bar/baz". We'd return early if we find "foo/.hg", before we get to "foo/bar", no? What am I missing?

I think the original comment mentions the logic within this function, not later uses of the PathAuditor. But maybe I misunderstood your question.

> martinvonz wrote in path_auditor.rs:144
> Could you replace "for" by "because" if that's what it means here. Or fix the sentence if that's not what you mean.

This entire sentence is confusing and the first one is useful enough. I copied it over, but it's simpler without it.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7866/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7866

To: Alphare, #hg-reviewers, kevincox, martinvonz
Cc: martinvonz, yuja, durin42, kevincox, mercurial-devel