Better ls implementation #50
Conversation
There was a problem hiding this comment.
Thanks for giving ls a much-needed facelift, it looks great! I've never seen permission colorization before, it's a really cool idea :)
I suggested a few changes, nothing major. I think that we would ideally hide file sizes, permission colorization, and human readable / raw sizes behind option flags, but there's no need to worry about that in this PR for now. Thanks again!
programs/coreutils/ls.cpp
Outdated
| args.add_named(colorize, std::nullopt, "color", "Colorize the output."); | ||
| args.add_named(show_all, "a", "all", "Show entries starting with \".\"."); | ||
| args.add_positional(g_dir_name, false, "DIR", "The directory to list."); | ||
| args.add_named(g_no_color, "n", "no-color", "Do not g_colorize the output."); |
There was a problem hiding this comment.
| args.add_named(g_no_color, "n", "no-color", "Do not g_colorize the output."); | |
| args.add_named(g_no_color, "n", "no-color", "Do not colorize the output."); |
Accidental find+replace mishap :)
libraries/libduck/DirectoryEntry.cpp
Outdated
| } | ||
| s += '-'; | ||
| } | ||
| return s; | ||
| } | ||
|
|
||
| static std::string entry_permissions_string(mode_t mode, bool colorize) { | ||
| std::string s; | ||
| s += entry_permission_string(mode, S_IRUSR, colorize ? R_FORMAT : nullptr, 'r'); | ||
| s += entry_permission_string(mode, S_IWUSR, colorize ? W_FORMAT : nullptr, 'w'); | ||
| s += entry_permission_string(mode, S_IXUSR, colorize ? X_FORMAT : nullptr, 'x'); | ||
| s += entry_permission_string(mode, S_IRGRP, colorize ? R_FORMAT : nullptr, 'r'); | ||
| s += entry_permission_string(mode, S_IWGRP, colorize ? W_FORMAT : nullptr, 'w'); | ||
| s += entry_permission_string(mode, S_IXGRP, colorize ? X_FORMAT : nullptr, 'x'); | ||
| s += entry_permission_string(mode, S_IROTH, colorize ? R_FORMAT : nullptr, 'r'); | ||
| s += entry_permission_string(mode, S_IWOTH, colorize ? W_FORMAT : nullptr, 'w'); | ||
| s += entry_permission_string(mode, S_IXOTH, colorize ? X_FORMAT : nullptr, 'x'); | ||
| return s; | ||
| } | ||
|
|
||
| static std::string entry_fmt_size(Duck::DirectoryEntry::Type d_type, off_t size) { | ||
| std::stringstream ss; | ||
| if(d_type == Duck::DirectoryEntry::REGULAR) { | ||
| char mul = ' '; | ||
| if(size >= 1024 * 1024 * 1024) { | ||
| mul = 'G'; | ||
| size /= 1024 * 1024 * 1024; | ||
| } else if(size >= 1024 * 1024) { | ||
| mul = 'M'; | ||
| size /= 1024 * 1024; | ||
| } else if(size >= 1024) { | ||
| mul = 'K'; | ||
| size /= 1024; | ||
| } | ||
| ss << std::right << std::setw(4) << size << mul; | ||
| } else { | ||
| ss << std::right << std::setw(4) << '-' << ' '; | ||
| } | ||
| return ss.str(); | ||
| } | ||
|
|
||
| static std::string entry_name(std::string const& name, Duck::DirectoryEntry::Type d_type, bool colorize) { | ||
| std::string s; | ||
| if(colorize) { | ||
| switch(d_type) { | ||
| case DirectoryEntry::UNKNOWN: | ||
| break; | ||
| case DirectoryEntry::REGULAR: | ||
| s += REG_FORMAT; | ||
| break; | ||
| case DirectoryEntry::DIRECTORY: | ||
| s += DIR_FORMAT; | ||
| break; | ||
| case DirectoryEntry::CHARDEVICE: | ||
| s += CHR_FORMAT; | ||
| break; | ||
| case DirectoryEntry::BLOCKDEVICE: | ||
| s += BLK_FORMAT; | ||
| break; | ||
| case DirectoryEntry::FIFO: | ||
| s += FIFO_FORMAT; | ||
| break; | ||
| case DirectoryEntry::SOCKET: | ||
| s += SOCK_FORMAT; | ||
| break; | ||
| case DirectoryEntry::SYMLINK: | ||
| s += LNK_FORMAT; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| s += name; | ||
|
|
||
| if(colorize) { | ||
| s += RESET_FORMAT; | ||
| } | ||
| return s; | ||
| } | ||
|
|
||
| DirectoryEntry::DirectoryEntry(const Path& parent_path, const struct dirent* entry) : | ||
| m_path(parent_path / entry->d_name), m_inode(entry->d_ino), m_type((Type) entry->d_type), | ||
| m_name(entry->d_name) { | ||
| struct stat st; | ||
| stat(m_path.string().c_str(), &st); | ||
| m_size = st.st_size; | ||
| m_mode = st.st_mode; | ||
| } | ||
|
|
||
| std::string DirectoryEntry::as_fmt_string(bool colorize) const { | ||
| std::stringstream ss; | ||
| ss << entry_type_char(m_type) | ||
| << ' ' | ||
| << entry_permissions_string(m_mode, colorize) | ||
| << ' ' | ||
| << entry_fmt_size(m_type, m_size) | ||
| << ' ' | ||
| << entry_name(m_name, m_type, colorize); | ||
| return ss.str(); | ||
| } |
There was a problem hiding this comment.
Ideally, I think this sort of logic dealing with formatting directory entries with colors and such should stay in ls.cpp, since I don't see it being super useful elsewhere for now. If necessary in the future, we could extract it to another utility class, but I don't think it belongs in DirectoryEntry for now.
libraries/libduck/Path.cpp
Outdated
| std::sort(entries.begin(), entries.end(), [](auto const& lhs, auto const& rhs) { | ||
| return lhs.name() < rhs.name(); | ||
| }); | ||
| std::sort(entries.begin(), entries.end(), [](auto const& lhs, auto const& rhs) { | ||
| return lhs.type() > rhs.type(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
I don't know if we should be sorting directory entries by default - this could incur unnecessary performance costs when iterating over directories where sorting isn't important. I think we should probably extract this logic to ls.cpp for now.
What we could do is override operator< for DirectoryEntry so that it's easier to just call std::sort on it without writing a custom comparison function.
libraries/libduck/DirectoryEntry.cpp
Outdated
| std::stringstream ss; | ||
| if(d_type == Duck::DirectoryEntry::REGULAR) { | ||
| char mul = ' '; | ||
| if(size >= 1024 * 1024 * 1024) { | ||
| mul = 'G'; | ||
| size /= 1024 * 1024 * 1024; | ||
| } else if(size >= 1024 * 1024) { | ||
| mul = 'M'; | ||
| size /= 1024 * 1024; | ||
| } else if(size >= 1024) { | ||
| mul = 'K'; | ||
| size /= 1024; | ||
| } | ||
| ss << std::right << std::setw(4) << size << mul; | ||
| } else { | ||
| ss << std::right << std::setw(4) << '-' << ' '; | ||
| } | ||
| return ss.str(); | ||
| } |
There was a problem hiding this comment.
There's actually logic for this sort of thing in Memory::Amount in libsys, although that is technically for memory and not storage sizes (not that it makes a ton of difference). We can leave this for now, but I think later I'll make a general utility class for formatting sizes like this
|
I moved the logic from |
|
I personally use exa as |
|
Looks good, thank you! Merged :) |
No description provided.