Merge branch 'copilot/modify-watch-for-config-changes' of https://github.com/kovidgoyal/kitty

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

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.