This is an archive of the discontinued Mercurial Phabricator instance.

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

Authored by Alphare on Jan 14 2020, 12:33 PM.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

Alphare created this revision.Jan 14 2020, 12:33 PM
kevincox accepted this revision.Jan 15 2020, 11:07 AM
kevincox added inline comments.
rust/hg-core/src/utils/path_auditor.rs
54

Should this be std::path::is_separator(*c as char)?.

If not please add a comment explaining why.

55

It would be nice to have this in a helper function in path to get a component iterator.

73
Alphare marked 2 inline comments as done.Jan 15 2020, 4:05 PM
Alphare added inline comments.
rust/hg-core/src/utils/path_auditor.rs
55

I think that's a good idea indeed, but I would prefer to do it in another patch.

Alphare updated this revision to Diff 19317.Jan 15 2020, 4:55 PM
Alphare updated this revision to Diff 19351.Jan 16 2020, 5:21 AM
martinvonz requested changes to this revision.
martinvonz added inline comments.
rust/hg-core/src/utils/path_auditor.rs
56

As @yuja pointed out on 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.

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.

This revision now requires changes to proceed.Jan 23 2020, 1:16 PM
Alphare marked an inline comment as done.Jan 24 2020, 4:49 AM
Alphare added inline comments.
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.

martinvonz added inline comments.Jan 24 2020, 8:50 PM
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?
nit 2: does as_slice() better convey what the second line does?

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)?

martinvonz requested changes to this revision.Jan 28 2020, 12:20 PM
This revision now requires changes to proceed.Jan 28 2020, 12:20 PM
Alphare added inline comments.Feb 6 2020, 4:36 AM
rust/hg-core/src/utils/path_auditor.rs
58–59

nit 1: that creates a temporary variable that is instantly freed, so no luck there

87

I see. What error message do you propose?

martinvonz added inline comments.Feb 6 2020, 8:34 AM
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" :)

Alphare added inline comments.Feb 6 2020, 8:35 AM
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.

Alphare updated this revision to Diff 19934.Feb 6 2020, 10:14 AM
martinvonz added inline comments.Feb 6 2020, 10:57 AM
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.

martinvonz accepted this revision.Feb 6 2020, 11:01 AM
martinvonz added inline comments.
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 :)

This revision is now accepted and ready to land.Feb 6 2020, 11:01 AM
martinvonz added inline comments.Feb 6 2020, 11:45 AM
rust/hg-core/src/utils/path_auditor.rs
192

This test now fails. I'll fix it.

Alphare added inline comments.Feb 6 2020, 11:46 AM
rust/hg-core/src/utils/path_auditor.rs
192

Right, sorry!