added watch func

[Unbanneduser]

added watch func

[mvvillarrealblozis]

i can code review this

[suhani-arora]

I can code review this!

[suhani-arora]

Hi! Really cool project, and great implementation of the watch command. One thing: I wish you had pushed your work in the testpause branch to the main branch. As a reviewer, I didn't know that the working code was in this branch, so I was testing with the main branch, wondering why it wasn't working. But, I was super impressed to see the working implementation in testpause.

Everything is very well documented in the README.md file, I appreciate this effort.

The core features of the watch command (interval, header display, and output redirection) work as expected. I tested commands with various options, and they performed well. Except for the following test case:
watch -n 5 -h -o date_log.txt date in which the output stored in date_log.txt was:

--- 4031 ticks: date ---
Failed to execute command: date

--- 4084 ticks: date ---
Failed to execute command: date 

This would be an issue to solve in parsing the command arguments.

One suggestion regarding usability: Consider extending the error message for unknown flags. Currently, it exits with a generic message Unknown option: . Maybe print a usage message with the expected options could be more user-friendly.

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.

[mvvillarrealblozis]

I really liked that the README was thorough, although it would've been nice to see some more Java-doc style documentation. Another thing I noticed is that there was some dead or unused code left in user/watch.c. Like Suhani, I also had some trouble getting the code to compile on this branch. Overall I think its a pretty solid addition, good work.

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)

[Unbanneduser]

Sorry I didn't merge the correct one to main, I think the code will works now.

[malensek]

Ok, this is an interesting project and you got to work with redirection, that's cool. Your description for this PR leaves a lot to be desired. I guess at a minimum you could reference the issue.

The original PR stated you would support:

Alerts and Notifications
Pause and Resume Functionality

I do see support for killing the program with Ctrl+C but not pause/resume or alerts/notifications.

Going through your code, I am confused about the use of print_header. Couldn't this be replaced with:

fprintf(fd, "\n--- %d ticks: %s ---\n", uptime(), command);

Also, wouldn't it make sense to print the number of seconds instead (you're already doing that conversion)? I don't know if users will care how many ticks have elapsed. Ideally this would be done with the RTC instead.

If the watched program fails to run, it just keeps running it anyway as Suhani mentioned. There is also a decent amount of dead (commented) code in this project and it doesn't follow xv6 formatting.

I think the idea to kill the program with Ctrl+C is interesting but now you have a special key combination just for your program. I think I'd ideally find a way to do it in a bit more generic fashion but changing the kernel isn't really the point for this assignment so that's totally fine. It was a good way to get that working for now.

You can do the code cleanup and error checking mentioned above to get +1 back on your implementation.

[malensek]

Updated version looks good, has all the changes except the error checking.