[Changed Subscribers] D10913: rhg: refactor relative path resolution in utility fn

[SimonSapin]

SimonSapin added inline comments.

INLINE COMMENTS

> status.rs:293
>      if relative && !ui.plain() {
> -        let cwd = current_dir()?;
> -        let working_directory = repo.working_directory_path();
> -        let working_directory = cwd.join(working_directory); // Make it absolute
> -        let working_directory_hgpath =
> -            HgPathBuf::from(get_bytes_from_path(working_directory.to_owned()));
> -        let outside_repo: bool;
> -        let cwd_hgpath: HgPathBuf;
> -
> -        if let Ok(cwd_relative_to_repo) = cwd.strip_prefix(&working_directory)
> -        {
> -            // The current directory is inside the repo, so we can work with
> -            // relative paths
> -            outside_repo = false;
> -            cwd_hgpath =
> -                HgPathBuf::from(get_bytes_from_path(cwd_relative_to_repo));
> -        } else {
> -            outside_repo = true;
> -            cwd_hgpath = HgPathBuf::from(get_bytes_from_path(cwd));
> -        }
> -
> -        let print_path = |path: &HgPath| -> Result<(), UiError> {
> +        let print_path = |path: Cow<[u8]>| -> Result<(), UiError> {
>              ui.write_stdout(

Since this now only used once it can be moved into the `relativize_paths` call without assigning to a variable. This gives "more local" information to type inference, so type annotations in the closure signature won’t be needed anymore.

> path_utils.rs:17
> +    repo: &Repo,
> +    paths: &[HgPathCow],
> +    callback: &dyn Fn(Cow<[u8]>) -> Result<(), UiError>,

In order not to force callers to allocate a `Vec` of of `Cow`s just to call this, the parameter could be made a generic iterable of generic items:

  paths: impl IntoIterator<Item = impl AsRef<HgPath>>

Then in the `for` loop below use `file.as_ref()` to go from the generic item type to `&HgPath`.

> path_utils.rs:18
> +    paths: &[HgPathCow],
> +    callback: &dyn Fn(Cow<[u8]>) -> Result<(), UiError>,
> +) -> Result<(), CommandError> {

There is no need for dynamic dispatch here. Replacing `&dyn Fn` with `impl Fn` will let the compiler inline and optimize for each specific callback.

> path_utils.rs:21-23
> +    let working_directory = repo.working_directory_path();
> +    let working_directory = cwd.join(working_directory); // Make it absolute
> +    let working_directory_hgpath =

This is from code that I originally wrote, but even though in mercurial we talk about a repository’s "working directory", this code is slightly confusing because `cwd` also means "current working directory" but is not necessarily the same directory.

Maybe rename `working_directory` to something like `repo_root`?

REPOSITORY
  rHG Mercurial

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

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

To: pulkit, #hg-reviewers
Cc: SimonSapin, mercurial-patches
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.mercurial-scm.org/pipermail/mercurial-patches/attachments/20210629/38c1b21e/attachment-0002.html>