Adding tree command to FogOS by chansrinivas · Pull Request #85 · USF-OS/FogOS · GitHub
Skip to content

Adding tree command to FogOS#85

Open
chansrinivas wants to merge 25 commits into
USF-OS:mainfrom
chansrinivas:main
Open

Adding tree command to FogOS#85
chansrinivas wants to merge 25 commits into
USF-OS:mainfrom
chansrinivas:main

Conversation

@chansrinivas

@chansrinivas chansrinivas commented Sep 26, 2024

Copy link
Copy Markdown

Team: Riya Mathur, Suhani Arora, Chandana Srinivas

This project is a command-line tool written in C that displays the directory structure in a tree-like format. It allows users to explore directories and subdirectories with options to filter files by extension, display file sizes, and show file and directory counts. Supports recursive traversal of directories.

Features:
/tree <directory> - Display a directory tree structure for a given path (Default is . which is the root directory).
/tree . -F <file extension> -Optionally filter files by their extension.
/tree . -S - Optionally show the size of files in bytes.
/tree . -C - Optionally count and display the number of files and directories in each directory.
/tree . -L <depth> - Optionally limits the depth of the directory tree. Only shows directories and files up to the specified depth level.

You could also multiple flags:
/tree . -C -L <depth> - Shows the counts of files and limits the depth of the directory tree.
/tree . -S -F <file extension> - Shows the file sizes in bytes and filters files by their extension.

Since xv6 does not have directories by default, the updated mkfs.c will create two directories: dir1 and dir2 within xv6 for more convenient recursive testing of directories.

@andywu42

Copy link
Copy Markdown

@ajsdkty3

Copy link
Copy Markdown

I can code review this

@chansrinivas

Copy link
Copy Markdown
Author

Added in new mkfs.c for testing

@ajsdkty3

ajsdkty3 commented Oct 1, 2024

Copy link
Copy Markdown

I really like this project! The way the directory tree is displayed using graphs and symbols is very clear and intuitive. The code works impressively well for most cases, including recursive traversal with depth limits. The flags also can be combined and work together well without errors.

Suggestions for Improvement:
1.When an invalid flag is provided, the error message is unclear. For example, running /tree -Z results in:
tree: cannot open -Z
Instead, it could display an error message indicating an invalid flag.

2.The tree function opens the file descriptor multiple times when counting and printing files. To improve efficiency, it could handle both tasks in a single pass without reopening directories.

High Level Checks:
✅ PR fully resolves the task/issue
✅ Does the PR have tests?
✅ Does the PR follow project code formatting standards?
✅ Does the OS still compile and run?
✅ Is documentation included?

Code Checks:
✅ Does the code achieve its intended purpose?
❌ Were all edge cases considered?
✅ Is the code organized and maintainable?
✅ Does the code maintain the same level of performance? (no performance regressions)

@andywu42

andywu42 commented Oct 4, 2024

Copy link
Copy Markdown

Comments:
I like your project! The output is very clean and easy to understand. Good comments in code, easy to follow logic. Overall, design is tailored for readability and clean functionality which I really enjoy as a reviewer.
Suggestions for improvement:

  1. Output for parsing flags should be more informative towards the flag being processed incorrectly
    Ex: /tree -c
    Output: tree: cannot open -c
    Suggestion: “Invalid flag”
    Ex: /tree . -F
    Output: tree: cannot open -F
    Suggestion: “Expected value for -F”
  2. The parsing of values for flags -F and -L should have some informative errors when nothing or something wrong is read after these flags.
    Ex: /tree . -F hi
    Output: (Nothing)
    Suggestion: Print something like “Invalid value for -F flag. File extensions should start with ‘.’ ”
    Ex: /tree . -L hi
    Output: ./
    Suggestion: add if case to check for value, if nothing/non-int print something like “Invalid value for -L flag.”
  3. Edge case not checked: -S and -C flags used together
    The output of -S is at the end of the file, with the output of -C at the end of directories, I would expect both should be able to run at the same time? (I notice it runs -C over -S no matter what)
    Ex: /tree dir2 -S -C
    Output:
    dir2/ [0 directories, 1 files]
    Expected:
    dir2/ [0 directories, 1 files]
    └── dir2/test.txt (size: 6 bytes)
  4. Cleanup: remove test.txt in root branch (different from the one in dir2), it is not used or displayed when running make qemu (or add it to the Makefile)

I do think to improve efficiency you could potentially make some changes to how you process flags. As the earlier CR points out, runtime might be slow through the uses of recursively counting files/directories multiple times for one task, so a change I could recommend is making your status variables global. This way, you can check for them during your tree() and contains_valid_file() functions and hopefully get rid of the large while loop inside of tree() that seems to have the same or similar logic to contains_valid_file().

High Level Checks:

  • PR fully resolves the task/issue
  • Does the PR have tests?
  • Does the PR follow project code formatting standards?
  • Does the OS still compile and run?
  • Is documentation included?

Code Checks:

  • Does the code achieve its intended purpose?
  • Were all edge cases considered?
  • Is the code organized and maintainable?
  • Does the code maintain the same level of performance? (no performance regressions)

@malensek

Copy link
Copy Markdown
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants