Patchwork [1,of,2] rust-dirstate: add helper to iterate ancestor paths

login
register
mail settings
Submitter Yuya Nishihara
Date June 30, 2019, 11:05 a.m.
Message ID <72ab74c704053b2212b8.1561892734@mimosa>
Download mbox | patch
Permalink /patch/40737/
State Accepted
Headers show

Comments

Yuya Nishihara - June 30, 2019, 11:05 a.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1561887163 -32400
#      Sun Jun 30 18:32:43 2019 +0900
# Node ID 72ab74c704053b2212b874a902574fedadad4252
# Parent  904e0da2e195d2a29977ceccdd25480233c34d7a
rust-dirstate: add helper to iterate ancestor paths

This is modeled after std::path::Path::ancestors().

find_dirs(b"") yields b"" because Mercurial's util.finddirs() works in that
way, and the test case for DirsMultiset expects such behavior.
Pulkit Goyal - July 10, 2019, 1:24 p.m.
On Sun, Jun 30, 2019 at 2:16 PM Yuya Nishihara <yuya@tcha.org> wrote:

> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1561887163 -32400
> #      Sun Jun 30 18:32:43 2019 +0900
> # Node ID 72ab74c704053b2212b874a902574fedadad4252
> # Parent  904e0da2e195d2a29977ceccdd25480233c34d7a
> rust-dirstate: add helper to iterate ancestor paths
>
> This is modeled after std::path::Path::ancestors().
>
> find_dirs(b"") yields b"" because Mercurial's util.finddirs() works in that
> way, and the test case for DirsMultiset expects such behavior.
>

Queued this one, many thanks. The next one fails to apply on tip of default.
via Mercurial-devel - July 10, 2019, 2:36 p.m.
On Sun, Jun 30, 2019, 04:17 Yuya Nishihara <yuya@tcha.org> wrote:

> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1561887163 -32400
> #      Sun Jun 30 18:32:43 2019 +0900
> # Node ID 72ab74c704053b2212b874a902574fedadad4252
> # Parent  904e0da2e195d2a29977ceccdd25480233c34d7a
> rust-dirstate: add helper to iterate ancestor paths
>
> This is modeled after std::path::Path::ancestors().
>
> find_dirs(b"") yields b"" because Mercurial's util.finddirs() works in that
> way, and the test case for DirsMultiset expects such behavior.
>

That's also how std::path::Path::ancestors() works, it seems.


> diff --git a/rust/hg-core/src/utils/files.rs b/rust/hg-core/src/utils/
> files.rs
> --- a/rust/hg-core/src/utils/files.rs
> +++ b/rust/hg-core/src/utils/files.rs
> @@ -1,3 +1,4 @@
> +use std::iter::FusedIterator;
>  use std::path::Path;
>
>  pub fn get_path_from_bytes(bytes: &[u8]) -> &Path {
> @@ -17,3 +18,66 @@ pub fn get_path_from_bytes(bytes: &[u8])
>
>      Path::new(os_str)
>  }
> +
> +/// An iterator over repository path yielding itself and its ancestors.
> +#[derive(Copy, Clone, Debug)]
> +pub struct Ancestors<'a> {
> +    next: Option<&'a [u8]>,
> +}
> +
> +impl<'a> Iterator for Ancestors<'a> {
> +    // if we had an HgPath type, this would yield &'a HgPath
> +    type Item = &'a [u8];
> +
> +    fn next(&mut self) -> Option<Self::Item> {
> +        let next = self.next;
> +        self.next = match self.next {
> +            Some(s) if s.is_empty() => None,
> +            Some(s) => {
> +                let p = s.iter().rposition(|&c| c == b'/').unwrap_or(0);
> +                Some(&s[..p])
> +            }
> +            None => None,
> +        };
> +        next
> +    }
> +}
> +
> +impl<'a> FusedIterator for Ancestors<'a> {}
> +
> +/// Returns an iterator yielding ancestor directories of the given
> repository
> +/// path.
> +///
> +/// The path is separated by '/', and must not start with '/'.
> +///
> +/// The path itself isn't included unless it is b"" (meaning the root
> +/// directory.)
> +pub fn find_dirs<'a>(path: &'a [u8]) -> Ancestors<'a> {
> +    let mut dirs = Ancestors { next: Some(path) };
> +    if !path.is_empty() {
> +        dirs.next(); // skip itself
> +    }
> +    dirs
> +}
> +
> +#[cfg(test)]
> +mod tests {
> +    #[test]
> +    fn find_dirs_some() {
> +        let mut dirs = super::find_dirs(b"foo/bar/baz");
> +        assert_eq!(dirs.next(), Some(b"foo/bar".as_ref()));
> +        assert_eq!(dirs.next(), Some(b"foo".as_ref()));
> +        assert_eq!(dirs.next(), Some(b"".as_ref()));
> +        assert_eq!(dirs.next(), None);
> +        assert_eq!(dirs.next(), None);
> +    }
> +
> +    #[test]
> +    fn find_dirs_empty() {
> +        // looks weird, but mercurial.util.finddirs(b"") yields b""
> +        let mut dirs = super::find_dirs(b"");
> +        assert_eq!(dirs.next(), Some(b"".as_ref()));
> +        assert_eq!(dirs.next(), None);
> +        assert_eq!(dirs.next(), None);
> +    }
> +}
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Yuya Nishihara - July 11, 2019, 12:18 a.m.
On Wed, 10 Jul 2019 07:36:26 -0700, Martin von Zweigbergk wrote:
> On Sun, Jun 30, 2019, 04:17 Yuya Nishihara <yuya@tcha.org> wrote:
> 
> > # HG changeset patch
> > # User Yuya Nishihara <yuya@tcha.org>
> > # Date 1561887163 -32400
> > #      Sun Jun 30 18:32:43 2019 +0900
> > # Node ID 72ab74c704053b2212b874a902574fedadad4252
> > # Parent  904e0da2e195d2a29977ceccdd25480233c34d7a
> > rust-dirstate: add helper to iterate ancestor paths
> >
> > This is modeled after std::path::Path::ancestors().
> >
> > find_dirs(b"") yields b"" because Mercurial's util.finddirs() works in that
> > way, and the test case for DirsMultiset expects such behavior.
> >
> 
> That's also how std::path::Path::ancestors() works, it seems.

finddirs(path) is basically equivalent to

  Ancestors { next: path.parent() }
  // not including the path itself.
  // note that Path::new("/").parent() returns None.

so I feel it's weird that both finddirs("foo") and finddirs("") return [''].
Raphaël Gomès - July 12, 2019, 9:18 a.m.
I've rewritten the patch to apply on default. I'll rebase both of my 
series on it.

On 7/10/19 3:24 PM, Pulkit Goyal wrote:
>
>
> On Sun, Jun 30, 2019 at 2:16 PM Yuya Nishihara <yuya@tcha.org 
> <mailto:yuya@tcha.org>> wrote:
>
>     # HG changeset patch
>     # User Yuya Nishihara <yuya@tcha.org <mailto:yuya@tcha.org>>
>     # Date 1561887163 -32400
>     #      Sun Jun 30 18:32:43 2019 +0900
>     # Node ID 72ab74c704053b2212b874a902574fedadad4252
>     # Parent  904e0da2e195d2a29977ceccdd25480233c34d7a
>     rust-dirstate: add helper to iterate ancestor paths
>
>     This is modeled after std::path::Path::ancestors().
>
>     find_dirs(b"") yields b"" because Mercurial's util.finddirs()
>     works in that
>     way, and the test case for DirsMultiset expects such behavior.
>
>
> Queued this one, many thanks. The next one fails to apply on tip of 
> default.
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Patch

diff --git a/rust/hg-core/src/utils/files.rs b/rust/hg-core/src/utils/files.rs
--- a/rust/hg-core/src/utils/files.rs
+++ b/rust/hg-core/src/utils/files.rs
@@ -1,3 +1,4 @@ 
+use std::iter::FusedIterator;
 use std::path::Path;
 
 pub fn get_path_from_bytes(bytes: &[u8]) -> &Path {
@@ -17,3 +18,66 @@  pub fn get_path_from_bytes(bytes: &[u8])
 
     Path::new(os_str)
 }
+
+/// An iterator over repository path yielding itself and its ancestors.
+#[derive(Copy, Clone, Debug)]
+pub struct Ancestors<'a> {
+    next: Option<&'a [u8]>,
+}
+
+impl<'a> Iterator for Ancestors<'a> {
+    // if we had an HgPath type, this would yield &'a HgPath
+    type Item = &'a [u8];
+
+    fn next(&mut self) -> Option<Self::Item> {
+        let next = self.next;
+        self.next = match self.next {
+            Some(s) if s.is_empty() => None,
+            Some(s) => {
+                let p = s.iter().rposition(|&c| c == b'/').unwrap_or(0);
+                Some(&s[..p])
+            }
+            None => None,
+        };
+        next
+    }
+}
+
+impl<'a> FusedIterator for Ancestors<'a> {}
+
+/// Returns an iterator yielding ancestor directories of the given repository
+/// path.
+///
+/// The path is separated by '/', and must not start with '/'.
+///
+/// The path itself isn't included unless it is b"" (meaning the root
+/// directory.)
+pub fn find_dirs<'a>(path: &'a [u8]) -> Ancestors<'a> {
+    let mut dirs = Ancestors { next: Some(path) };
+    if !path.is_empty() {
+        dirs.next(); // skip itself
+    }
+    dirs
+}
+
+#[cfg(test)]
+mod tests {
+    #[test]
+    fn find_dirs_some() {
+        let mut dirs = super::find_dirs(b"foo/bar/baz");
+        assert_eq!(dirs.next(), Some(b"foo/bar".as_ref()));
+        assert_eq!(dirs.next(), Some(b"foo".as_ref()));
+        assert_eq!(dirs.next(), Some(b"".as_ref()));
+        assert_eq!(dirs.next(), None);
+        assert_eq!(dirs.next(), None);
+    }
+
+    #[test]
+    fn find_dirs_empty() {
+        // looks weird, but mercurial.util.finddirs(b"") yields b""
+        let mut dirs = super::find_dirs(b"");
+        assert_eq!(dirs.next(), Some(b"".as_ref()));
+        assert_eq!(dirs.next(), None);
+        assert_eq!(dirs.next(), None);
+    }
+}