D6773: rust-hgpath: add HgPath and HgPathBuf structs to encapsulate handling of paths
yuja (Yuya Nishihara)
phabricator at mercurial-scm.org
Sun Sep 1 13:25:03 UTC 2019
yuja added a comment.
> > @kevincox said "it would be good to [...] add a debug_assert! on
> > creation/modification to check that everything is as expected."
> > IIUC, his idea is that the `debug_assert!()` condition must always be met,
> > which is the guarantee that the HgPath type provides. In order to ensure
> > that, we'll have to validate all paths read from dirstate, for example,
> > prior to casting to HgPath type. Otherwise `debug_assert!()` would fail,
> > meaning the implementation had a bug.
> > So, my question is, do we really need such strong guarantee by default?
> I indeed do not think we do, for the reasons I gave in my previous comment.
Okay, let's remove the debug_asserts, and update the documentation
accordingly.
> > Instead, we could lower the bar to "HgPath is merely a path represented in
> > platform-specific encoding though it's supposed to be a canonical path",
> > and add e.g. `is_canonicalized()` to check if the path meets the strong
> > requirement.
> Is it really represented in a platform-specific encoding or it just treated as ASCII until file system operations are needed?
It's a chunk of bytes. I just said "platform-specific encoding" to clarify
that the encoding might be different from OsStr. Sorry for confusion.
> I would use the strong check when converting to platform-specific types like `OsStr` or `Path`.
Perhaps, that's similar to what the pathauditor would do in Python.
> >> >> +impl Index<usize> for HgPath {
> >> >> + type Output = u8;
> >> >> +
> >> >> + fn index(&self, i: usize) -> &Self::Output {
> >> >> + &self.inner[i]
> >> >> + }
> >> >> +}
> >> >
> >> > [...]
> >> > Better to not implement `path[...]` since it can be considered returning
> >> > a path component at the specified index. We can always do
> >> > `path.as_bytes()[...]` explicitly.
> >> I'd leave `Index<RangeFull>`since it it not ambiguous and is a neat shorthand.
> >
> > What shorthand would it be used for?
> > I feel it's weird only `path[..]` is allowed.
> This is useful in `Deref` for example: `&self[..]`. But I can remove it if needed.
Maybe it can be written as `HgPath::new(&self.inner)`.
REPOSITORY
rHG Mercurial
CHANGES SINCE LAST ACTION
https://phab.mercurial-scm.org/D6773/new/
REVISION DETAIL
https://phab.mercurial-scm.org/D6773
To: Alphare, #hg-reviewers, kevincox
Cc: yuja, durin42, kevincox, mercurial-devel
More information about the Mercurial-devel
mailing list