( )⚙ D8872 rhg: make output of `files` relative to the current directory and the root

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D8872

rhg: make output of `files` relative to the current directory and the root
ClosedPublic

Authored by acezar on Aug 3 2020, 10:26 AM.

Details

Reviewers
Alphare
marmoute
pulkit
Group Reviewers
hg-reviewers
Commits
rHG1b3197047f5c: rhg: make output of `files` relative to the current directory and the root
Summary

This matches the behavior of hg files.
The util is added in hg-core instead of rhg because this operation could
be useful for other external tools. (this was definitely not prompted by rust
issue #50784, I swear)

Diff Detail

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

Event Timeline

acezar created this revision.Aug 3 2020, 10:26 AM
Alphare accepted this revision.Aug 3 2020, 11:25 AM
Alphare added a subscriber: Alphare.

To reviewers: this was my changeset, I'm not sure how authorship is attributed but I... accept the revision of course.

acezar updated this revision to Diff 22255.Aug 4 2020, 4:59 AM
indygreg added a subscriber: indygreg.Aug 8 2020, 4:19 PM
indygreg added inline comments.
rust/hg-core/src/utils/files.rs
274

It feels weird to me that this returns [u8] instead of an HgPath[Buf]. What's your justification for this?

Alphare added inline comments.Aug 12 2020, 6:01 AM
rust/hg-core/src/utils/files.rs
274

An HgPath[Buf] is meant to be the canonical internal representation of a Mercurial path. There are a few places already where this "rule" is violated, but I am planning on refactoring them, and I can start by not adding a new instance of that issue.

acezar updated this revision to Diff 22403.Aug 14 2020, 5:26 AM
Alphare added a comment.Aug 18 2020, 5:41 AM

@indygreg Does my justification seem reasonable? Could we get both patches accepted?

marmoute accepted this revision.Sep 11 2020, 4:42 AM
marmoute added a subscriber: marmoute.

This looks fine.

rust/hg-core/src/operations/list_tracked_files.rs
64

Doesn't all operation that need a repository needs to find the repository root just to be able to open it anyway ? or am I missing something?

marmoute added inline comments.Sep 11 2020, 4:47 AM
rust/hg-core/src/utils/files.rs
274

By canonical, you mean "relative to the repository root" right ? And that is why a "relative to current working copy" path cannot fit in that. That seems right.

Using Vec<u8> seems fine since this is a intended for display only.

pulkit accepted this revision.Sep 11 2020, 4:58 AM
This revision is now accepted and ready to land.Sep 11 2020, 4:58 AM

Revision Contents
Changeset List

Diff 22237

rust/hg-core/src/operations/list_tracked_files.rs

use super::find_root; use super::find_root;
use crate::dirstate::parsers::parse_dirstate; use crate::dirstate::parsers::parse_dirstate;
use crate::utils::hg_path::HgPath; use crate::utils::hg_path::HgPath;
use crate::{DirstateParseError, EntryState}; use crate::{DirstateParseError, EntryState};
use rayon::prelude::*; use rayon::prelude::*;
use std::convert::From; use std::convert::From;
use std::fmt; use std::fmt;
use std::fs; use std::fs;
use std::io; use std::io;
use std::path::PathBuf; use std::path::{Path, PathBuf};
/// Kind of error encoutered by ListTrackedFiles /// Kind of error encoutered by ListTrackedFiles
#[derive(Debug)] #[derive(Debug)]
pub enum ListTrackedFilesErrorKind { pub enum ListTrackedFilesErrorKind {
ParseError(DirstateParseError), ParseError(DirstateParseError),
} }
/// A ListTrackedFiles error /// A ListTrackedFiles error
} }
/// Load the tracked files data from disk /// Load the tracked files data from disk
pub fn load(&self) -> Result<ListDirstateTrackedFiles, io::Error> { pub fn load(&self) -> Result<ListDirstateTrackedFiles, io::Error> {
let dirstate = &self.root.join(".hg/dirstate"); let dirstate = &self.root.join(".hg/dirstate");
let content = fs::read(&dirstate)?; let content = fs::read(&dirstate)?;
Ok(ListDirstateTrackedFiles { content }) Ok(ListDirstateTrackedFiles { content })
} }
/// Returns the repository root directory
/// TODO I think this is a crutch that creates a dependency that should not
/// be there. Operations that need the root of the repository should get
/// it themselves, probably in a lazy fashion. But this would make the
/// current series even larger, so this is simplified for now.
pub fn get_root(&self) -> &Path {
&self.root
}
marmouteUnsubmitted
Not Done

Doesn't all operation that need a repository needs to find the repository root just to be able to open it anyway ? or am I missing something?

marmoute: Doesn't all operation that need a repository needs to find the repository root just to be able…
} }
/// List files under Mercurial control in the working directory /// List files under Mercurial control in the working directory
/// by reading the dirstate /// by reading the dirstate
pub struct ListDirstateTrackedFiles { pub struct ListDirstateTrackedFiles {
content: Vec<u8>, content: Vec<u8>,
} }

rust/hg-core/src/utils/files.rs

use crate::utils::{ use crate::utils::{
hg_path::{path_to_hg_path_buf, HgPath, HgPathBuf, HgPathError}, hg_path::{path_to_hg_path_buf, HgPath, HgPathBuf, HgPathError},
path_auditor::PathAuditor, path_auditor::PathAuditor,
replace_slice, replace_slice,
}; };
use lazy_static::lazy_static; use lazy_static::lazy_static;
use same_file::is_same_file; use same_file::is_same_file;
use std::borrow::ToOwned; use std::borrow::{Cow, ToOwned};
use std::fs::Metadata; use std::fs::Metadata;
use std::iter::FusedIterator; use std::iter::FusedIterator;
use std::ops::Deref; use std::ops::Deref;
use std::path::{Path, PathBuf}; use std::path::{Path, PathBuf};
pub fn get_path_from_bytes(bytes: &[u8]) -> &Path { pub fn get_path_from_bytes(bytes: &[u8]) -> &Path {
let os_str; let os_str;
#[cfg(unix)] #[cfg(unix)]
// Bubble up the responsibility to Python for now // Bubble up the responsibility to Python for now
Err(HgPathError::NotUnderRoot { Err(HgPathError::NotUnderRoot {
path: original_name.to_owned(), path: original_name.to_owned(),
root: root.to_owned(), root: root.to_owned(),
}) })
} }
} }
/// Returns the representation of the path relative to the current working
/// directory for display purposes.
///
/// `cwd` is a `HgPath`, so it is considered relative to the root directory
/// of the repository.
///
/// # Examples
///
/// ```
/// use hg::utils::hg_path::HgPath;
/// use hg::utils::files::relativize_path;
/// use std::borrow::Cow;
///
/// let file = HgPath::new(b"nested/file");
/// let cwd = HgPath::new(b"");
/// assert_eq!(relativize_path(file, cwd), Cow::Borrowed(b"nested/file"));
///
/// let cwd = HgPath::new(b"nested");
/// assert_eq!(relativize_path(file, cwd), Cow::Borrowed(b"file"));
///
/// let cwd = HgPath::new(b"other");
/// assert_eq!(relativize_path(file, cwd), Cow::Borrowed(b"../nested/file"));
/// ```
pub fn relativize_path(path: &HgPath, cwd: impl AsRef<HgPath>) -> Cow<[u8]> {
indygregUnsubmitted
Not Done

It feels weird to me that this returns [u8] instead of an HgPath[Buf]. What's your justification for this?

indygreg: It feels weird to me that this returns `[u8]` instead of an `HgPath[Buf]`. What's your…
AlphareUnsubmitted
Not Done

An HgPath[Buf] is meant to be the canonical internal representation of a Mercurial path. There are a few places already where this "rule" is violated, but I am planning on refactoring them, and I can start by not adding a new instance of that issue.

Alphare: An `HgPath[Buf]` is meant to be the canonical internal representation of a Mercurial path.
marmouteUnsubmitted
Not Done

By canonical, you mean "relative to the repository root" right ? And that is why a "relative to current working copy" path cannot fit in that. That seems right.

Using Vec<u8> seems fine since this is a intended for display only.

marmoute: By canonical, you mean "relative to the repository root" right ? And that is why a "relative to…
if cwd.as_ref().is_empty() {
Cow::Borrowed(path.as_bytes())
} else {
let mut res: Vec<u8> = Vec::new();
let mut path_iter = path.as_bytes().split(|b| *b == b'/').peekable();
let mut cwd_iter =
cwd.as_ref().as_bytes().split(|b| *b == b'/').peekable();
loop {
match (path_iter.peek(), cwd_iter.peek()) {
(Some(a), Some(b)) if a == b => (),
_ => break,
}
path_iter.next();
cwd_iter.next();
}
let mut need_sep = false;
for _ in cwd_iter {
if need_sep {
res.extend(b"/")
} else {
need_sep = true
};
res.extend(b"..");
}
for c in path_iter {
if need_sep {
res.extend(b"/")
} else {
need_sep = true
};
res.extend(c);
}
Cow::Owned(res)
}
}
#[cfg(test)] #[cfg(test)]
mod tests { mod tests {
use super::*; use super::*;
use pretty_assertions::assert_eq; use pretty_assertions::assert_eq;
#[test] #[test]
fn find_dirs_some() { fn find_dirs_some() {
let mut dirs = super::find_dirs(HgPath::new(b"foo/bar/baz")); let mut dirs = super::find_dirs(HgPath::new(b"foo/bar/baz"));

rust/rhg/src/commands/files.rs

use crate::commands::Command; use crate::commands::Command;
use crate::error::{CommandError, CommandErrorKind}; use crate::error::{CommandError, CommandErrorKind};
use crate::ui::Ui; use crate::ui::Ui;
use hg::operations::{ListTrackedFiles, ListTrackedFilesErrorKind}; use hg::operations::{ListTrackedFiles, ListTrackedFilesErrorKind};
use hg::utils::files::{get_bytes_from_path, relativize_path};
use hg::utils::hg_path::HgPathBuf;
pub const HELP_TEXT: &str = " pub const HELP_TEXT: &str = "
List tracked files. List tracked files.
Returns 0 on success. Returns 0 on success.
"; ";
pub struct FilesCommand<'a> { pub struct FilesCommand<'a> {
ListTrackedFilesErrorKind::ParseError(_) => { ListTrackedFilesErrorKind::ParseError(_) => {
CommandErrorKind::Abort(Some( CommandErrorKind::Abort(Some(
// TODO find a better error message // TODO find a better error message
b"abort: parse error\n".to_vec(), b"abort: parse error\n".to_vec(),
)) ))
} }
})?; })?;
let cwd = std::env::current_dir()
.or_else(|e| Err(CommandErrorKind::CurrentDirNotFound(e)))?;
let rooted_cwd = cwd
.strip_prefix(operation_builder.get_root())
.expect("cwd was already checked within the repository");
let rooted_cwd = HgPathBuf::from(get_bytes_from_path(rooted_cwd));
let mut stdout = self.ui.stdout_buffer(); let mut stdout = self.ui.stdout_buffer();
for file in files { for file in files {
stdout.write_all(file.as_bytes())?; stdout.write_all(relativize_path(file, &rooted_cwd).as_ref())?;
stdout.write_all(b"\n")?; stdout.write_all(b"\n")?;
} }
stdout.flush()?; stdout.flush()?;
Ok(()) Ok(())
} }
} }