Parse file mode properly from git and pass through

CHANGELOG.md

@@ -11,6 +11,9 @@ Added support for Smali.
 Fixed an issue with paths not showing the containing directory when
 using difftastic with `git difftool`.
 
+Difftastic now correctly reports file permission changes when called
+from `git diff`.
+
 Fixed an issue with the experimental JSON display mode where it
 ignored `--skip-unchanged`.
 

src/main.rs

@@ -44,6 +44,7 @@ extern crate log;
 use display::style::print_warning;
 use log::info;
 use mimalloc::MiMalloc;
+use options::FilePermissions;
 use options::USAGE;
 
 use crate::conflicts::apply_conflict_markers;
@@ -70,7 +71,6 @@ use crate::parse::syntax;
 #[global_allocator]
 static GLOBAL: MiMalloc = MiMalloc;
 
-use std::fs::Permissions;
 use std::path::Path;
 use std::{env, thread};
 
@@ -219,6 +219,8 @@ fn main() {
             language_overrides,
             lhs_path,
             rhs_path,
+            lhs_permissions,
+            rhs_permissions,
             display_path,
             renamed,
         } => {
@@ -298,6 +300,8 @@ fn main() {
                         renamed,
                         &lhs_path,
                         &rhs_path,
+                        lhs_permissions.as_ref(),
+                        rhs_permissions.as_ref(),
                         &display_options,
                         &diff_options,
                         false,
@@ -328,68 +332,14 @@ fn main() {
     };
 }
 
-#[cfg(unix)]
-fn compare_permissions(lhs: &Permissions, rhs: &Permissions) -> Option<String> {
-    use std::os::unix::fs::PermissionsExt;
-
-    let lhs_mode = lhs.mode();
-    let rhs_mode = rhs.mode();
-
-    if lhs_mode != rhs_mode {
-        let lhs_mode = format!("{:o}", lhs_mode);
-        let rhs_mode = format!("{:o}", rhs_mode);
-        Some(format!(
-            "File permissions changed from {} to {}.",
-            lhs_mode, rhs_mode
-        ))
-    } else {
-        None
-    }
-}
-
-#[cfg(windows)]
-fn compare_permissions(lhs: &Permissions, rhs: &Permissions) -> Option<String> {
-    if lhs.readonly() != rhs.readonly() {
-        Some(format!(
-            "File permissions changed from {} to {}.",
-            if lhs.readonly() {
-                "readonly"
-            } else {
-                "read-write"
-            },
-            if rhs.readonly() {
-                "readonly"
-            } else {
-                "read-write"
-            },
-        ))
-    } else {
-        None
-    }
-}
-
-#[cfg(not(any(unix, windows)))]
-fn compare_permissions(_lhs: &Permissions, _rhs: &Permissions) -> Option<String> {
-    None
-}
-
-fn describe_permissions_change(lhs_path: &FileArgument, rhs_path: &FileArgument) -> Option<String> {
-    match (lhs_path, rhs_path) {
-        (FileArgument::NamedPath(lhs_path), FileArgument::NamedPath(rhs_path)) => {
-            let lhs_metadata = std::fs::metadata(lhs_path).ok()?;
-            let rhs_metadata = std::fs::metadata(rhs_path).ok()?;
-            compare_permissions(&lhs_metadata.permissions(), &rhs_metadata.permissions())
-        }
-        _ => None,
-    }
-}
-
 /// Print a diff between two files.
 fn diff_file(
     display_path: &str,
     renamed: Option<String>,
     lhs_path: &FileArgument,
     rhs_path: &FileArgument,
+    lhs_permissions: Option<&FilePermissions>,
+    rhs_permissions: Option<&FilePermissions>,
     display_options: &DisplayOptions,
     diff_options: &DiffOptions,
     missing_as_empty: bool,
@@ -420,12 +370,19 @@ fn diff_file(
     }
 
     let mut extra_info = renamed;
-    if let Some(permissions_change) = describe_permissions_change(lhs_path, rhs_path) {
-        if let Some(extra_info) = &mut extra_info {
-            extra_info.push('\n');
-            extra_info.push_str(&permissions_change);
-        } else {
-            extra_info = Some(permissions_change);
+    if let (Some(lhs_perms), Some(rhs_perms)) = (lhs_permissions, rhs_permissions) {
+        if lhs_perms != rhs_perms {
+            let msg = format!(
+                "File permissions changed from {} to {}.",
+                lhs_perms, rhs_perms
+            );
+
+            if let Some(extra_info) = &mut extra_info {
+                extra_info.push('\n');
+                extra_info.push_str(&msg);
+            } else {
+                extra_info = Some(msg);
+            }
         }
     }
 
@@ -780,14 +737,16 @@ fn diff_directories<'a>(
     paths.into_par_iter().map(move |rel_path| {
         info!("Relative path is {:?} inside {:?}", rel_path, lhs_dir);
 
-        let lhs_path = Path::new(lhs_dir).join(&rel_path);
-        let rhs_path = Path::new(rhs_dir).join(&rel_path);
+        let lhs_path = FileArgument::NamedPath(Path::new(lhs_dir).join(&rel_path));
+        let rhs_path = FileArgument::NamedPath(Path::new(rhs_dir).join(&rel_path));
 
         diff_file(
             &rel_path.display().to_string(),
             None,
-            &FileArgument::NamedPath(lhs_path),
-            &FileArgument::NamedPath(rhs_path),
+            &lhs_path,
+            &rhs_path,
+            lhs_path.permissions().as_ref(),
+            rhs_path.permissions().as_ref(),
             &display_options,
             &diff_options,
             true,

src/options.rs

@@ -1,6 +1,11 @@
 //! CLI option parsing.
 
-use std::{env, ffi::OsStr, fmt::Display, path::Path, path::PathBuf};
+use std::{
+    env,
+    ffi::OsStr,
+    fmt::Display,
+    path::{Path, PathBuf},
+};
 
 use clap::{crate_authors, crate_description, Arg, Command};
 use const_format::formatcp;
@@ -312,6 +317,61 @@ pub(crate) enum FileArgument {
     DevNull,
 }
 
+impl FileArgument {
+    pub(crate) fn permissions(&self) -> Option<FilePermissions> {
+        match self {
+            FileArgument::NamedPath(path) => {
+                let metadata = std::fs::metadata(path).ok()?;
+                Some(metadata.permissions().into())
+            }
+            FileArgument::Stdin => None,
+            FileArgument::DevNull => None,
+        }
+    }
+}
+
+/// A cross-platform representation of file permissions.
+///
+/// We're only interested in whether two permissions are the same, and
+/// how to display them, so internally this is just a human-friendly
+/// string.
+#[derive(Debug, Eq, PartialEq)]
+pub(crate) struct FilePermissions(String);
+
+impl Display for FilePermissions {
+    fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+        write!(f, "{}", self.0)
+    }
+}
+
+impl From<String> for FilePermissions {
+    fn from(s: String) -> Self {
+        Self(s)
+    }
+}
+
+impl From<&OsStr> for FilePermissions {
+    fn from(s: &OsStr) -> Self {
+        Self(s.to_string_lossy().into_owned())
+    }
+}
+
+impl From<std::fs::Permissions> for FilePermissions {
+    fn from(perms: std::fs::Permissions) -> Self {
+        if cfg!(unix) {
+            use std::os::unix::fs::PermissionsExt;
+            Self(format!("{:o}", perms.mode()))
+        } else {
+            let s = if perms.readonly() {
+                "readonly"
+            } else {
+                "read-write"
+            };
+            Self(s.to_owned())
+        }
+    }
+}
+
 fn try_canonicalize(path: &Path) -> PathBuf {
     path.canonicalize().unwrap_or_else(|_| path.into())
 }
@@ -378,6 +438,8 @@ pub(crate) enum Mode {
         /// The path where we can read the RHS file. This is often a
         /// temporary file generated by source control.
         rhs_path: FileArgument,
+        lhs_permissions: Option<FilePermissions>,
+        rhs_permissions: Option<FilePermissions>,
         /// The path that we show to the user.
         display_path: String,
         /// If this file has been renamed, a description of the change.
@@ -660,23 +722,38 @@ pub(crate) fn parse_args() -> Mode {
     info!("CLI arguments: {:?}", args);
 
     // TODO: document these different ways of calling difftastic.
-    let (display_path, lhs_path, rhs_path, renamed) = match &args[..] {
+    let (display_path, lhs_path, rhs_path, lhs_permissions, rhs_permissions, renamed) = match &args
+        [..]
+    {
         [lhs_path, rhs_path] => {
             let lhs_arg = FileArgument::from_cli_argument(lhs_path);
             let rhs_arg = FileArgument::from_cli_argument(rhs_path);
             let display_path = build_display_path(&lhs_arg, &rhs_arg);
-            (display_path, lhs_arg, rhs_arg, None)
+
+            let lhs_permissions = lhs_arg.permissions();
+            let rhs_permissions = rhs_arg.permissions();
+
+            (
+                display_path,
+                lhs_arg,
+                rhs_arg,
+                lhs_permissions,
+                rhs_permissions,
+                None,
+            )
         }
-        [display_path, lhs_tmp_file, _lhs_hash, _lhs_mode, rhs_tmp_file, _rhs_hash, _rhs_mode] => {
+        [display_path, lhs_tmp_file, _lhs_hash, lhs_mode, rhs_tmp_file, _rhs_hash, rhs_mode] => {
             // https://git-scm.com/docs/git#Documentation/git.txt-codeGITEXTERNALDIFFcode
             (
                 display_path.to_string_lossy().to_string(),
                 FileArgument::from_path_argument(lhs_tmp_file),
                 FileArgument::from_path_argument(rhs_tmp_file),
+                Some((*lhs_mode).into()),
+                Some((*rhs_mode).into()),
                 None,
             )
         }
-        [old_name, lhs_tmp_file, _lhs_hash, _lhs_mode, rhs_tmp_file, _rhs_hash, _rhs_mode, new_name, _similarity] =>
+        [old_name, lhs_tmp_file, _lhs_hash, lhs_mode, rhs_tmp_file, _rhs_hash, rhs_mode, new_name, _similarity] =>
         {
             // Rename file.
             // TODO: where does git document these 9 arguments?
@@ -689,6 +766,8 @@ pub(crate) fn parse_args() -> Mode {
                 new_name,
                 FileArgument::from_path_argument(lhs_tmp_file),
                 FileArgument::from_path_argument(rhs_tmp_file),
+                Some((*lhs_mode).into()),
+                Some((*rhs_mode).into()),
                 Some(renamed),
             )
         }
@@ -749,6 +828,8 @@ pub(crate) fn parse_args() -> Mode {
         language_overrides,
         lhs_path,
         rhs_path,
+        lhs_permissions,
+        rhs_permissions,
         display_path,
         renamed,
     }