Skip to content

Conversation

@jlazaro2
Copy link

@jlazaro2 jlazaro2 commented Sep 30, 2024

Teammate: Esha Dupuguntla @eshadupz

Find command

Description
The find command prints out the path of a file or directory based on the start of a directory given within the command. Syntax to search for a file is formatted like this:
find <flag/option> <pattern/name of file or directory>

Additional flags

  • -name: accounts for the specific name of a file/directory or pattern
  • -type: checks for files or directories specifically

Changed files

  • Makefile: accounts for find.c file and consider tester.txt file within fd.img
  • find.c: find command and added flags
  • tester.txt: text file used for testing

Limitations

  • make sure to create a directory within QEMU to test directories "$ mkdir /testdir"
  • add files within test directory by running command "$ echo "test" > /testdir/sample.txt"
  • type flag specifically only checks for files or directories (more types are included in real find command)
  • name flag only accounts for wildcard symbols such as * or ? (more patterns are included in real find command)
  • nothing will print if target is not found

Example run in make qemu to test
make qemu
$ mkdir /testdir
$ echo "test" > /testdir/sample.txt
$ find /testdir -name sample.txt

other commands that can be run using find:
$ find . -name tester.txt
$ find . -name *.txt
$ find /testdir -name sample.txt (double check that testdir exists)
$ find . -type d
$ find . -type f
$ find /testdir -type f

@jcmendoza2
Copy link

jcmendoza2 commented Sep 30, 2024

I can code review this.

1 similar comment
@nestrada2
Copy link

I can code review this.

@suhani-arora
Copy link

I can review this!

@mvvillarrealblozis
Copy link

I can also review this

@suhani-arora
Copy link

Hi! This is a great implementation of a basic find command. Well done! The PR meets the core functionality outlined in the description. I liked the use of flags like -name and -type, and the logic for handling pattern matching with * and ? is well-structured. A few suggestions:

  • I see that you are using tester.txt for testing, but adding more varied test cases (such as those with deeply nested directories or missing files) would enhance coverage.
  • While testing, I discovered:
  1. When no files match the pattern (e.g. find . -name non_existing_file.txt, a message could be returned (e.g., No matching files found).
  2. find . -testflag sample.txt should print maybe a usage error or handle the unknown flag gracefully.
  • In terms of code readability, you've done an great job with all the documentation and comments. The code was easy to follow. One suggestion: In the find() function, the core logic for handling the flags (-type, -name) could be extracted into a separate helper function to improve readability and modularity.

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)

Overall, superb job! Thank you.

@nestrada2
Copy link

The PR complies with all the posted specifications for the find command. The code is nicely streamlined and there is not much requiring fixing.

Suggestions:

JavaDocs: add more information for the header comment in find.c such as the description, flags, and how to run it.

Add more comments within functions to clarify what each block is doing such as the while loop in find().

In this if statement (line 80-84):

// if it's a directory, traverse it
    if (st.type == T_DIR) {
        if (strlen(path) + 1 + DIRSIZ + 1 > sizeof(buf)) {
            printf("find: path too long\n");
            return;
        }

close fd before returning.

Add more test cases such as more complex patterns to match for the name flag (e.g. a mix of ?'s and *'s).

Print an error message/usage when the syntax of the find command is incorrect in anyway such as an invalid flag, wrong order of arguments or even too many arguments.

Add a help flag such as -h which would print the usage information.

  • 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)

@jcmendoza2
Copy link

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)

Hi! The code implementation achieves the suggested purpose specified in the PR outline. While having a good description of how to test the PR as well as the current limitations, the formatting is good and documentation is included. Great job!

A few suggestions:
During testing I tried:
find . -type [missing next argument]
Result: returned
find . -hello (improper flag)
Result: returned
mkdir /testdir
mkdir /testdir/moreDir
echo "test" > /testdir/moreDir/sample.md
find /testdir/moreDir/ -name sample.md
Result: /testdir/moreDir//sample.md

  • Error Handling or User Output Suggestions:
    Check to ensure an argument follows the flag (-type or -name) and tell the user another arg is needed
    Check to ensure the provided flag is valid and if not, tell the user an improper flag was provided and they could choose from -name or -type
    If provided directory is invalid in format (invalid: /testdir/moreDir/ vs valid: /testdir/moreDir) maybe provide error check saying invalid directory path? Since the result provided an extra '/' in the result
    If the target isn't found could you possible add error/user output: [insert file name] not found; to allow user to understand why they were returned

This may seem redundant but in agreement to the other reviewers I also agree that another approach to testing could have been to provide test directories and nested test directories into FogOS-find:
testdir/example.txt
testdir/moreexample.c
testdir/evenmoree.md
or testdir/cDir (subdirectory with C files) then testdir/txtDir (another subdirectory with txt files)

Overall, amazing work!

@malensek
Copy link
Contributor

This is really nice, self contained, and perfect for P1. I don't have any reservations merging this because it's a great start.

There are some things that I ran into that were surprising coming from the usual find command:

  • find by itself is usually okay, it matches any file and defaults to .
  • find <dir> usually matches anything in the dir provided

I like the support for checking file / dir types. As mentioned in the other CRs, I think the only major issue here is with how command lines are handled. As is, it's pretty brittle and expects things to be input in the exact order or it won't work, and there's no help info supplied (and nothing added to /docs to explain either). Instead of hard coding the argument positions, I'd handle them up front by searching for args that begin with - and then set up a struct with your program options: what the pattern is, the dir, and whether to check for files/dirs. Then you'd pass those options in to find and go from there -- no need to strcmp against args to see what you need to do.

Overall this is minor, you can earn +0.25 for implementing improved arg parsing. Overall very well done.

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.

6 participants