Add size option (-s) in the `ls` command #1102

nnh12 · 2024-08-14T02:30:54Z

This pull request is to add the ls -s option in the Theseus operating environment. The ls -s is a standard linux command. It shows the sizes of files and directories in blocks, which is useful for getting a quick overview of file sizes. Below are some sample test cases:

/extra_files: ls -s
    --         wasm
    --         test_files
    1159    README.md
/extra_files: ls test_files -s
    --         zork_dat
    --         text
/extra_files/test_files/text: ls -s
    422     the_rise_of_skywalker.txt
    492     the_phanton_menace.txt
    477     the_last_jedi.txt
    499     the_force_awakens.txt
    486     the_empire_strikes_back.txt
    482     revenge_of_the_sith.txt
    462     return_of_the_jedi.txt
    490     attack_of_the_clones.txt
    503     a_new_hope.txt

Default ls command still works:

/extra_files/test_files: ls 
zork_dat
text

* Lists the size of each file (in bytes) when `ls -s` is invoked

kevinaboos

Thanks for the PR! I left a few comments.

My top-level comment is that the default output of size should be in units of bytes, not in disk sectors. Kindly make that change and address my other comments, and then I can merge it in. Thanks!

applications/ls/src/lib.rs

kevinaboos · 2024-08-15T19:33:31Z

applications/ls/src/lib.rs

@@ -49,7 +58,12 @@ pub fn main(args: Vec<String>) -> isize {
    // Navigate to the path specified by first argument
    match path.get(&curr_wd) {
        Some(FileOrDir::Dir(dir)) => {
-            print_children(&dir);
+            if size_option {
+                print_children(&dir, true);


Same comment here, no need to duplicate code, just pass in size_option directly as an argument.

kernel/fs_node/src/lib.rs

kevinaboos · 2024-08-15T19:36:53Z

kernel/fs_node/src/lib.rs

@@ -190,4 +197,15 @@ impl FileOrDir {
            FileOrDir::Dir(_) => true,
        }
    }
+
+    /// Returns the size of the file in bytes, or directory disk space.
+    pub fn get_file_size(file_or_dir: &FileOrDir) -> usize {


This is redundant, see the existing len() method for files (part of the KnownLength trait). See my other comment about handling sizes for directories.

* Add '--' option in place for directory size * Remove remove redundant Kernel function and perform operation in `ls` appliciation instead

nnh12 · 2024-08-17T21:59:42Z

I have created the PR to the requested changes
The file size is returned from the ls -s in bytes (since it calls the len() function from KnownLength
The directory size is returned as -- as you suggested

kevinaboos

thanks! i left only one small comment about code duplication that you missed.

For future reference, kindly don't force push commits to your PR branch, as it removes all of my review comments (notice that they're all gone now). This makes it hard to track whether all of the comments have been addressed.

kevinaboos · 2024-08-19T17:21:00Z

applications/ls/src/lib.rs

+    if size_option && matches.free.is_empty() {
+        print_children(&curr_wd, size_option);
+        return 0;
+    }


this duplicated code block is unnecessary, you can remove it as it's redundant with the block below it

Got it I will remove that

nnh12 · 2024-08-19T18:01:23Z

Got it thank you!
Just to confirm, so should I just make a new branch and open a new PR addressing the feedback?

kevinaboos · 2024-08-19T18:17:04Z

Got it thank you! Just to confirm, so should I just make a new branch and open a new branch addressing the feedback?

Nope, all you have to do is make another change, commit it to this same branch, and push it. Github will automatically show the new commit in this PR.

nnh12 · 2024-08-19T20:08:54Z

Got it thanks - I won't force push

nnh12 · 2024-08-19T23:07:26Z

I just removed the duplicated code block- thanks

kevinaboos

Thanks, looks great!

* Lists the size in bytes of each file when `ls -s` is invoked * Prints '--' to indicate directory sizes are not calculated yet Co-authored-by: Nathan Hsiao <nnh12@DESKTOP-EQ10QSJ> 8a1d149

Add size option (-s) in the ls command

aac805c

* Lists the size of each file (in bytes) when `ls -s` is invoked

kevinaboos requested changes Aug 15, 2024

View reviewed changes

Update directory size and remove Kernel level function call

15d16d3

* Add '--' option in place for directory size * Remove remove redundant Kernel function and perform operation in `ls` appliciation instead

nnh12 force-pushed the nathan/ls_s branch from d82d7a1 to 15d16d3 Compare August 17, 2024 21:54

kevinaboos requested changes Aug 19, 2024

View reviewed changes

Remove verbose check for size option

2ac1a5f

kevinaboos approved these changes Aug 20, 2024

View reviewed changes

kevinaboos merged commit 8a1d149 into theseus-os:theseus_main Aug 20, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add size option (-s) in the `ls` command #1102

Add size option (-s) in the `ls` command #1102

nnh12 commented Aug 14, 2024 •

edited

Loading

kevinaboos left a comment

kevinaboos Aug 15, 2024

kevinaboos Aug 15, 2024

nnh12 commented Aug 17, 2024

kevinaboos left a comment

kevinaboos Aug 19, 2024

nnh12 Aug 19, 2024

nnh12 commented Aug 19, 2024 •

edited

Loading

kevinaboos commented Aug 19, 2024

nnh12 commented Aug 19, 2024

nnh12 commented Aug 19, 2024

kevinaboos left a comment

Add size option (-s) in the ls command #1102

Add size option (-s) in the ls command #1102

Conversation

nnh12 commented Aug 14, 2024 • edited Loading

kevinaboos left a comment

Choose a reason for hiding this comment

kevinaboos Aug 15, 2024

Choose a reason for hiding this comment

kevinaboos Aug 15, 2024

Choose a reason for hiding this comment

nnh12 commented Aug 17, 2024

kevinaboos left a comment

Choose a reason for hiding this comment

kevinaboos Aug 19, 2024

Choose a reason for hiding this comment

nnh12 Aug 19, 2024

Choose a reason for hiding this comment

nnh12 commented Aug 19, 2024 • edited Loading

kevinaboos commented Aug 19, 2024

nnh12 commented Aug 19, 2024

nnh12 commented Aug 19, 2024

kevinaboos left a comment

Choose a reason for hiding this comment

Add size option (-s) in the `ls` command #1102

Add size option (-s) in the `ls` command #1102

nnh12 commented Aug 14, 2024 •

edited

Loading

nnh12 commented Aug 19, 2024 •

edited

Loading