Attempt to uniformly handle Windows-style paths. (#549)

Author: ehuss

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<T: IntoCString>(&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

src/index.rs

@@ -291,7 +291,7 @@ impl Index {
         T: IntoCString,
         I: IntoIterator<Item = T>,
     {
-        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<Item = T>,
     {
-        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<Item = T>,
     {
-        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()

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

src/oid.rs

@@ -91,6 +91,7 @@ impl Oid {
     pub fn hash_file<P: AsRef<Path>>(kind: ObjectType, path: P) -> Result<Oid, Error> {
         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 {

src/pathspec.rs

@@ -45,7 +45,7 @@ impl Pathspec {
         T: IntoCString,
         I: IntoIterator<Item = T>,
     {
-        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));

src/repo.rs

@@ -14,7 +14,7 @@ use crate::diff::{
 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,
@@ -676,7 +676,7 @@ impl Repository {
         T: IntoCString,
         I: IntoIterator<Item = T>,
     {
-        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));
@@ -881,13 +881,7 @@ impl Repository {
     /// through looking for the path that you are interested in.
     pub fn status_file(&self, path: &Path) -> Result<Status, Error> {
         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));
         }
@@ -2603,13 +2597,7 @@ impl Repository {
 
     /// Test if the ignore rules apply to a given path.
     pub fn is_path_ignored<P: AsRef<Path>>(&self, path: P) -> Result<bool, Error> {
-        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(
@@ -3358,18 +3346,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());
         }
     }
 

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<T: IntoCString>(&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

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<TreeEntry<'static>, 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));

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<T, I>(
+    iter: I,
+) -> Result<(Vec<CString>, Vec<*const c_char>, raw::git_strarray), Error>
+where
+    T: IntoCString,
+    I: IntoIterator<Item = T>,
+{
+    let cstrs = iter
+        .into_iter()
+        .map(|i| fixup_windows_path(i.into_c_string()?))
+        .collect::<Result<Vec<CString>, _>>()?;
+    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<T, I>(
     iter: I,
 ) -> Result<(Vec<CString>, 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<CString, Error> {
+    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<T: IntoCString>(path: T) -> Result<CString, Error> {
+    fixup_windows_path(path.into_c_string()?)
+}
+
+#[cfg(windows)]
+fn fixup_windows_path<P: Into<Vec<u8>>>(path: P) -> Result<CString, Error> {
+    let mut bytes: Vec<u8> = 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<CString, Error> {
+    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");
+    }
+}