Skip to content

Conversation

@MichaelBooyers
Copy link

This is an implementation of the pwd (print working directory) command line user utility. To print our working directory, we need to first get the current working directory. This code below gets the current working directory (cwd) then allows the shell to automatically update with the cwd when we change directories throughout our system. Then pwd will execute as a normal command and print our directory in a new line.

@ebeckk
Copy link

ebeckk commented Oct 4, 2025

I will be reviewing this PR.

@ebeckk
Copy link

ebeckk commented Oct 5, 2025

Verification
The original issue/task is to implement the pwd command to allow user to see the pathname of their current working directory. Additionally, when the user cds around, you'll see the updated PWD in the prompt.

Testing
The testing was good. All the directories/subdirectories were loaded in on init so I was able to test pwd successfully. The command worked great throughout the whole time and I never ran into any issues.

  • One issue I ran into is that there doesn't seem to be the /pwd implemented in the UPROGS section of the Makefile. This isn't a big deal at all though because "pwd" by itself works fine in the OS.

Code walkthrough:

  • One suggestion/something that could be considered an edge case is that when isolating the components/building cwd, maybe you could be checking num_components against the size of components[32][64]. Although it's very unlikely to have a path that large, its just an edge case to be considered.

  • Besides that, I couldn't find other edge cases/logical errors in the code

  • Regarding documentation, it is present in docs/pwd.md. Most of it was easy to understand but the section on expected outputs of the tests was a bit hard to follow at first.

  • The comments in the code were helpful in understanding what was happening and made the code more readable.

  • Formatting was consistent and very good overall.

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? (no performance regressions)

@Marcosg1
Copy link

Marcosg1 commented Oct 8, 2025

I will code review this

@Marcosg1
Copy link

Verification
The scope of this pull request was to implement a pwd command and you did that. It correctly retrieves and displays the current working directory, updating as the user navigates through the system. The functionality matches the expected behavior with no missing features.

Testing
It was easy to test your pwd implementation. I appreciated that you created both parent and child directories in the build, this made it simple to check that pwd correctly reflects the user's location in the directory structure. The examples in your documentation also helped show how the command should behave during navigation.I tested quite a bit and couldn’t find any issues with its core functionality.

The one issue I did run into is that /pwd doesn’t execute. This seems to be because you didn’t add pwd to the UPROGS section of the Makefile.

Code Walkthrough

  • Your implementation of pwd is well structured and it looks great logically. I liked that you preloaded both parent and child directories, which made testing more straightforward.
  • The documentation was clear and comprehensive, showing us how the pwd command behaves as the user goes through directories.
  • Your comments throughout helped me understand what you were doing and it made your code more readable
  • I will say reading your getcwd_user.c file took a bit to understand will so much going on so I would suggest making helper functions to make it to read and understand what is going on.

Formatting
I found no formatting issues. Your code formatting was consistent all around.

Checklist:

High Level Checks:
[✅] PR fully resolves the task/issue
[✅] PR includes test cases
[✅] PR follows the project code formatting standards
[✅] The OS compiles and runs successfully
[✅] Documentation is included

Code Checks:
[✅] The code achieves its intended purpose
[✅] All edge cases were considered
[✅] Code is organized and maintainable
[✅] The code maintained the same level of performance

@malensek
Copy link
Contributor

This is a good implementation of tracking the CWD without using the process level CWD maintained in the kernel.

The unfortunate side effect of tracking all the movements yourself is:

  • If you ever add another place where chdir is used in the shell, you'll need to remember to also call your CWD function
  • If you execute the shell anywhere other than /, then it won't have any idea where it's actually at. Ex: cd /test1/show/done, then /sh. New sh thinks it's in the root directory and any cds we do now will show the wrong information.

There isn't much you can do to avoid that with this approach though.

As far as the code goes, I like that this change was moved out to a separate file to decrease the impact on sh.c, but I agree that it needs helper functions or some additional organization so that a reader can easily understand what is happening. Even some comments with examples of what the path looks like at various stages would help make this more understandable.

Right now, the way this works, all programs now have a global 'cwd' taking up MAX_PATH characters of space. Instead of doing it this way, we can make cwd an input argument to your function. That way only programs that care about the CWD need to eat that memory penalty.

32x64 seems reasonable for your components, but how did you land on that? Honestly, maybe you could base those on MAX_PATH so you know all bases are covered. The dimensions of this array should be constants (initially I was confused when I saw 63 somewhere in your code, wondering what that was for).

4.75/5
Earn back +0.25 w/ code reorganization, comments to make it more readable / maintainable, refactoring described 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.

4 participants