diff --git a/tools/watch/api.go b/tools/watch/api.go index e08632fd7..0a1acc229 100644 --- a/tools/watch/api.go +++ b/tools/watch/api.go @@ -21,27 +21,16 @@ import ( var _ = fmt.Print -// watch_dir starts fswatcher in a background goroutine and pipes events to a custom channel. -func watch_dirs(ctx context.Context, paths []string, debounce time.Duration, eventChan chan<- fswatcher.WatchEvent) error { - opts := []fswatcher.WatcherOpt{ - fswatcher.WithCooldown(debounce), +// get_parent_dirs returns a deduplicated list of the immediate parent directory for each path. +// Unlike get_unique_directories it does not filter out subdirectories, so every unique +// parent is returned even when some are descendants of others. This is the correct +// set of directories to pass to a non-recursive (top-level) file-system watcher. +func get_parent_dirs(paths []string) []string { + dirSet := utils.NewSet[string](len(paths)) + for _, p := range paths { + dirSet.Add(filepath.Dir(p)) } - for _, path := range paths { - if unix.Access(path, unix.R_OK|unix.X_OK) == nil { - opts = append(opts, fswatcher.WithPath(path)) - } - } - w, err := fswatcher.New(opts...) - if err != nil { - return err - } - go w.Watch(ctx) - go func() { - for event := range w.Events() { - eventChan <- event - } - }() - return nil + return dirSet.AsSlice() } // returns the closest unique parent directories for a list of paths. @@ -122,33 +111,91 @@ func get_set_of_config_files(config_paths []string) *utils.Set[string] { return result } -// watch_for_config_changes watches the directories derived from config_paths and calls action -// whenever a watched config file (including includes and auto color scheme files) changes. +// watch_for_config_changes watches the parent directories of every conf file (main configs, +// includes, and auto color-scheme files) and calls action whenever one of those files changes. +// Watching is non-recursive (top-level only): only the immediate parent directories are added. +// When a conf file change is detected the full set of conf files is re-scanned so that newly +// added or removed include directives are reflected in the watched-directory set. // It runs until ctx is cancelled. func watch_for_config_changes(ctx context.Context, action func() error, debounce_time time.Duration, config_paths []string) error { event_chan := make(chan fswatcher.WatchEvent) + all_paths := get_set_of_config_files(config_paths) - dirs_to_watch := get_unique_directories(all_paths.AsSlice()) - if len(dirs_to_watch) == 0 { + + // desired_dirs is the full set of parent directories we want to watch + // (one per conf file, including files that may not yet exist). + desired_dirs := utils.NewSet[string]() + for _, p := range all_paths.AsSlice() { + desired_dirs.Add(filepath.Dir(p)) + } + if desired_dirs.Len() == 0 { return fmt.Errorf("No directories to watch provided") } - filtered_action := func(ev fswatcher.WatchEvent) error { - all_paths := get_set_of_config_files(config_paths) - if all_paths.Has(resolve_path(ev.Path)) { - return action() + // Create the watcher with top-level (non-recursive) depth. + opts := []fswatcher.WatcherOpt{fswatcher.WithCooldown(debounce_time)} + watched_dirs := utils.NewSet[string]() + for _, dir := range desired_dirs.AsSlice() { + if unix.Access(dir, unix.R_OK|unix.X_OK) == nil { + opts = append(opts, fswatcher.WithPath(dir, fswatcher.WithDepth(fswatcher.WatchTopLevel))) + watched_dirs.Add(dir) } - return nil + } + if watched_dirs.Len() == 0 { + return fmt.Errorf("No directories to watch provided") } - if err := watch_dirs(ctx, dirs_to_watch, debounce_time, event_chan); err != nil { + w, err := fswatcher.New(opts...) + if err != nil { return err } + go w.Watch(ctx) + go func() { + for event := range w.Events() { + event_chan <- event + } + }() + + // sync_watched_dirs reconciles watched_dirs with desired_dirs: any desired directory + // that now exists is added to the watcher, and any watched directory that is no longer + // desired is dropped. + sync_watched_dirs := func() { + desired_dirs.ForEach(func(d string) { + if !watched_dirs.Has(d) && unix.Access(d, unix.R_OK|unix.X_OK) == nil { + if err := w.AddPath(d, fswatcher.WithDepth(fswatcher.WatchTopLevel)); err == nil { + watched_dirs.Add(d) + } + } + }) + watched_dirs.ForEach(func(d string) { + if !desired_dirs.Has(d) { + _ = w.DropPath(d) + watched_dirs.Discard(d) + } + }) + } + for { select { case event := <-event_chan: - if err := filtered_action(event); err != nil { - fmt.Fprintf(os.Stderr, "failed to signal kitty in event: %s with error: %s\n", event, err) + // On every event try to activate any desired directories that may have been + // created since the last check (e.g. a new include directory was mkdir'd). + sync_watched_dirs() + + new_all_paths := get_set_of_config_files(config_paths) + if new_all_paths.Has(resolve_path(event.Path)) { + // A conf file changed: rebuild desired_dirs from the new include set and + // sync the watcher so new include directories are watched and stale ones dropped. + new_desired := utils.NewSet[string]() + for _, p := range new_all_paths.AsSlice() { + new_desired.Add(filepath.Dir(p)) + } + desired_dirs = new_desired + sync_watched_dirs() + + if err := action(); err != nil { + fmt.Fprintf(os.Stderr, "failed to signal kitty in event: %s with error: %s\n", event, err) + } } case <-ctx.Done(): return nil diff --git a/tools/watch/api_test.go b/tools/watch/api_test.go index d28cf289b..a325c8ee6 100644 --- a/tools/watch/api_test.go +++ b/tools/watch/api_test.go @@ -15,6 +15,30 @@ import ( "github.com/google/go-cmp/cmp" ) +// prime_watcher repeatedly writes to path until the watcher delivers an event and +// the action fires, confirming the watcher goroutine is ready. A single write may +// race the watcher's initialisation so retrying ensures at least one write lands +// while the watcher is active. After success it waits one full debounce period +// so the cooldown window is clear before returning. +// Returns the current action count after settling. +func prime_watcher(t *testing.T, path string, counter *atomic.Int32, debounce time.Duration) int32 { + t.Helper() + before := counter.Load() + deadline := time.Now().Add(5 * time.Second) + n := 0 + for time.Now().Before(deadline) { + write_file(t, path, fmt.Sprintf("# prime %d\n", n)) + n++ + if wait_for_count(counter, before+1, debounce+100*time.Millisecond) > before { + // Action fired — clear the debounce window before returning. + time.Sleep(debounce + 20*time.Millisecond) + return counter.Load() + } + } + t.Fatal("watcher failed to become ready: no action fired within 5 seconds of repeated prime writes") + return 0 +} + func write_file(t *testing.T, path string, data string) { t.Helper() if err := os.WriteFile(path, []byte(data), 0o600); err != nil { @@ -22,6 +46,68 @@ func write_file(t *testing.T, path string, data string) { } } +func TestGetParentDirs(t *testing.T) { + type tc struct { + name string + input []string + expect []string + } + cases := []tc{ + { + name: "nil input", + input: nil, + expect: nil, + }, + { + name: "single file", + input: []string{"/a/b/file.conf"}, + expect: []string{"/a/b"}, + }, + { + name: "files in same directory", + input: []string{"/a/b/file1.conf", "/a/b/file2.conf"}, + expect: []string{"/a/b"}, + }, + { + name: "sibling directories", + input: []string{"/a/b/file.conf", "/a/c/file.conf"}, + expect: []string{"/a/b", "/a/c"}, + }, + { + name: "parent and child directory both returned", + // Unlike get_unique_directories, subdirectories are NOT filtered out. + input: []string{"/a/file.conf", "/a/b/file.conf"}, + expect: []string{"/a", "/a/b"}, + }, + { + name: "deeply nested all returned", + input: []string{ + "/a/file.conf", + "/a/b/file.conf", + "/a/b/c/file.conf", + }, + expect: []string{"/a", "/a/b", "/a/b/c"}, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + result := get_parent_dirs(tc.input) + sort.Strings(result) + sort.Strings(tc.expect) + if tc.expect == nil { + if len(result) != 0 { + t.Fatalf("expected empty result, got %v", result) + } + return + } + if diff := cmp.Diff(tc.expect, result); diff != "" { + t.Fatalf("get_parent_dirs mismatch (-want +got):\n%s", diff) + } + }) + } +} + func TestGetUniqueDirectories(t *testing.T) { // Empty input if result := get_unique_directories(nil); result != nil { @@ -135,22 +221,35 @@ func wait_for_count(counter *atomic.Int32, target int32, timeout time.Duration) return counter.Load() } +// TestWatchForConfigChanges consolidates all watcher integration tests into a single +// function that starts the watcher once and confirms readiness via prime_watcher +// instead of a blind time.Sleep. Include-watching scenarios (file changes, include +// added/removed from main config, include added to an already-included file) run as +// sequential subtests. func TestWatchForConfigChanges(t *testing.T) { - tdir := t.TempDir() + tdir := resolve_path(t.TempDir()) subdir := filepath.Join(tdir, "sub") - if err := os.Mkdir(subdir, 0o700); err != nil { - t.Fatal(err) + extradir := filepath.Join(tdir, "extra") + extra2dir := filepath.Join(tdir, "extra2") + for _, d := range []string{subdir, extradir, extra2dir} { + if err := os.Mkdir(d, 0o700); err != nil { + t.Fatal(err) + } } main_conf := filepath.Join(tdir, "kitty.conf") included_conf := filepath.Join(subdir, "included.conf") dark_theme := filepath.Join(tdir, "dark-theme.auto.conf") unrelated := filepath.Join(tdir, "unrelated.txt") + extra_conf := filepath.Join(extradir, "custom.conf") + extra2_conf := filepath.Join(extra2dir, "another.conf") write_file(t, main_conf, "include sub/included.conf\n") write_file(t, included_conf, "background black\n") write_file(t, dark_theme, "background #000000\n") write_file(t, unrelated, "this file should not trigger action\n") + write_file(t, extra_conf, "background black\n") + write_file(t, extra2_conf, "background black\n") var action_count atomic.Int32 action := func() error { @@ -167,47 +266,102 @@ func TestWatchForConfigChanges(t *testing.T) { done <- watch_for_config_changes(ctx, action, debounce, []string{main_conf}) }() - // Give the watcher time to start - time.Sleep(200 * time.Millisecond) + // Confirm the watcher is ready by writing to the dark-theme auto conf (an + // always-watched file that does not affect the include graph) and waiting for + // an action to fire. This replaces the blind time.Sleep used previously. + prime_watcher(t, dark_theme, &action_count, debounce) t.Run("main config change triggers action", func(t *testing.T) { before := action_count.Load() write_file(t, main_conf, "include sub/included.conf\nfont_size 13\n") - count := wait_for_count(&action_count, before+1, 2*time.Second) - if count <= before { - t.Fatalf("Expected action to be called after main config change, count=%d", count) + if wait_for_count(&action_count, before+1, 2*time.Second) <= before { + t.Fatalf("Expected action to be called after main config change, count=%d", action_count.Load()) } }) + time.Sleep(debounce + 20*time.Millisecond) t.Run("included file change triggers action", func(t *testing.T) { before := action_count.Load() write_file(t, included_conf, "background white\n") - count := wait_for_count(&action_count, before+1, 2*time.Second) - if count <= before { - t.Fatalf("Expected action to be called after included file change, count=%d", count) + if wait_for_count(&action_count, before+1, 2*time.Second) <= before { + t.Fatalf("Expected action to be called after included file change, count=%d", action_count.Load()) } }) + time.Sleep(debounce + 20*time.Millisecond) t.Run("auto color scheme file change triggers action", func(t *testing.T) { before := action_count.Load() write_file(t, dark_theme, "background #111111\n") - count := wait_for_count(&action_count, before+1, 2*time.Second) - if count <= before { - t.Fatalf("Expected action to be called after dark-theme.auto.conf change, count=%d", count) + if wait_for_count(&action_count, before+1, 2*time.Second) <= before { + t.Fatalf("Expected action to be called after dark-theme.auto.conf change, count=%d", action_count.Load()) } }) + time.Sleep(debounce + 20*time.Millisecond) t.Run("unrelated file change does not trigger action", func(t *testing.T) { before := action_count.Load() write_file(t, unrelated, "still unrelated\n") - // Wait debounce + a bit more to ensure no spurious call time.Sleep(debounce + 200*time.Millisecond) - after := action_count.Load() - if after != before { + if after := action_count.Load(); after != before { t.Fatalf("Expected action NOT to be called for unrelated file, count went from %d to %d", before, after) } }) + // include added to main config: extradir must become watched. + // sync_watched_dirs() runs before action() in the event loop, so by the time + // wait_for_count returns the new directory is already registered. + t.Run("include added to main config is watched", func(t *testing.T) { + before := action_count.Load() + write_file(t, main_conf, "include sub/included.conf\ninclude extra/custom.conf\n") + if wait_for_count(&action_count, before+1, 2*time.Second) <= before { + t.Fatalf("Expected action after kitty.conf gained include directive") + } + // Let the debounce window clear before writing to the newly watched file. + time.Sleep(debounce + 20*time.Millisecond) + before = action_count.Load() + write_file(t, extra_conf, "background white\n") + if wait_for_count(&action_count, before+1, 2*time.Second) <= before { + t.Fatalf("Expected action after modifying newly included file") + } + }) + time.Sleep(debounce + 20*time.Millisecond) + + // include added to an already-included file: extra2dir must become watched. + t.Run("include added to already-included file adds its parent dir", func(t *testing.T) { + // Add an include to sub/included.conf that points into extra2dir, which is + // not currently watched. The watcher re-scans the full include graph on + // every conf-file change, so extra2dir must be added to the watch set. + before := action_count.Load() + write_file(t, included_conf, "include ../extra2/another.conf\n") + if wait_for_count(&action_count, before+1, 2*time.Second) <= before { + t.Fatalf("Expected action after sub/included.conf gained include directive") + } + time.Sleep(debounce + 20*time.Millisecond) + // extra2dir is now watched; a change to extra2/another.conf must fire. + before = action_count.Load() + write_file(t, extra2_conf, "background blue\n") + if wait_for_count(&action_count, before+1, 2*time.Second) <= before { + t.Fatalf("Expected action after modifying file included from an already-included conf file") + } + }) + time.Sleep(debounce + 20*time.Millisecond) + + // include removed from main config: extradir must be dropped from the watch set. + t.Run("include removed from main config is no longer watched", func(t *testing.T) { + before := action_count.Load() + write_file(t, main_conf, "include sub/included.conf\n") + if wait_for_count(&action_count, before+1, 2*time.Second) <= before { + t.Fatalf("Expected action after kitty.conf lost include directive") + } + time.Sleep(debounce + 20*time.Millisecond) + before = action_count.Load() + write_file(t, extra_conf, "background green\n") + time.Sleep(debounce + 200*time.Millisecond) + if after := action_count.Load(); after != before { + t.Fatalf("Expected NO action after modifying removed-include file, count went from %d to %d", before, after) + } + }) + cancel() select { case err := <-done: @@ -239,29 +393,31 @@ func TestWatchForConfigChangesDebounce(t *testing.T) { done <- watch_for_config_changes(ctx, action, debounce, []string{main_conf}) }() - // Give the watcher time to start - time.Sleep(200 * time.Millisecond) + // Confirm the watcher is ready before sending the burst. + prime_watcher(t, main_conf, &action_count, debounce) // Write to the file several times rapidly within the debounce window. // The fswatcher debouncer drops events that occur within the cooldown period // after the first event, so only the first write should produce an action call. + before_burst := action_count.Load() for i := range 5 { write_file(t, main_conf, fmt.Sprintf("font_size %d\n", 12+i)) time.Sleep(20 * time.Millisecond) } - // Wait for up to one full debounce period for the first action to fire - count_after_burst := wait_for_count(&action_count, 1, debounce+500*time.Millisecond) + // Wait for up to one full debounce period for the first action to fire. + count_after_burst := wait_for_count(&action_count, before_burst+1, debounce+500*time.Millisecond) + action_calls_in_burst := count_after_burst - before_burst // Debouncing should have collapsed the burst into at most 2 calls. // The fswatcher debouncer uses leading-edge logic: the first event fires // immediately and subsequent events within the cooldown window are dropped. // A trailing event may fire at the end of the cooldown window, giving at most 2. - if count_after_burst == 0 { + if action_calls_in_burst == 0 { t.Fatalf("Expected at least one action call after burst of writes, got 0") } - if count_after_burst > 2 { - t.Fatalf("Expected debouncing to collapse burst: want ≤2 calls, got %d", count_after_burst) + if action_calls_in_burst > 2 { + t.Fatalf("Expected debouncing to collapse burst: want ≤2 calls, got %d", action_calls_in_burst) } // After waiting well past the debounce window, a new write should trigger exactly one more call.