Merge pull request #1164 from tmccombs/owner-without-panic

Fix panic when using --owner

3 files changed, 71 insertions, 17 deletions

diff --git a/src/filter/owner.rs b/src/filter/owner.rs
index b69c2ef..842c711 100644
--- a/src/filter/owner.rs
+++ b/src/filter/owner.rs
@@ -1,13 +1,13 @@
 use anyhow::{anyhow, Result};
 use std::fs;
 
-#[derive(Clone, Copy, Debug, PartialEq)]
+#[derive(Clone, Copy, Debug, PartialEq, Eq)]
 pub struct OwnerFilter {
     uid: Check<u32>,
     gid: Check<u32>,
 }
 
-#[derive(Clone, Copy, Debug, PartialEq)]
+#[derive(Clone, Copy, Debug, PartialEq, Eq)]
 enum Check<T> {
     Equal(T),
     NotEq(T),
@@ -15,10 +15,15 @@ enum Check<T> {
 }
 
 impl OwnerFilter {
+    const IGNORE: Self = OwnerFilter {
+        uid: Check::Ignore,
+        gid: Check::Ignore,
+    };
+
     /// Parses an owner constraint
     /// Returns an error if the string is invalid
     /// Returns Ok(None) when string is acceptable but a noop (such as "" or ":")
-    pub fn from_string(input: &str) -> Result<Option<Self>> {
+    pub fn from_string(input: &str) -> Result<Self> {
         let mut it = input.split(':');
         let (fst, snd) = (it.next(), it.next());
 
@@ -42,10 +47,15 @@ impl OwnerFilter {
                 .ok_or_else(|| anyhow!("'{}' is not a recognized group name", s))
         })?;
 
-        if let (Check::Ignore, Check::Ignore) = (uid, gid) {
-            Ok(None)
+        Ok(OwnerFilter { uid, gid })
+    }
+
+    /// If self is a no-op (ignore both uid and gid) then return `None`, otherwise wrap in a `Some`
+    pub fn filter_ignore(self) -> Option<Self> {
+        if self == Self::IGNORE {
+            None
         } else {
-            Ok(Some(OwnerFilter { uid, gid }))
+            Some(self)
         }
     }
 
@@ -106,16 +116,16 @@ mod owner_parsing {
 
     use super::Check::*;
     owner_tests! {
-        empty:      ""      => Ok(None),
-        uid_only:   "5"     => Ok(Some(OwnerFilter { uid: Equal(5), gid: Ignore     })),
-        uid_gid:    "9:3"   => Ok(Some(OwnerFilter { uid: Equal(9), gid: Equal(3)   })),
-        gid_only:   ":8"    => Ok(Some(OwnerFilter { uid: Ignore,   gid: Equal(8)   })),
-        colon_only: ":"     => Ok(None),
-        trailing:   "5:"    => Ok(Some(OwnerFilter { uid: Equal(5), gid: Ignore     })),
-
-        uid_negate: "!5"    => Ok(Some(OwnerFilter { uid: NotEq(5), gid: Ignore     })),
-        both_negate:"!4:!3" => Ok(Some(OwnerFilter { uid: NotEq(4), gid: NotEq(3)   })),
-        uid_not_gid:"6:!8"  => Ok(Some(OwnerFilter { uid: Equal(6), gid: NotEq(8)   })),
+        empty:      ""      => Ok(OwnerFilter::IGNORE),
+        uid_only:   "5"     => Ok(OwnerFilter { uid: Equal(5), gid: Ignore     }),
+        uid_gid:    "9:3"   => Ok(OwnerFilter { uid: Equal(9), gid: Equal(3)   }),
+        gid_only:   ":8"    => Ok(OwnerFilter { uid: Ignore,   gid: Equal(8)   }),
+        colon_only: ":"     => Ok(OwnerFilter::IGNORE),
+        trailing:   "5:"    => Ok(OwnerFilter { uid: Equal(5), gid: Ignore     }),
+
+        uid_negate: "!5"    => Ok(OwnerFilter { uid: NotEq(5), gid: Ignore     }),
+        both_negate:"!4:!3" => Ok(OwnerFilter { uid: NotEq(4), gid: NotEq(3)   }),
+        uid_not_gid:"6:!8"  => Ok(OwnerFilter { uid: Equal(6), gid: NotEq(8)   }),
 
         more_colons:"3:5:"  => Err(_),
         only_colons:"::"    => Err(_),
diff --git a/src/main.rs b/src/main.rs
index 9fa4edc..08a9881 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -184,7 +184,7 @@ fn construct_config(mut opts: Opts, pattern_regex: &str) -> Result<Config> {
     let size_limits = std::mem::take(&mut opts.size);
     let time_constraints = extract_time_constraints(&opts)?;
     #[cfg(unix)]
-    let owner_constraint: Option<OwnerFilter> = opts.owner;
+    let owner_constraint: Option<OwnerFilter> = opts.owner.and_then(OwnerFilter::filter_ignore);
 
     #[cfg(windows)]
     let ansi_colors_support =
diff --git a/tests/tests.rs b/tests/tests.rs
index 1cc561f..eb24472 100644
--- a/tests/tests.rs
+++ b/tests/tests.rs
@@ -1919,6 +1919,50 @@ fn test_modified_absolute() {
     );
 }
 
+#[cfg(unix)]
+#[test]
+fn test_owner_ignore_all() {
+    let te = TestEnv::new(DEFAULT_DIRS, DEFAULT_FILES);
+    te.assert_output(&["--owner", ":", "a.foo"], "a.foo");
+    te.assert_output(&["--owner", "", "a.foo"], "a.foo");
+}
+
+#[cfg(unix)]
+#[test]
+fn test_owner_current_user() {
+    let te = TestEnv::new(DEFAULT_DIRS, DEFAULT_FILES);
+    let uid = users::get_current_uid();
+    te.assert_output(&["--owner", &uid.to_string(), "a.foo"], "a.foo");
+    if let Some(username) = users::get_current_username().map(|u| u.into_string().unwrap()) {
+        te.assert_output(&["--owner", &username, "a.foo"], "a.foo");
+    }
+}
+
+#[cfg(unix)]
+#[test]
+fn test_owner_current_group() {
+    let te = TestEnv::new(DEFAULT_DIRS, DEFAULT_FILES);
+    let gid = users::get_current_gid();
+    te.assert_output(&["--owner", &format!(":{}", gid), "a.foo"], "a.foo");
+    if let Some(groupname) = users::get_current_groupname().map(|u| u.into_string().unwrap()) {
+        te.assert_output(&["--owner", &format!(":{}", groupname), "a.foo"], "a.foo");
+    }
+}
+
+#[cfg(target_os = "linux")]
+#[test]
+fn test_owner_root() {
+    // This test assumes the current user isn't root
+    if users::get_current_uid() == 0 || users::get_current_gid() == 0 {
+        return;
+    }
+    let te = TestEnv::new(DEFAULT_DIRS, DEFAULT_FILES);
+    te.assert_output(&["--owner", "root", "a.foo"], "");
+    te.assert_output(&["--owner", "0", "a.foo"], "");
+    te.assert_output(&["--owner", ":root", "a.foo"], "");
+    te.assert_output(&["--owner", ":0", "a.foo"], "");
+}
+
 #[test]
 fn test_custom_path_separator() {
     let te = TestEnv::new(DEFAULT_DIRS, DEFAULT_FILES);