Patchwork D7526: rust-hg-path: add method to get part of a path relative to a prefix

login
register
mail settings
Submitter phabricator
Date Nov. 29, 2019, 5:57 p.m.
Message ID <differential-rev-PHID-DREV-nywc2wtvqt6afn4vwwbs-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/43532/
State Superseded
Headers show

Comments

phabricator - Nov. 29, 2019, 5:57 p.m.
Alphare created this revision.
Herald added subscribers: mercurial-devel, kevincox, durin42.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  This will be used in the next patch in this series.

REPOSITORY
  rHG Mercurial

BRANCH
  default

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

AFFECTED FILES
  rust/hg-core/src/utils/hg_path.rs

CHANGE DETAILS




To: Alphare, #hg-reviewers
Cc: durin42, kevincox, mercurial-devel
phabricator - Dec. 2, 2019, 1:17 p.m.
This revision now requires changes to proceed.
kevincox added inline comments.
kevincox requested changes to this revision.

INLINE COMMENTS

> hg_path.rs:131
> +        let base = base.as_ref();
> +        if base.len() == 0 {
> +            return Some(self);

`base.is_empty()`? If we don't have that helper we should probably add it.

> hg_path.rs:131
> +        let base = base.as_ref();
> +        if base.len() == 0 {
> +            return Some(self);

The comment says that `base` must end with a `/` however you appear to handle cases where it doesn't Can you either update the documentation or the code?

> hg_path.rs:134
> +        }
> +        let is_dir = base.len() > 0 && base.as_bytes().last() == Some(&b'/');
> +        if is_dir && self.starts_with(base) {

Instead of using `.last()` can we add and use `base.ends_with(b'/')`?

> hg_path.rs:134
> +        }
> +        let is_dir = base.len() > 0 && base.as_bytes().last() == Some(&b'/');
> +        if is_dir && self.starts_with(base) {

`base.len() > 0` is redundant both with the early return above and because `last()` will return `None` if `base` is empty.

> hg_path.rs:215
> +    fn eq(&self, other: &HgPath) -> bool {
> +        self.inner == other.as_bytes()
> +    }

I would do `self.as_bytes()` for consistency.

Or even better I believe you can `#[derive(Eq,PartialEq)]`.

REPOSITORY
  rHG Mercurial

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

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

To: Alphare, #hg-reviewers, kevincox
Cc: durin42, kevincox, mercurial-devel
phabricator - Dec. 2, 2019, 2:21 p.m.
Alphare added inline comments.

INLINE COMMENTS

> kevincox wrote in hg_path.rs:131
> The comment says that `base` must end with a `/` however you appear to handle cases where it doesn't Can you either update the documentation or the code?

I've clarified the documentation.

REPOSITORY
  rHG Mercurial

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

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

To: Alphare, #hg-reviewers, kevincox
Cc: durin42, kevincox, mercurial-devel
phabricator - Dec. 10, 2019, 6:04 p.m.
martinvonz added inline comments.

INLINE COMMENTS

> hg_path.rs:438
> +        let base = HgPath::new(b"");
> +        assert_eq!(Some(path), path.relative_to(base));
> +

The argument order here feels backwards. I know some frameworks (like Java's JUnit) put the expected value first, but the definition of `assert_eq!` doesn't seem to have any opinion and https://doc.rust-lang.org/stable/rust-by-example/testing/unit_testing.html puts the actual value first. What do you think about changing the order? That would make it more readable to me.

REPOSITORY
  rHG Mercurial

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

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

To: Alphare, #hg-reviewers, kevincox, pulkit
Cc: martinvonz, durin42, kevincox, mercurial-devel
phabricator - Dec. 11, 2019, 8:42 a.m.
Alphare added inline comments.

INLINE COMMENTS

> martinvonz wrote in hg_path.rs:438
> The argument order here feels backwards. I know some frameworks (like Java's JUnit) put the expected value first, but the definition of `assert_eq!` doesn't seem to have any opinion and https://doc.rust-lang.org/stable/rust-by-example/testing/unit_testing.html puts the actual value first. What do you think about changing the order? That would make it more readable to me.

It looks that I took the habit because the test runner in PyCharm that I used when starting with Rust prints `expected` and `actual` in that order. I can reverse it if you really think it's more readable this way.

REPOSITORY
  rHG Mercurial

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

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

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

Patch

diff --git a/rust/hg-core/src/utils/hg_path.rs b/rust/hg-core/src/utils/hg_path.rs
--- a/rust/hg-core/src/utils/hg_path.rs
+++ b/rust/hg-core/src/utils/hg_path.rs
@@ -112,6 +112,9 @@ 
     pub fn contains(&self, other: u8) -> bool {
         self.inner.contains(&other)
     }
+    pub fn starts_with(&self, needle: impl AsRef<HgPath>) -> bool {
+        self.inner.starts_with(needle.as_ref().as_bytes())
+    }
     pub fn join<T: ?Sized + AsRef<HgPath>>(&self, other: &T) -> HgPathBuf {
         let mut inner = self.inner.to_owned();
         if inner.len() != 0 && inner.last() != Some(&b'/') {
@@ -120,6 +123,21 @@ 
         inner.extend(other.as_ref().bytes());
         HgPathBuf::from_bytes(&inner)
     }
+    /// Given a base directory (has to end with a `b'/'`), returns the slice
+    /// of `self` relative to the base directory. If `base` is not a directory,
+    /// returns `None`.
+    pub fn relative_to(&self, base: impl AsRef<HgPath>) -> Option<&HgPath> {
+        let base = base.as_ref();
+        if base.len() == 0 {
+            return Some(self);
+        }
+        let is_dir = base.len() > 0 && base.as_bytes().last() == Some(&b'/');
+        if is_dir && self.starts_with(base) {
+            Some(HgPath::new(&self.inner[base.len()..]))
+        } else {
+            None
+        }
+    }
     /// Checks for errors in the path, short-circuiting at the first one.
     /// This generates fine-grained errors useful for debugging.
     /// To simply check if the path is valid during tests, use `is_valid`.
@@ -192,6 +210,12 @@ 
     }
 }
 
+impl PartialEq<HgPath> for HgPathBuf {
+    fn eq(&self, other: &HgPath) -> bool {
+        self.inner == other.as_bytes()
+    }
+}
+
 impl fmt::Display for HgPathBuf {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         write!(
@@ -416,4 +440,35 @@ 
         let path = HgPathBuf::from_bytes(b"a").join(HgPath::new(b"/b"));
         assert_eq!(b"a//b", path.as_bytes());
     }
+
+    #[test]
+    fn test_relative_to() {
+        let path = HgPath::new(b"");
+        let base = HgPath::new(b"");
+        assert_eq!(Some(path), path.relative_to(base));
+
+        let path = HgPath::new(b"path");
+        let base = HgPath::new(b"");
+        assert_eq!(Some(path), path.relative_to(base));
+
+        let path = HgPath::new(b"a");
+        let base = HgPath::new(b"b");
+        assert_eq!(None, path.relative_to(base));
+
+        let path = HgPath::new(b"a/b");
+        let base = HgPath::new(b"a");
+        assert_eq!(None, path.relative_to(base));
+
+        let path = HgPath::new(b"a/b");
+        let base = HgPath::new(b"a/");
+        assert_eq!(Some(HgPath::new(b"b")), path.relative_to(base));
+
+        let path = HgPath::new(b"nested/path/to/b");
+        let base = HgPath::new(b"nested/path/");
+        assert_eq!(Some(HgPath::new(b"to/b")), path.relative_to(base));
+
+        let path = HgPath::new(b"ends/with/dir/");
+        let base = HgPath::new(b"ends/");
+        assert_eq!(Some(HgPath::new(b"with/dir/")), path.relative_to(base));
+    }
 }