D7871: rust-utils: add util for canonical path

martinvonz (Martin von Zweigbergk) phabricator at mercurial-scm.org
Thu Feb 6 17:39:43 UTC 2020


This revision now requires changes to proceed.
martinvonz added inline comments.
martinvonz requested changes to this revision.

INLINE COMMENTS

> files.rs:207-216
> +    let name = if !name.is_absolute() {
> +        root.join(&cwd).join(&name)
> +    } else {
> +        name.to_owned()
> +    };
> +    let mut auditor = PathAuditor::new(&root);
> +    if name != root && name.starts_with(&root) {

Might be worth having a fast path for when `name` is not absolute (including "") and neither `name` nor `cwd` contains "../"so we don't do `let name = root.join(&cwd).join(&name)` immediately followed by `let name = name.strip_prefix(&root).unwrap()`. Especially since that seems like the common case. Or can that ever produce different results? Anyway, that can be done later.

> files.rs:220-222
> +        // Determine whether `name' is in the hierarchy at or beneath `root',
> +        // by iterating name=name.parent() until that causes no change (can't
> +        // check name == '/', because that doesn't work on windows).

Is this accurate? It looks like we break when `name.parent() == None`.

> files.rs:227-230
> +                if name.components().next().is_none() {
> +                    // `name` was actually the same as root (maybe a symlink)
> +                    return Ok("".into());
> +                }

`name.components().next()` will be `None` only if `name` is something like "/", right? But then we shouldn't return "" (a repo-relative path), we should error out.

REPOSITORY
  rHG Mercurial

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

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

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


More information about the Mercurial-devel mailing list