It does not offer the same flexibility as the Python implementation, but
should check incoming paths just as well.
Details
Details
- Reviewers
kevincox martinvonz - Group Reviewers
hg-reviewers - Commits
- rHGc18dd48cea4a: rust-pathauditor: add Rust implementation of the `pathauditor`
rHG4d6e568a1e0e: rust-pathauditor: add Rust implementation of the `pathauditor`
rHGa540e96b9029: rust-pathauditor: add Rust implementation of the `pathauditor`
Diff Detail
Diff Detail
- Repository
- rHG Mercurial
- Branch
- default
- Lint
No Linters Available - Unit
No Unit Test Coverage
Event Timeline
| rust/hg-core/src/utils/path_auditor.rs | ||
|---|---|---|
| 54 | I think that's a good idea indeed, but I would prefer to do it in another patch. | |
| rust/hg-core/src/utils/path_auditor.rs | ||
|---|---|---|
| 56 | ||
| 71 | nit: chain to_ascii_uppercase() onto the previous line (and drop mut) instead | |
| 73 | or is u8::is_ascii_digit preferred? (just a question; i don't know which is preferred) | |
| 74 | 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. | |
| 84–85 | 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? | |
| 87 | What's the 1 about? It seems to say it's okay if the path has .hg as its first component. Why? | |
| 88–91 | Can we eliminate this by using .enumerate() on line 85? | |
| 92–96 | 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. | |
| 115 | Should this be a PathBuf? It's weird to have a HgPath containing std::path::MAIN_SEPARATOR. | |
| 124–125 | 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? | |
| 145 | Could you replace "for" by "because" if that's what it means here. Or fix the sentence if that's not what you mean. | |
| rust/hg-core/src/utils/path_auditor.rs | ||
|---|---|---|
| 56 | Yes, it should just act on bytes and be a separate function in utils/files.rs. | |
| 73 | 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. | |
| 74 | Much clearer indeed, I don't know what I was thinking. | |
| 84–85 | I don't think that could ever happen, since the / is left intact lower-clean and no weird multi-byte shenanigans would arise. | |
| 87 | It would be valid to have a path inside the root .hg wouldn't it? | |
| 88–91 | Used pattern matching instead of unwrapping. | |
| 92–96 | 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. | |
| 115 | It has to be an HgPath, but I agree that I should just use a plain b'/'. | |
| 124–125 | I think the original comment mentions the logic within this function, not later uses of the PathAuditor. But maybe I misunderstood your question. | |
| 145 | This entire sentence is confusing and the first one is useful enough. I copied it over, but it's simpler without it. | |
| rust/hg-core/src/utils/path_auditor.rs | ||
|---|---|---|
| 58–59 | nit 1: inline the first first_component rather than redefining it on the next line? | |
| 87 | Since HgRepoPath is a path that's stored in the repo, I don't think so. Oh, we just want to provide a different error message so we handle that further up (lines 60-61). | |
| 124–125 | I've sent D8002 to fix the Python side. Can you update this to match (assuming I'm right, of course)? | |
| rust/hg-core/src/utils/path_auditor.rs | ||
|---|---|---|
| 87 | Sorry, I just meant that we do it this way so that we can provide different error messages for "inside .hg" and "inside subrepo" :) | |
| rust/hg-core/src/utils/path_auditor.rs | ||
|---|---|---|
| 87 | Good idea. I'm rewriting my patches, I will soon send an update for all of them. | |
| rust/hg-core/src/utils/path_auditor.rs | ||
|---|---|---|
| 87 | Well, you already have different errors for InsideDotHg and IsInsideNestedRepo, so I think you're good. I was just trying to explain the reason for the 1 there in the code (which I assume you just copied from Python). Sorry about the confusion. | |
| rust/hg-core/src/utils/path_auditor.rs | ||
|---|---|---|
| 87 | Oh, I only now looked at the diff from the previous review and you just added InsideDotHg. So my comment was useful then and you were not confused -- I was. Thanks for fixing :) | |
| rust/hg-core/src/utils/path_auditor.rs | ||
|---|---|---|
| 192 | This test now fails. I'll fix it. | |
| rust/hg-core/src/utils/path_auditor.rs | ||
|---|---|---|
| 192 | Right, sorry! | |
Should this be std::path::is_separator(*c as char)?.
If not please add a comment explaining why.