This is an archive of the discontinued Mercurial Phabricator instance.

Differential D10913

rhg: refactor relative path resolution in utility fn
AbandonedPublic

Authored by pulkit on Jun 25 2021, 1:47 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Summary

The same code is being used in rhg files and rhg status and I expect it to
be used at more places in upcoming future. So let's have a utility function for
that.

rhg files still doesn't use this codepath but will start using in upcoming
patches.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

pulkit created this revision.Jun 25 2021, 1:47 PM
SimonSapin added a subscriber: SimonSapin.Jun 29 2021, 2:17 PM
SimonSapin added inline comments.
rust/rhg/src/commands/status.rs
293

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.

rust/rhg/src/utils/path_utils.rs
17

In order not to force callers to allocate a Vec of of Cows 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.

18

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.

21–23

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?

pulkit planned changes to this revision.Oct 5 2021, 7:51 AM
pulkit abandoned this revision.Oct 5 2021, 8:49 AM

Superseeded by D11613.

Revision Contents
Changeset List

Diff 28700

rust/rhg/src/commands/status.rs

// status.rs // status.rs
// //
// Copyright 2020, Georges Racinet <georges.racinets@octobus.net> // Copyright 2020, Georges Racinet <georges.racinets@octobus.net>
// //
// This software may be used and distributed according to the terms of the // This software may be used and distributed according to the terms of the
// GNU General Public License version 2 or any later version. // GNU General Public License version 2 or any later version.
use crate::error::CommandError; use crate::error::CommandError;
use crate::ui::{Ui, UiError}; use crate::ui::{Ui, UiError};
use crate::utils::path_utils::relativize_paths;
use clap::{Arg, SubCommand}; use clap::{Arg, SubCommand};
use hg; use hg;
use hg::config::Config; use hg::config::Config;
use hg::dirstate_tree::dirstate_map::DirstateMap; use hg::dirstate_tree::dirstate_map::DirstateMap;
use hg::errors::HgResultExt; use hg::errors::HgResultExt;
use hg::errors::IoResultExt; use hg::errors::IoResultExt;
use hg::matchers::AlwaysMatcher; use hg::matchers::AlwaysMatcher;
use hg::operations::cat; use hg::operations::cat;
use hg::repo::Repo; use hg::repo::Repo;
use hg::revlog::node::Node; use hg::revlog::node::Node;
use hg::utils::current_dir;
use hg::utils::files::{get_bytes_from_path, relativize_path};
use hg::utils::hg_path::HgPathBuf;
use hg::utils::hg_path::{hg_path_to_os_string, HgPath}; use hg::utils::hg_path::{hg_path_to_os_string, HgPath};
use hg::StatusError; use hg::StatusError;
use hg::{HgPathCow, StatusOptions}; use hg::{HgPathCow, StatusOptions};
use log::{info, warn}; use log::{info, warn};
use std::borrow::Cow;
use std::convert::TryInto; use std::convert::TryInto;
use std::fs; use std::fs;
use std::io::BufReader; use std::io::BufReader;
use std::io::Read; use std::io::Read;
pub const HELP_TEXT: &str = " pub const HELP_TEXT: &str = "
Show changed files in the working directory Show changed files in the working directory
) -> Result<(), CommandError> { ) -> Result<(), CommandError> {
paths.sort_unstable(); paths.sort_unstable();
let mut relative: bool = let mut relative: bool =
config.get_bool(b"ui", b"relative-paths").unwrap_or(false); config.get_bool(b"ui", b"relative-paths").unwrap_or(false);
relative = config relative = config
.get_bool(b"commands", b"status.relative") .get_bool(b"commands", b"status.relative")
.unwrap_or(relative); .unwrap_or(relative);
if relative && !ui.plain() { if relative && !ui.plain() {
let cwd = current_dir()?; let print_path = |path: Cow<[u8]>| -> Result<(), UiError> {
SimonSapinUnsubmitted
Not Done

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.

SimonSapin: Since this now only used once it can be moved into the `relativize_paths` call without…
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> {
ui.write_stdout( ui.write_stdout(
&[ &[status_prefix, b" ", path.as_ref(), b"\n"].concat(),
status_prefix,
b" ",
relativize_path(path, &cwd_hgpath).as_ref(),
b"\n",
]
.concat(),
) )
}; };
relativize_paths(repo, paths, &print_path)?;
for file in paths {
if outside_repo {
let file = working_directory_hgpath.join(file);
print_path(&file)?;
} else {
print_path(file)?;
}
}
} else { } else {
for path in paths { for path in paths {
// Same TODO as in commands::root // Same TODO as in commands::root
let bytes: &[u8] = path.as_bytes(); let bytes: &[u8] = path.as_bytes();
// TODO optim, probably lots of unneeded copies here, especially // TODO optim, probably lots of unneeded copies here, especially
// if out stream is buffered // if out stream is buffered
ui.write_stdout(&[status_prefix, b" ", bytes, b"\n"].concat())?; ui.write_stdout(&[status_prefix, b" ", bytes, b"\n"].concat())?;
} }

rust/rhg/src/main.rs

use hg::utils::SliceExt; use hg::utils::SliceExt;
use std::ffi::OsString; use std::ffi::OsString;
use std::path::PathBuf; use std::path::PathBuf;
use std::process::Command; use std::process::Command;
mod blackbox; mod blackbox;
mod error; mod error;
mod ui; mod ui;
pub mod utils {
pub mod path_utils;
}
use error::CommandError; use error::CommandError;
fn main_with_result( fn main_with_result(
process_start_time: &blackbox::ProcessStartTime, process_start_time: &blackbox::ProcessStartTime,
ui: &ui::Ui, ui: &ui::Ui,
repo: Result<&Repo, &NoRepoInCwdError>, repo: Result<&Repo, &NoRepoInCwdError>,
config: &Config, config: &Config,
) -> Result<(), CommandError> { ) -> Result<(), CommandError> {

rust/rhg/src/utils/path_utils.rs

  • This file was added.
// path utils module
//
// This software may be used and distributed according to the terms of the
// GNU General Public License version 2 or any later version.
use crate::error::CommandError;
use crate::ui::UiError;
use hg::repo::Repo;
use hg::utils::current_dir;
use hg::utils::files::{get_bytes_from_path, relativize_path};
use hg::utils::hg_path::HgPathBuf;
use hg::HgPathCow;
use std::borrow::Cow;
pub fn relativize_paths(
repo: &Repo,
paths: &[HgPathCow],
SimonSapinUnsubmitted
Not Done

In order not to force callers to allocate a Vec of of Cows 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.

SimonSapin: In order not to force callers to allocate a `Vec` of of `Cow`s just to call this, the parameter…
callback: &dyn Fn(Cow<[u8]>) -> Result<(), UiError>,
SimonSapinUnsubmitted
Not Done

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.

SimonSapin: There is no need for dynamic dispatch here. Replacing `&dyn Fn` with `impl Fn` will let the…
) -> Result<(), CommandError> {
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 =
SimonSapinUnsubmitted
Not Done

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?

SimonSapin: This is from code that I originally wrote, but even though in mercurial we talk about a…
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));
}
for file in paths {
if outside_repo {
let file = working_directory_hgpath.join(file);
callback(relativize_path(&file, &cwd_hgpath))?;
} else {
callback(relativize_path(file, &cwd_hgpath))?;
}
}
Ok(())
}