Skip to content

Conversation

@spganti
Copy link

@spganti spganti commented Sep 27, 2025

This pull request addresses issue #7.

The find command finds the path for a given file in a directory recursively. If a directory is not given then it will find the path of the file in the current directory. We added a test directory with text files to ensure the command is working. If a file does not exist in the current directory, then it will print nothing.

find

Examples:
find . test2.txt
path: /tests/test2.txt

find . test1.txt
path: /tests/itests/test1.txt
path: /tests/tests1.txt

@NeuerWang
Copy link

I am going to review this PR.

@NeuerWang
Copy link

Verification
Issue: "Find is used to locate files and directories, and we would implement the basic syntax. We would like to cover the following components: path, options, and expression."

This PR meets 2 goals out of 3 described in the issue. Missing: the options component.

Testing
When I test it, the basic functionality of the utility is good. However, when I test none-exist files, subdirs, and dirs, it wasn't working.
局部截取_20251002_195256

Code Walkthrough
Logical Error in findit function
The primary bug causing incorrect directory matching and incomplete results is a logical error.
if (st.type == T_FILE) {
if (match(expr, fmtname(path))) {
printf("Path: %s\n", path);
}
} else if (T_DIR) { // This is the error
if(strlen(path) + 1 + DIRSIZ + 1 > sizeof buf){
// ...
}
// ... recursive search logic ...
}

The line else if (T_DIR) will always be true if the entry is not a file, because T_DIR is a constant equal to 1. The code never checks if the directory's name actually matches the expression. It just immediately starts searching inside it.

Formatting
The indentation is correct, and the style is clean. No major formatting issues were found.

Here is the updated checklist based on the testing and code walkthrough:

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:

[ ] 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?

@bnhtphh
Copy link

bnhtphh commented Oct 5, 2025

Verification
This pull request addresses issue #7, the command syntax: find . Great job!

Testing
There are test files included, and I am able to test them with it. However, when I tried to test with , I am not sure if it is a bug or not, it confused me a little bit. When I test with c, it prints out all of them.

"find / c*
Path: //README.md
Found!
Path: //cat
Found!
Path: //echo
Found!
Path: //forktest
Found!
Path: //grep
Found!
Path: //init
Found!
Path: //kill
Found!
Path: //ln
Found!
Path: //ls
Found!
Path: //mkdir
Found!
Path: //rm
Found!
Path: //sh
Found!
Path: //stressfs
Found!
Path: //usertests
Found!
Path: //grind
Found!
Path: //wc
Found!
Path: //zombie
Found!
Path: //logstress
Found!
Path: //forphan
Found!
Path: //dorphan
Found!
Path: //find
Found!
Path: //tests/itestdir/itest1.txt
Found!
Path: //tests/itestdir/itest2.txt
Found!
Path: //tests/test1.txt
Found!
Path: //tests/test2.txt
Found!
Path: //console
Found!
$
"

Code Walkthrough
The code looks great overall. Also, I think you should check again for the * check and match code.

Checklist
[ ] 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?

@RebeccaLLittle
Copy link

RebeccaLLittle commented Oct 6, 2025

Verification
The project meets the majority of the provided spec but is missing the “options” component mentioned in issue #7, not sure if you truly committed to that though. But other than that, overall nice work!

Testing
You’re tests do a good baseline job of showing how “find” is supposed to work. I also agree with all the comments up above. However, I think the tests could be a little more comprehensive. For example:

tests/itestdir/test1.txt
tests/test1.txt

→ to test how well the recursive search works

Code Walkthrough
I think overall you’re code looks great, it is a little hard for me to understand given its complexity but also impressive! I think you may have an extra buffer hanging around on line 7, or I may be missing it usage.

I also agree with the logical error mentioned above.

Formatting
Format is great nice work! I appreciate to documentation for Regexp matcher and all the comments

Checklist
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?

Very impressive job guys!! : )

@malensek
Copy link
Contributor

Looks good!! I am not a huge fan of how it prints "found!" all over the place but I will chalk that up to personal preference :-)

Behavior seems all good to me. I do agree that the extra buffer needs to go, and so does the logic error (although it's not really causing problems in most cases, it should still be fixed).

It's easy to get it to mess up the formatted output, especially with slashes. Searching in ., ./, and / all produce different outputs, which is okay, except with / it actually displays two slashes (which is technically okay)

4.75/5

0.25 -> fix errors mentioned above.

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