Skip to content

add watch / unwatch commands #6063

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
aryairani merged 9 commits into from
Dec 19, 2025
Merged

add watch / unwatch commands #6063

aryairani merged 9 commits into from
Dec 19, 2025

Conversation

@aryairani
Copy link
Contributor

@aryairani aryairani commented Dec 18, 2025 •
edited
Loading

Overview

These commands let you watch multiple directories or multiple individual files. Initially we watch the start-up directory like usual, but you can remove it and/or add other paths.

scratch/main> help watches

  watches
  List all external paths currently being watched for changes.

scratch/main> help watch

  watch
  `watch <file or directory>`  Watch an external file or directory for changes.
                               Changes to `.u` files in watched locations will
                               be automatically loaded.

scratch/main> help unwatch

  unwatch
  `unwatch <file or directory>`  Stop watching one or more external files or
                                 directories for changes.
  `unwatch`                      With no arguments, list currently watched
                                 paths.

Closes #4850.

Implementation approach and notes

How does it accomplish it, in broad strokes? i.e. How does it change the Haskell codebase?

Interesting/controversial decisions

  • Hopefully I didn't mess up any Ki / finalizer stuff. (I did?)

  • unwatch accepts multiple args, but watch only accepts one arg. I guess there's no harm in having watch accept multiple args, but I wasn't sure it was worth the effort.

Test coverage

  • Have you included tests (which could be a transcript) for this change, or is it somehow covered by existing tests?

Also a transcript showing that the three new commands are disabled (along with file watching in general) during transcripts.

  • Would you recommend improving the test coverage (either as part of this PR or as a separate issue) or do you think it’s adequate?

I would like better test coverage, but it would require coordinating two processes to use CLI on one side and trigger file events on the other side, and I acknowledge we don't have any tests like that yet and it might be a pain to set up.

  • If you only tested by hand, because that's all that's practical to do for this change, mention that. Include screenshots.
  Now starting the Unison Codebase Manager (UCM)...


   _____     _             
  |  |  |___|_|___ ___ ___ 
  |  |  |   | |_ -| . |   |
  |_____|_|_|_|___|___|_|_|
  
  👋 Welcome to Unison!
  
  You are running version: release/1.0.0-88-g6458285fd'


  📚 Read the official docs at https://www.unison-lang.org/docs/
  
  Hint: Type 'projects' to list all your projects, or 'project.create' to start
  something new.

scratch/main> watch /tmp/test-watch          

  I'm now watching /private/tmp/test-watch for changes.
  
  Tip: Use `watches` to see all locations being watched.

scratch/main> 

  Loading changes detected in /private/tmp/test-watch/scratch.u.


  + x : ##Nat
  
  Run `update` to apply these changes to your codebase.

scratch/main> unwatch

  I'm watching these paths for changes:
  
    1. /Users/arya/work/unison/watch-other-file
    2. /private/tmp/test-watch
  
  Tip: Use `watch` or `unwatch` to add or remove paths.

scratch/main> unwatch 2
scratch/main> unwatch /private/tmp/test-watch

  I'm no longer watching /private/tmp/test-watch for changes.
  
  I'm still watching:
  
    1. /Users/arya/work/unison/watch-other-file

scratch/main> load /private/tmp/test-watch/scratch.u 

  Loading changes detected in /private/tmp/test-watch/scratch.u.


  + y : ##Nat
  
  Run `update` to apply these changes to your codebase.

scratch/main> watch /tmp

  I'm now watching /private/tmp for changes.
  
  Tip: Use `watches` to see all locations being watched.

scratch/main> watch /var

  I'm now watching /private/var for changes.
  
  Tip: Use `watches` to see all locations being watched.

scratch/main> watches

  I'm watching these paths for changes:
  
    1. /Users/arya/work/unison/watch-other-file
    2. /private/tmp
    3. /private/var
  
  Tip: Use `watch` or `unwatch` to add or remove paths.

scratch/main> unwatch 2-3
scratch/main> unwatch /private/tmp /private/var

  I'm no longer watching /private/tmp or /private/var for changes.
  
  I'm still watching:
  
    1. /Users/arya/work/unison/watch-other-file

scratch/main> unwatch 1
scratch/main> unwatch /Users/arya/work/unison/watch-other-file

  I'm no longer watching /Users/arya/work/unison/watch-other-file (or any other
  paths) for changes.
  
  Tip: Use `watch <path>` to watch a file or directory.

scratch/main> unwatch /not/a/real/path

  
  ⚠️
  
  I already wasn't watching these paths: /not/a/real/path
  
  Tip: Use `watch <path>` to watch a file or directory.

scratch/main> watch /not/a/real/path

  ⚠️
  
  When I tried to watch /not/a/real/path, it didn't seem to exist.

scratch/main>

Both as shown above.

Loose ends

n/a

Final checklist

  • Choose your PR title well: Your pull request title is what's used to create release notes, so please make it descriptive of the change itself, which may be different from the initial motivation to make the change.
  • Update your PR description if the specifics of the PR have changed over time.
  • Include transcripts or screenshots that demonstrate the changed behavior.
  • If you changed .cabal files, make sure the package.yaml files are up-to-date instead.

@aryairani
Copy link
Contributor Author

aryairani commented Dec 18, 2025

@mitchellwrosen Could you check if I broke Watch.hs (although it seems to work fine).

@mitchellwrosen
Copy link
Member

mitchellwrosen commented Dec 18, 2025

@aryairani Couple things –

  1. It looks like the ki scope isn't used anymore? That's the mechanism that ensured file watcher thread cleanup. In practice, in this app, I don't think it's very important to guarantee that file watcher threads as a scope exits, as that means we're exiting the global app scope and the process is terminating. But I still think the pattern is cleaner than fire-and-forget style concurrency that can leave threads running in the background accidentally, and I'm not sure removing the structured concurrency was intentional. Could you comment?

  2. Minor, but there's a race condition related to the non-atomicity of checking whether a path is already watched, and setting up a watcher for it. Do you think that matters?

@aryairani
Copy link
Contributor Author

aryairani commented Dec 18, 2025 •
edited
Loading

@aryairani Couple things –

  1. It looks like the ki scope isn't used anymore? That's the mechanism that ensured file watcher thread cleanup. In practice, in this app, I don't think it's very important to guarantee that file watcher threads as a scope exits, as that means we're exiting the global app scope and the process is terminating. But I still think the pattern is cleaner than fire-and-forget style concurrency that can leave threads running in the background accidentally, and I'm not sure removing the structured concurrency was intentional. Could you comment?
  2. Minor, but there's a race condition related to the non-atomicity of checking whether a path is already watched, and setting up a watcher for it. Do you think that matters?

Thanks yeah, these were both accidental delirium-related outcomes.

For 1. I think I ended up there because of something to do with wanting to to unwatch paths even before application exit. Would it be better to have both an explicit and an implicit cleanup? Do you know if this can actually leave threads running in the background? In any case, it was an accident to leave a Ki scope that isn't used.

For 2. Yeah not ideal; is it that the race would happen if watch path set changes are come from some other thread? but I don't think we want that anyway. i.e. right now the idea is that they're disabled in non-interactive contexts, and there shouldn't be a race in an interactive context, I don't think?

@mitchellwrosen
Copy link
Member

mitchellwrosen commented Dec 18, 2025

For 1, I would say it'd only be theoretically better to ensure all file-watching threads get cleaned up. I think we could just abandon ki and go with this solution of only calling the "stop watching" command on an explicit unwatch.

For 2, agree there shouldn't be a race in practice, just wanted to call out the non-atomicity in the implementation.

I think we could just delete Ki.Scope from the state and merge

@aryairani
Copy link
Contributor Author

aryairani commented Dec 18, 2025

Okay I'll do that then. We can always revisit too, thanks for taking a look.

@aryairani
forgetting about Ki scope for file watching for now
5e50976 
though we could revisit later
@aryairani
tweak output message
90591db 
doesn't appear in transcripts because the file watcher is disabled in transcripts
@aryairani
tweak help text
9cb1fab
@aryairani
Merge branch 'arya/watch-other-file.claude' of github.com:unisonweb/u…
00b7545 
…nison into arya/watch-other-file.claude
@aryairani aryairani merged commit cc4c107 into trunk Dec 19, 2025
57 of 59 checks passed
@aryairani aryairani deleted the arya/watch-other-file.claude branch December 19, 2025 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mitchellwrosen mitchellwrosen mitchellwrosen approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

No way to monitor a scratch-file outside of working dir

3 participants

@aryairani @mitchellwrosen