D6773: rust-hgpath: add HgPath and HgPathBuf structs to encapsulate handling of paths

[yuja (Yuya Nishihara)]

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