Handle -L with broken symlink

[meain]

As of now, lsd does not display the file when using -L on a broken link.

Expected behavior

This is from gnu ls.

$ gls ~/.config/calcurse-work -Ll
gls: cannot access '/Users/meain/.config/calcurse-work/hooks': No such file or directory
total 8
-rw-r--r-- 1 meain staff    0 Jun  5  2020 apts
drwxr-xr-x 4 meain staff  128 Jun  5  2020 caldav
-rw-r--r-- 1 meain staff  892 Jun  5  2020 conf
l????????? ? ?     ?        ?            ? hooks
-rw-r--r-- 1 meain staff 1205 Jun  5  2020 keys
drwx------ 2 meain staff   64 Jun  5  2020 notes
-rw-r--r-- 1 meain staff    0 Jun  5  2020 todo

Actual behavior

The hooks directory is completely skipped from the list of files.

$ lsd ~/.config/calcurse-work -Ll
lsd: /Users/meain/.config/calcurse-work/hooks: No such file or directory (os error 2)
.rw-r--r--   0B 6 months ago apts
drwxr-xr-x 128B 6 months ago caldav
.rw-r--r-- 892B 6 months ago conf
.rw-r--r-- 1.2K 6 months ago keys
drwx------  64B 6 months ago notes
.rw-r--r--   0B 6 months ago todo

[r3dArch]

Hello, I would like to try to fix this bug. Are there any particular guidelines I should adhere to?

[meain]

Nothing major as such, as long as the code passes fmt and clippy and have tests, we should be good for the most part.

[zwpaper]

@r3dArch here is where the file metadata read https://github.com/Peltoche/lsd/blob/master/src/core.rs#L103-L131, it would be a good starting point if you were new to rust and did not know where to start

[r3dArch]

Thank you for the responses. I have made progress, but I have a few questions now. Below is an example of the current output of a broken symlink with the changes I've made, and then the output using the GNU ls tool.

lsd -lL
lrwxrwxrwx user user  9 B Tue Oct  4 18:40:10 2022  test_link

The metadata above is from the symlink, which is incorrect as the dereference option was used.

ls -lL
l????????? ? ?    ?     ?            ? test_link

I think matching the formatting of GNU ls would require changes in a lot of the structs found in the Meta struct in https://github.com/Peltoche/lsd/blob/master/src/meta/mod.rs as the render() function for each struct would need to return question marks for the unknown values. Looking at the Permissions struct as an example, it seems that there would need to be some way to indicate that the permissions are unknown and have the render() function return question marks based on that. Considering how large of change this may be, do you have any thoughts on how best to implement this? I think using an enum could be a decent way to represent the variants of Known and Unknown. I have made this change to the Permissions struct as an example.

#[derive(Debug, PartialEq, Eq, Copy, Clone)]
pub enum Permissions {
    Unknown,
    Known(KnownPermissions),
}

impl Permissions {
    pub fn render(&self, colors: &Colors, flags: &Flags) -> ColoredString {
        match self {
            Self::Unknown => {
                let res = (0..9)
                    .into_iter()
                    .map(|_| colors.colorize('?', &Elem::NoAccess))
                    .into_iter()
                    .fold(String::with_capacity(160), |mut acc, x| {
                        acc.push_str(&x.to_string());
                        acc
                    });

                ColoredString::new(Colors::default_style(), res)
            }
            Self::Known(permissions) => permissions.render(colors, flags),
        }
    }

    pub fn is_executable(&self) -> bool {
        match self {
            Self::Unknown => false,
            Self::Known(permissions) => permissions.is_executable(),
        }
    }

    pub fn get_setuid(&self) -> bool {
        match self {
            Self::Unknown => false,
            Self::Known(permissions) => permissions.setuid,
        }
    }
}

impl From<&Metadata> for Permissions {
    #[cfg(unix)]
    fn from(meta: &Metadata) -> Self {
        use std::os::unix::fs::PermissionsExt;

        let bits = meta.permissions().mode();
        let has_bit = |bit| bits & bit == bit;

        Self::Known(KnownPermissions {
            user_read: has_bit(modes::USER_READ),
            user_write: has_bit(modes::USER_WRITE),
            user_execute: has_bit(modes::USER_EXECUTE),

            group_read: has_bit(modes::GROUP_READ),
            group_write: has_bit(modes::GROUP_WRITE),
            group_execute: has_bit(modes::GROUP_EXECUTE),

            other_read: has_bit(modes::OTHER_READ),
            other_write: has_bit(modes::OTHER_WRITE),
            other_execute: has_bit(modes::OTHER_EXECUTE),

            sticky: has_bit(modes::STICKY),
            setgid: has_bit(modes::SETGID),
            setuid: has_bit(modes::SETUID),
        })
    }

    #[cfg(windows)]
    fn from(_: &Metadata) -> Self {
        panic!("Cannot get permissions from metadata on Windows")
    }
}

The Permissions struct was renamed to KnownPermissions.

Here is the output now; the other information is still from the symlink, but the question marks are correct.

lsd -lL
l????????? user user  9 B Tue Oct  4 18:40:10 2022  test_link

Here is the output of a file to show that permissions are still working correctly in other cases.

lsd -lL
.rw-r--r-- user user  0 B Tue Oct  4 18:56:56 2022  file

[r3dArch]

@meain I was just wondering if you have any thoughts on my previous comment, thank you.

[meain]

I have not had time to look in depth into it, but how about we make each item in Meta optional? Would that be a bigger change? We can have it render ? if none(expcpt for permissions which would be ????????), but pass to respective renders if it actually has a content.

[r3dArch]

Thank you @meain. I will look into that soon and let you know. If you are busy there is no rush to respond, I just wasn't sure if you had seen my comment.

[r3dArch]

Here is the change I have made to the Meta struct.

--- a/src/meta/mod.rs
+++ b/src/meta/mod.rs
@@ -36,17 +36,17 @@ use std::path::{Component, Path, PathBuf};
 pub struct Meta {
     pub name: Name,
     pub path: PathBuf,
-    pub permissions: Permissions,
-    pub date: Date,
-    pub owner: Owner,
+    pub permissions: Option<Permissions>,
+    pub date: Option<Date>,
+    pub owner: Option<Owner>,
     pub file_type: FileType,
-    pub size: Size,
+    pub size: Option<Size>,
     pub symlink: SymLink,
     pub indicator: Indicator,
-    pub inode: INode,
-    pub links: Links,
+    pub inode: Option<INode>,
+    pub links: Option<Links>,
     pub content: Option<Vec<Meta>>,
-    pub access_control: AccessControl,
+    pub access_control: Option<AccessControl>,
 }

I then modified the relevant code to handle the change to being an Option. Here are the changes in /src/display.rs:

--- a/src/display.rs
+++ b/src/display.rs
@@ -289,6 +289,8 @@ fn get_output(
     tree: (usize, &str),
 ) -> Vec<String> {
     let mut strings: Vec<String> = Vec::new();
+    let colorize_missing = |string: &str| colors.colorize(string, &Elem::NoAccess);
+
     for (i, block) in flags.blocks.0.iter().enumerate() {
         let mut block_vec = if Layout::Tree == flags.layout && tree.0 == i {
             vec![colors.colorize(tree.1, &Elem::TreeEdge)]
@@ -297,28 +299,58 @@ fn get_output(
         };
 
         match block {
-            Block::INode => block_vec.push(meta.inode.render(colors)),
-            Block::Links => block_vec.push(meta.links.render(colors)),
+            Block::INode => block_vec.push(match &meta.inode {
+                Some(inode) => inode.render(colors),
+                None => colorize_missing("?"),
+            }),
+            Block::Links => block_vec.push(match &meta.links {
+                Some(links) => links.render(colors),
+                None => colorize_missing("?"),
+            }),
             Block::Permission => {
                 block_vec.extend([
                     meta.file_type.render(colors),
-                    meta.permissions.render(colors, flags),
-                    meta.access_control.render_method(colors),
+                    match meta.permissions {
+                        Some(permissions) => permissions.render(colors, flags),
+                        None => colorize_missing("?????????"),
+                    },
+                    match &meta.access_control {
+                        Some(access_control) => access_control.render_method(colors),
+                        None => colorize_missing(""),
+                    },
                 ]);
             }
-            Block::User => block_vec.push(meta.owner.render_user(colors)),
-            Block::Group => block_vec.push(meta.owner.render_group(colors)),
-            Block::Context => block_vec.push(meta.access_control.render_context(colors)),
+            Block::User => block_vec.push(match &meta.owner {
+                Some(owner) => owner.render_user(colors),
+                None => colorize_missing("?"),
+            }),
+            Block::Group => block_vec.push(match &meta.owner {
+                Some(owner) => owner.render_group(colors),
+                None => colorize_missing("?"),
+            }),
+            Block::Context => block_vec.push(match &meta.access_control {
+                Some(access_control) => access_control.render_context(colors),
+                None => colorize_missing("?"),
+            }),
             Block::Size => {
                 let pad = if Layout::Tree == flags.layout && 0 == tree.0 && 0 == i {
                     None
                 } else {
                     Some(padding_rules[&Block::SizeValue])
                 };
-                block_vec.push(meta.size.render(colors, flags, pad))
+                block_vec.push(match &meta.size {
+                    Some(size) => size.render(colors, flags, pad),
+                    None => colorize_missing("?"),
+                })
             }
-            Block::SizeValue => block_vec.push(meta.size.render_value(colors, flags)),
-            Block::Date => block_vec.push(meta.date.render(colors, flags)),
+            Block::SizeValue => block_vec.push(match &meta.size {
+                Some(size) => size.render_value(colors, flags),
+                None => colorize_missing("?"),
+            }),
+            Block::Date => block_vec.push(match &meta.date {
+                Some(date) => date.render(colors, flags),
+                None => colorize_missing("?"),
+            }),
             Block::Name => {
                 block_vec.extend([
                     meta.name.render(
@@ -377,7 +409,10 @@ fn detect_size_lengths(metas: &[Meta], flags: &Flags) -> usize {
     let mut max_value_length: usize = 0;
 
     for meta in metas {
-        let value_len = meta.size.value_string(flags).len();
+        let value_len = match &meta.size {
+            Some(size) => size.value_string(flags).len(),
+            None => 0,
+        };

Here is a sample output running lsd -lL --blocks permission,user,group,context,size,date,name,inode,links on a broken link.

l????????? ?    ?    ? ?    ?                        broken_link                                                                           ?  ?

I do have a few questions though. First is about sorting by size. For now unknown sizes are sorted to the top, here is the change in src/size.rs:

--- a/src/sort.rs
+++ b/src/sort.rs
@@ -48,7 +48,12 @@ fn with_dirs_first(a: &Meta, b: &Meta) -> Ordering {
 }
 
 fn by_size(a: &Meta, b: &Meta) -> Ordering {
-    b.size.get_bytes().cmp(&a.size.get_bytes())
+    match (&a.size, &b.size) {
+        (Some(a_size), Some(b_size)) => b_size.get_bytes().cmp(&a_size.get_bytes()),
+        (Some(_), None) => Ordering::Greater,
+        (None, Some(_)) => Ordering::Less,
+        (None, None) => Ordering::Equal,
+    }
 }

Would you prefer they be sorted to the bottom? The GNU ls tool seems to sort based on the size of the symlink, which I think is a bad solution (also more difficult to do). The other question is about displaying an error for the missing file. The GNU ls tool and lsd both output an error message like lsd: ./broken_link: No such file or directory (os error 2)., but with the changes I've made the from_path() in src/meta/mod.rs no longer returns an error since that is the function that generates the results. Here is the relevant change:

@@ -217,11 +228,11 @@ impl Meta {
                         symlink_meta = Some(m);
                     }
                 }
-                Err(e) => {
+                Err(_) => {
                     // This case, it is definitely a symlink or
                     // path.symlink_metadata would have errored out
                     if dereference {
-                        return Err(e);
+                        broken_link = true;
                     }
                 }
             }

The broken_link variable is used to set the Option types to None, but now since no error is returned lsd does not display any kind of no such file error. One option would be to print an error when it is encountered since the results aren't printed until later.

Err(e) => {
    // This case, it is definitely a symlink or
    // path.symlink_metadata would have errored out
    if dereference {
        broken_link = true;
        eprintln!("lsd: {}: {}\n", path.to_str().unwrap_or(""), e);
    }
}

Which looks like this when run:

lsd: ./broken_link: No such file or directory (os error 2)

l????????? ?    ?    ? ?    ?                        broken_link                                                                           ?  ?

This seems a bit hacky, but it is simple. Another idea would be to change the return type so it can include a result and an error/warning together. What are your thoughts?

[meain]

@r3dArch Do you mind opening a draft PR for this change? It would be much easier to have this discussion on a PR where we can point to specific code segments and track comments.

[r3dArch]

@meain I created a pull request here #754.

[meain]

Closed by #754