diff --git a/src/build.rs b/src/build.rs index e1b75bd43b..6aaa6991dd 100644 --- a/src/build.rs +++ b/src/build.rs @@ -503,7 +503,7 @@ impl<'cb> CheckoutBuilder<'cb> { /// If no paths are specified, then all files are checked out. Otherwise /// only these specified paths are checked out. pub fn path(&mut self, path: T) -> &mut CheckoutBuilder<'cb> { - let path = path.into_c_string().unwrap(); + let path = util::cstring_to_repo_path(path).unwrap(); self.path_ptrs.push(path.as_ptr()); self.paths.push(path); self diff --git a/src/index.rs b/src/index.rs index 797e6d8815..6c27db143b 100644 --- a/src/index.rs +++ b/src/index.rs @@ -291,7 +291,7 @@ impl Index { T: IntoCString, I: IntoIterator, { - let (_a, _b, raw_strarray) = crate::util::iter2cstrs(pathspecs)?; + let (_a, _b, raw_strarray) = crate::util::iter2cstrs_paths(pathspecs)?; let ptr = cb.as_mut(); let callback = ptr .as_ref() @@ -469,7 +469,7 @@ impl Index { T: IntoCString, I: IntoIterator, { - let (_a, _b, raw_strarray) = crate::util::iter2cstrs(pathspecs)?; + let (_a, _b, raw_strarray) = crate::util::iter2cstrs_paths(pathspecs)?; let ptr = cb.as_mut(); let callback = ptr .as_ref() @@ -507,7 +507,7 @@ impl Index { T: IntoCString, I: IntoIterator, { - let (_a, _b, raw_strarray) = crate::util::iter2cstrs(pathspecs)?; + let (_a, _b, raw_strarray) = crate::util::iter2cstrs_paths(pathspecs)?; let ptr = cb.as_mut(); let callback = ptr .as_ref() diff --git a/src/lib.rs b/src/lib.rs index bc7bf1b6a5..bd176a2ee6 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -132,12 +132,12 @@ pub use crate::util::IntoCString; // Create a convinience method on bitflag struct which checks the given flag macro_rules! is_bit_set { - ($name:ident, $flag:expr) => ( + ($name:ident, $flag:expr) => { #[allow(missing_docs)] pub fn $name(&self) -> bool { self.intersects($flag) } - ) + }; } /// An enumeration of possible errors that can happen when working with a git diff --git a/src/oid.rs b/src/oid.rs index 59f2a7ffa2..207ecd6840 100644 --- a/src/oid.rs +++ b/src/oid.rs @@ -91,6 +91,7 @@ impl Oid { pub fn hash_file>(kind: ObjectType, path: P) -> Result { crate::init(); + // Normal file path OK (does not need Windows conversion). let rpath = path.as_ref().into_c_string()?; let mut out = raw::git_oid { diff --git a/src/pathspec.rs b/src/pathspec.rs index 3b23c2fc85..6d842a55eb 100644 --- a/src/pathspec.rs +++ b/src/pathspec.rs @@ -45,7 +45,7 @@ impl Pathspec { T: IntoCString, I: IntoIterator, { - let (_a, _b, arr) = crate::util::iter2cstrs(specs)?; + let (_a, _b, arr) = crate::util::iter2cstrs_paths(specs)?; unsafe { let mut ret = ptr::null_mut(); try_call!(raw::git_pathspec_new(&mut ret, &arr)); diff --git a/src/repo.rs b/src/repo.rs index b4daefa567..65fe377ead 100644 --- a/src/repo.rs +++ b/src/repo.rs @@ -11,7 +11,7 @@ use crate::build::{CheckoutBuilder, RepoBuilder}; use crate::oid_array::OidArray; use crate::stash::{stash_cb, StashApplyOptions, StashCbData}; use crate::string_array::StringArray; -use crate::util::{self, Binding}; +use crate::util::{self, path_to_repo_path, Binding}; use crate::CherrypickOptions; use crate::{ init, raw, AttrCheckFlags, Buf, Error, Object, Remote, RepositoryOpenFlags, RepositoryState, @@ -673,7 +673,7 @@ impl Repository { T: IntoCString, I: IntoIterator, { - let (_a, _b, mut arr) = crate::util::iter2cstrs(paths)?; + let (_a, _b, mut arr) = crate::util::iter2cstrs_paths(paths)?; let target = target.map(|t| t.raw()); unsafe { try_call!(raw::git_reset_default(self.raw, target, &mut arr)); @@ -878,13 +878,7 @@ impl Repository { /// through looking for the path that you are interested in. pub fn status_file(&self, path: &Path) -> Result { let mut ret = 0 as c_uint; - let path = if cfg!(windows) { - // `git_status_file` does not work with windows path separator - // so we convert \ to / - std::ffi::CString::new(path.to_string_lossy().replace('\\', "/"))? - } else { - path.into_c_string()? - }; + let path = path_to_repo_path(path)?; unsafe { try_call!(raw::git_status_file(&mut ret, self.raw, path)); } @@ -2528,13 +2522,7 @@ impl Repository { /// Test if the ignore rules apply to a given path. pub fn is_path_ignored>(&self, path: P) -> Result { - let path = if cfg!(windows) { - // `git_ignore_path_is_ignored` dose not work with windows path separator - // so we convert \ to / - std::ffi::CString::new(path.as_ref().to_string_lossy().replace('\\', "/"))? - } else { - path.as_ref().into_c_string()? - }; + let path = path_to_repo_path(path.as_ref())?; let mut ignored: c_int = 0; unsafe { try_call!(raw::git_ignore_path_is_ignored( @@ -3283,18 +3271,18 @@ mod tests { fn smoke_is_path_ignored() { let (_td, repo) = graph_repo_init(); - assert!(!repo.is_path_ignored(Path::new("/foo")).unwrap()); + assert!(!repo.is_path_ignored(Path::new("foo")).unwrap()); let _ = repo.add_ignore_rule("/foo"); - assert!(repo.is_path_ignored(Path::new("/foo")).unwrap()); + assert!(repo.is_path_ignored(Path::new("foo")).unwrap()); if cfg!(windows) { - assert!(repo.is_path_ignored(Path::new("\\foo\\thing")).unwrap()); + assert!(repo.is_path_ignored(Path::new("foo\\thing")).unwrap()); } let _ = repo.clear_ignore_rules(); - assert!(!repo.is_path_ignored(Path::new("/foo")).unwrap()); + assert!(!repo.is_path_ignored(Path::new("foo")).unwrap()); if cfg!(windows) { - assert!(!repo.is_path_ignored(Path::new("\\foo\\thing")).unwrap()); + assert!(!repo.is_path_ignored(Path::new("foo\\thing")).unwrap()); } } diff --git a/src/status.rs b/src/status.rs index 25b57327af..92858e48a5 100644 --- a/src/status.rs +++ b/src/status.rs @@ -5,7 +5,7 @@ use std::mem; use std::ops::Range; use std::str; -use crate::util::Binding; +use crate::util::{self, Binding}; use crate::{raw, DiffDelta, IntoCString, Repository, Status}; /// Options that can be provided to `repo.statuses()` to control how the status @@ -98,7 +98,7 @@ impl StatusOptions { /// path to match. If this is not called, then there will be no patterns to /// match and the entire directory will be used. pub fn pathspec(&mut self, pathspec: T) -> &mut StatusOptions { - let s = pathspec.into_c_string().unwrap(); + let s = util::cstring_to_repo_path(pathspec).unwrap(); self.ptrs.push(s.as_ptr()); self.pathspec.push(s); self diff --git a/src/tree.rs b/src/tree.rs index 436a8f50a2..2f644833dc 100644 --- a/src/tree.rs +++ b/src/tree.rs @@ -8,7 +8,7 @@ use std::path::Path; use std::ptr; use std::str; -use crate::util::{c_cmp_to_ordering, Binding, IntoCString}; +use crate::util::{c_cmp_to_ordering, path_to_repo_path, Binding}; use crate::{panic, raw, Error, Object, ObjectType, Oid, Repository}; /// A structure to represent a git [tree][1] @@ -179,7 +179,7 @@ impl<'repo> Tree<'repo> { /// Retrieve a tree entry contained in a tree or in any of its subtrees, /// given its relative path. pub fn get_path(&self, path: &Path) -> Result, Error> { - let path = path.into_c_string()?; + let path = path_to_repo_path(path)?; let mut ret = ptr::null_mut(); unsafe { try_call!(raw::git_tree_entry_bypath(&mut ret, &*self.raw(), path)); diff --git a/src/util.rs b/src/util.rs index d2402fcc71..a31e63bce2 100644 --- a/src/util.rs +++ b/src/util.rs @@ -2,7 +2,7 @@ use libc::{c_char, c_int, size_t}; use std::cmp::Ordering; use std::ffi::{CString, OsStr, OsString}; use std::iter::IntoIterator; -use std::path::{Path, PathBuf}; +use std::path::{Component, Path, PathBuf}; use crate::{raw, Error}; @@ -41,6 +41,29 @@ pub trait Binding: Sized { } } +/// Converts an iterator of repo paths into a git2-compatible array of cstrings. +/// +/// Only use this for repo-relative paths or pathspecs. +/// +/// See `iter2cstrs` for more details. +pub fn iter2cstrs_paths( + iter: I, +) -> Result<(Vec, Vec<*const c_char>, raw::git_strarray), Error> +where + T: IntoCString, + I: IntoIterator, +{ + let cstrs = iter + .into_iter() + .map(|i| fixup_windows_path(i.into_c_string()?)) + .collect::, _>>()?; + iter2cstrs(cstrs) +} + +/// Converts an iterator of things into a git array of c-strings. +/// +/// Returns a tuple `(cstrings, pointers, git_strarray)`. The first two values +/// should not be dropped before `git_strarray`. pub fn iter2cstrs( iter: I, ) -> Result<(Vec, Vec<*const c_char>, raw::git_strarray), Error> @@ -136,8 +159,7 @@ impl IntoCString for OsString { match self.to_str() { Some(s) => s.into_c_string(), None => Err(Error::from_str( - "only valid unicode paths are accepted \ - on windows", + "only valid unicode paths are accepted on windows", )), } } @@ -172,3 +194,149 @@ pub fn c_cmp_to_ordering(cmp: c_int) -> Ordering { _ => Ordering::Greater, } } + +/// Converts a path to a CString that is usable by the libgit2 API. +/// +/// Checks if it is a relative path. +/// +/// On Windows, this also requires the path to be valid unicode, and translates +/// back slashes to forward slashes. +pub fn path_to_repo_path(path: &Path) -> Result { + macro_rules! err { + ($msg:literal, $path:expr) => { + return Err(Error::from_str(&format!($msg, $path.display()))); + }; + } + match path.components().next() { + None => return Err(Error::from_str("repo path should not be empty")), + Some(Component::Prefix(_)) => err!( + "repo path `{}` should be relative, not a windows prefix", + path + ), + Some(Component::RootDir) => err!("repo path `{}` should be relative", path), + Some(Component::CurDir) => err!("repo path `{}` should not start with `.`", path), + Some(Component::ParentDir) => err!("repo path `{}` should not start with `..`", path), + Some(Component::Normal(_)) => {} + } + #[cfg(windows)] + { + match path.to_str() { + None => { + return Err(Error::from_str( + "only valid unicode paths are accepted on windows", + )) + } + Some(s) => return fixup_windows_path(s), + } + } + #[cfg(not(windows))] + { + path.into_c_string() + } +} + +pub fn cstring_to_repo_path(path: T) -> Result { + fixup_windows_path(path.into_c_string()?) +} + +#[cfg(windows)] +fn fixup_windows_path>>(path: P) -> Result { + let mut bytes: Vec = path.into(); + for i in 0..bytes.len() { + if bytes[i] == b'\\' { + bytes[i] = b'/'; + } + } + Ok(CString::new(bytes)?) +} + +#[cfg(not(windows))] +fn fixup_windows_path(path: CString) -> Result { + Ok(path) +} + +#[cfg(test)] +mod tests { + use super::*; + + macro_rules! assert_err { + ($path:expr, $msg:expr) => { + match path_to_repo_path(Path::new($path)) { + Ok(_) => panic!("expected `{}` to err", $path), + Err(e) => assert_eq!(e.message(), $msg), + } + }; + } + + macro_rules! assert_repo_path_ok { + ($path:expr) => { + assert_repo_path_ok!($path, $path) + }; + ($path:expr, $expect:expr) => { + assert_eq!( + path_to_repo_path(Path::new($path)), + Ok(CString::new($expect).unwrap()) + ); + }; + } + + #[test] + #[cfg(windows)] + fn path_to_repo_path_translate() { + assert_repo_path_ok!("foo"); + assert_repo_path_ok!("foo/bar"); + assert_repo_path_ok!(r"foo\bar", "foo/bar"); + assert_repo_path_ok!(r"foo\bar\", "foo/bar/"); + } + + #[test] + fn path_to_repo_path_no_weird() { + assert_err!("", "repo path should not be empty"); + assert_err!("./foo", "repo path `./foo` should not start with `.`"); + assert_err!("../foo", "repo path `../foo` should not start with `..`"); + } + + #[test] + #[cfg(not(windows))] + fn path_to_repo_path_no_absolute() { + assert_err!("/", "repo path `/` should be relative"); + assert_repo_path_ok!("foo/bar"); + } + + #[test] + #[cfg(windows)] + fn path_to_repo_path_no_absolute() { + assert_err!( + r"c:", + r"repo path `c:` should be relative, not a windows prefix" + ); + assert_err!( + r"c:\", + r"repo path `c:\` should be relative, not a windows prefix" + ); + assert_err!( + r"c:temp", + r"repo path `c:temp` should be relative, not a windows prefix" + ); + assert_err!( + r"\\?\UNC\a\b\c", + r"repo path `\\?\UNC\a\b\c` should be relative, not a windows prefix" + ); + assert_err!( + r"\\?\c:\foo", + r"repo path `\\?\c:\foo` should be relative, not a windows prefix" + ); + assert_err!( + r"\\.\COM42", + r"repo path `\\.\COM42` should be relative, not a windows prefix" + ); + assert_err!( + r"\\a\b", + r"repo path `\\a\b` should be relative, not a windows prefix" + ); + assert_err!(r"\", r"repo path `\` should be relative"); + assert_err!(r"/", r"repo path `/` should be relative"); + assert_err!(r"\foo", r"repo path `\foo` should be relative"); + assert_err!(r"/foo", r"repo path `/foo` should be relative"); + } +}