Tree + Clear

[ebeckk]

This pull request was created to address Issue #5 which is implements the 'clear' and 'tree' command

See /docs/TREE.md for more documentation and test cases with expected outputs.

[pjpatil30]

I will review this PR.

[pjpatil30]

Looks great! Functionality works as expected and accounts for edge cases. Clear description in TREE.md as well, making it easy to use. Clean implementation for clear. Good comments in tree.c, makes it easy to follow and understand flow of the code. Test files are good (simple but makes sense considering it is just a placeholder for files). Minor improvement recommendation, since all of us have user and kernel directories, add those into the Makefile as well so that tree works and can find those directories as well. Very simple fix, just how you added tests into the Makefile so it can find the directory. But overall looks great!

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)

[hnunez02]

Verification
Repo successfully implements all goals described in the issue. No Functionalities are missing.

Testing
Good tests were added to the repo and formatting was on point according to the README. One thing I did not see that was on the README was the tree test for testtree.

|-- file1.txt
|-- file2.txt
|-- file3.txt
|-- testdir
|   |-- test4.txt

2 directories, 4 files

When pulling the repo the testdir directory was not inside the test files. However I was able to create the directory as the README displayed and when testing, using tree /tests/testtree the tree showed up as expected.

Code Walkthrough

One think that was noticed when I added that testdir directory and tested tree . I saw an error:

$ tree /tests/testtree
/tests/testtree
|-- file1.txt
|-- file2.txt
|-- file3.txt
|-- testdir
|   |-- test4.txt

2 directories, 4 files
$ tree .
.
|-- README.md
|-- cat
|-- clear
|-- echo
|-- forktest
|-- grep
|-- init
|-- kill
|-- ln
|-- ls
|-- mkdir
|-- rm
|-- sh
|-- stressfs
|-- usertests
|-- grind
|-- wc
|-- zombie
|-- logstress
|-- forphan
|-- dorphan
|-- tree
|-- tests
|   |-- testtree
|   |   |-- file1.txt
|   |   |-- file2.txt
|   |   |-- file3.txt
|   |   |-- testdir
usertrap(): unexpected scause 0xf pid=6
            sepc=0x92e stval=0x2d78`

Going through the tree.c file this could be an issue with the recursion of the buffer here.

while (read(fd, &de, sizeof(de)) == sizeof(de)) {
        if (de.inum == 0 || strcmp(de.name, ".") == 0 || strcmp(de.name, "..") == 0)
            continue; // Skip the current dir and the parent dir entries to avoid infinite loop

        strcpy(p, de.name); // Holds the name of the current item it has found, copies the item's name into the buffer

The pointer p stays the same while the buffer is always changing when going through paths. Maybe add a checkpoint for the pointer.

Formatting
Formatting of the implementations was great. Outputs displayed as expected.

Checklist
High Level Checks:

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

Code Checks:

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

[malensek]

Tree is always nice to have, and the output looks great here.

The approach for tracking the indentation level as a string is interesting. However, you will always be stuck with some hard-coded value, unless you dynamically resize the string... At which point the question might be, why not simply track the indentation level as an integer, add a helper function that dynamically indents, and call that instead? Then there's no worry about running out of space.

I see you added a strcat helper but then are also still using memmove to concatenate... I'd pick one and stick with it.

There's a comment in there about "p" being correctly identified as critical, I guess leftover LLM output? Looks like an agent did the fix. But "p" is not the problem here. Your 512-char buf is causing your program to run out of stack space as it recurses deeper and deeper. That'd be a good place to use your "global string" approach instead, plus you can only have MAX_PATH characters in a path, so using those two would completely avoid the problem there.

Lastly, adding clear is a great idea, but I would be interested to know why you need to write in a loop. Did a humble printf single write() not do the trick?

4.5/5

+0.5 the 3 fixes described above.