From 76292ef10c6eb4c3eaaba7cf1ee39348580bc55f Mon Sep 17 00:00:00 2001
From: Reilly Wood <26268125+rgwood@users.noreply.github.com>
Date: Fri, 27 Jan 2023 15:02:38 +0100
Subject: [PATCH] Clean up `cd.rs` (#7876)

Some general cleanup of `cd.rs`; the permission checking code was a
little hard to follow. Reworded comments and variable names,
reorganized+renamed the module used for Unix file permissions.
---
 crates/nu-command/src/filesystem/cd.rs | 49 +++++++++++++-------------
 1 file changed, 24 insertions(+), 25 deletions(-)

diff --git a/crates/nu-command/src/filesystem/cd.rs b/crates/nu-command/src/filesystem/cd.rs
index 98233e821..47d3acea9 100644
--- a/crates/nu-command/src/filesystem/cd.rs
+++ b/crates/nu-command/src/filesystem/cd.rs
@@ -10,22 +10,16 @@ use nu_protocol::{
 };
 use std::path::Path;
 
-// when the file under the fold executable
+// For checking whether we have permission to cd to a directory
 #[cfg(unix)]
-mod permission_mods {
+mod file_permissions {
     pub type Mode = u32;
-    pub mod unix {
-        use super::Mode;
-        pub const USER_EXECUTE: Mode = libc::S_IXUSR as Mode;
-        pub const GROUP_EXECUTE: Mode = libc::S_IXGRP as Mode;
-        pub const OTHER_EXECUTE: Mode = libc::S_IXOTH as Mode;
-    }
+    pub const USER_EXECUTE: Mode = libc::S_IXUSR as Mode;
+    pub const GROUP_EXECUTE: Mode = libc::S_IXGRP as Mode;
+    pub const OTHER_EXECUTE: Mode = libc::S_IXOTH as Mode;
 }
 
-// use to return the message of the result of change director
-// TODO: windows, maybe should use file_attributes function in https://doc.rust-lang.org/std/os/windows/fs/trait.MetadataExt.html
-// TODO: the meaning of the result of the function can be found in https://learn.microsoft.com/en-us/windows/win32/fileio/file-attribute-constants
-// TODO: if have realize the logic on windows, remove the cfg
+// The result of checking whether we have permission to cd to a directory
 #[derive(Debug)]
 enum PermissionResult<'a> {
     PermissionOk,
@@ -169,8 +163,10 @@ impl Command for Cd {
             }
         };
 
-        let path_tointo = path.clone();
-        let path_value = Value::String { val: path, span };
+        let path_value = Value::String {
+            val: path.clone(),
+            span,
+        };
         let cwd = Value::string(cwd.to_string_lossy(), call.head);
 
         let mut shells = get_shells(engine_state, stack, cwd);
@@ -193,16 +189,16 @@ impl Command for Cd {
             stack.add_env_var("OLDPWD".into(), oldpwd)
         }
 
-        //FIXME: this only changes the current scope, but instead this environment variable
-        //should probably be a block that loads the information from the state in the overlay
-        match have_permission(&path_tointo) {
+        match have_permission(&path) {
+            //FIXME: this only changes the current scope, but instead this environment variable
+            //should probably be a block that loads the information from the state in the overlay
             PermissionResult::PermissionOk => {
                 stack.add_env_var("PWD".into(), path_value);
                 Ok(PipelineData::empty())
             }
             PermissionResult::PermissionDenied(reason) => Err(ShellError::IOError(format!(
                 "Cannot change directory to {}: {}",
-                path_tointo, reason
+                path, reason
             ))),
         }
     }
@@ -227,6 +223,9 @@ impl Command for Cd {
         ]
     }
 }
+
+// TODO: Maybe we should use file_attributes() from https://doc.rust-lang.org/std/os/windows/fs/trait.MetadataExt.html
+// More on that here: https://learn.microsoft.com/en-us/windows/win32/fileio/file-attribute-constants
 #[cfg(windows)]
 fn have_permission(dir: impl AsRef<Path>) -> PermissionResult<'static> {
     match dir.as_ref().read_dir() {
@@ -260,26 +259,26 @@ fn have_permission(dir: impl AsRef<Path>) -> PermissionResult<'static> {
                 current_user_gid == owner_group,
             ) {
                 (true, _) => {
-                    if has_bit(permission_mods::unix::USER_EXECUTE) {
+                    if has_bit(file_permissions::USER_EXECUTE) {
                         PermissionResult::PermissionOk
                     } else {
                         PermissionResult::PermissionDenied(
-                            "You are the owner but do not have the execute permission",
+                            "You are the owner but do not have execute permission",
                         )
                     }
                 }
                 (false, true) => {
-                    if has_bit(permission_mods::unix::GROUP_EXECUTE) {
+                    if has_bit(file_permissions::GROUP_EXECUTE) {
                         PermissionResult::PermissionOk
                     } else {
                         PermissionResult::PermissionDenied(
-                            "You are in the group but do not have the execute permission",
+                            "You are in the group but do not have execute permission",
                         )
                     }
                 }
                 (false, false) => {
-                    if has_bit(permission_mods::unix::OTHER_EXECUTE)
-                        || (has_bit(permission_mods::unix::GROUP_EXECUTE)
+                    if has_bit(file_permissions::OTHER_EXECUTE)
+                        || (has_bit(file_permissions::GROUP_EXECUTE)
                             && any_group(current_user_gid, owner_group))
                     {
                         PermissionResult::PermissionOk
@@ -291,7 +290,7 @@ fn have_permission(dir: impl AsRef<Path>) -> PermissionResult<'static> {
                 }
             }
         }
-        Err(_) => PermissionResult::PermissionDenied("Could not retrieve the metadata"),
+        Err(_) => PermissionResult::PermissionDenied("Could not retrieve file metadata"),
     }
 }