diff --git a/changes/watch-repeated-events.fix.md b/changes/watch-repeated-events.fix.md new file mode 100644 index 0000000000..fd364c3f68 --- /dev/null +++ b/changes/watch-repeated-events.fix.md @@ -0,0 +1 @@ +- Watch mode no longer rebuilds, and prints the same errors again, when one file change is reported twice. diff --git a/rewatch-ocaml/tests/check_command_validation.sh b/rewatch-ocaml/tests/check_command_validation.sh index 93888d452b..705a81185a 100755 --- a/rewatch-ocaml/tests/check_command_validation.sh +++ b/rewatch-ocaml/tests/check_command_validation.sh @@ -1291,11 +1291,16 @@ if [ -s "$work/quiet-watch.out" ] || \ cat "$work/quiet-watch.out" "$work/quiet-watch.err" >&2 exit 1 fi -# macOS and Windows can report one write as several change events, and each -# rebuild reports the parse error again. Compare the distinct reports only. +# macOS and Windows can report one write as several change events. The +# watcher skips a rebuild when the file is unchanged, but a rebuild that sees +# the write half done would report the error again, so compare the distinct +# reports only. Each report starts with a blank line; the split is before it +# and must not cut a CRLF line ending. node -e ' const fs = require("fs"); - const blocks = fs.readFileSync(process.argv[1], "utf8").split(/^(?= Syntax error!)/m); + const blocks = fs + .readFileSync(process.argv[1], "utf8") + .split(/(?<=\n)(?=\r?\n Syntax error!\r?$)/m); const distinct = blocks.filter((block, i) => block !== blocks[i - 1]); fs.writeFileSync(process.argv[2], distinct.join("")); ' "$work/quiet-watch.err" "$work/quiet-watch.err.distinct" diff --git a/rewatch-ocaml/watch_snapshot.ml b/rewatch-ocaml/watch_snapshot.ml index c1c919b493..357134dbe5 100644 --- a/rewatch-ocaml/watch_snapshot.ml +++ b/rewatch-ocaml/watch_snapshot.ml @@ -316,8 +316,11 @@ let changes_between before after = (fun (first : change) second -> String.compare first.path second.path) !changes +type entries_update = Unchanged | Updated of entry list | Not_in_snapshot + let update_entries digest_cache previous changes = let changed = Hashtbl.create (List.length changes) in + let any_changed = ref false in List.iter (fun (change : change) -> Hashtbl.replace changed change.path change.kind) changes; @@ -329,13 +332,14 @@ let update_entries digest_cache previous changes = | Some Removed -> Hashtbl.remove changed entry.path; Hashtbl.remove digest_cache entry.path; + any_changed := true; None | Some (Added | Modified) -> ( Hashtbl.remove changed entry.path; try let stat = Unix.stat entry.path in let digest = hash_and_cache digest_cache entry.path stat in - Some + let updated = { path = entry.path; state = @@ -346,17 +350,24 @@ let update_entries digest_cache previous changes = digest; }; } + in + if not (entry_equal updated entry) then any_changed := true; + Some updated with | Unix.Unix_error ((Unix.ENOENT | Unix.ENOTDIR), _, _) -> Hashtbl.remove digest_cache entry.path; + any_changed := true; None | Unix.Unix_error _ | Sys_error _ -> (* Preserve the old baseline so the caller still runs the build. Source diagnostics then remain inside the watcher's recoverable build boundary instead of terminating the watch loop here. *) + any_changed := true; Some entry)) in - if Hashtbl.length changed = 0 then Some updated else None + if Hashtbl.length changed > 0 then Not_in_snapshot + else if !any_changed then Updated updated + else Unchanged let changes_are_incremental changes = changes <> [] diff --git a/rewatch-ocaml/watch_snapshot.mli b/rewatch-ocaml/watch_snapshot.mli index 3184ac7ee5..c049fe6cf4 100644 --- a/rewatch-ocaml/watch_snapshot.mli +++ b/rewatch-ocaml/watch_snapshot.mli @@ -47,8 +47,14 @@ val create_with_symlink_paths : digest_cache -> Watch_scope.t -> registration val changes_between : entry list -> entry list -> change list -val update_entries : - digest_cache -> entry list -> change list -> entry list option +(* The result of re-reading the files a native watcher reported. A platform + can report one write more than once (macOS FSEvents may repeat it after + the drain window), so [Unchanged] means every file still matches its + entry and no build is needed. [Not_in_snapshot] means a reported path has + no entry and the snapshot must be reconciled. *) +type entries_update = Unchanged | Updated of entry list | Not_in_snapshot + +val update_entries : digest_cache -> entry list -> change list -> entries_update val changes_are_incremental : change list -> bool diff --git a/rewatch-ocaml/watcher.ml b/rewatch-ocaml/watcher.ml index 350a51869c..de0fd5006c 100644 --- a/rewatch-ocaml/watcher.ml +++ b/rewatch-ocaml/watcher.ml @@ -223,12 +223,13 @@ let run_locked ~native_create ~report_native_fallback ~root ~prod ~features | Some [] -> native_loop watcher scope symlink_targets previous | Some changes -> ( match Watch_snapshot.update_entries digest_cache previous changes with - | Some before_build -> + | Updated before_build -> Output.debug ~verbosity "doing Incremental"; begin_rebuild Incremental; build ~poll ~changes:(Some changes) |> finish_rebuild; native_loop watcher scope symlink_targets before_build - | None -> native_reconcile watcher scope previous) + | Unchanged -> native_loop watcher scope symlink_targets previous + | Not_in_snapshot -> native_reconcile watcher scope previous) | None -> native_reconcile watcher scope previous) and native_reconcile watcher (scope : Watch_scope.t) previous = let current = Watch_snapshot.create digest_cache scope in diff --git a/tests/rewatch_ounit_tests/watcher_tests.ml b/tests/rewatch_ounit_tests/watcher_tests.ml index 6c697fc48f..96f0d98997 100644 --- a/tests/rewatch_ounit_tests/watcher_tests.ml +++ b/tests/rewatch_ounit_tests/watcher_tests.ml @@ -172,26 +172,36 @@ let tests = }; ] in - Test_support.write_file source "let value = 2\n"; + let modified = [Watch_snapshot.{path = source; kind = Modified}] in check - (Option.is_some - (Watch_snapshot.update_entries digest_cache previous - [Watch_snapshot.{path = source; kind = Modified}])) + (Watch_snapshot.update_entries digest_cache previous modified + = Unchanged) + "a repeated event for unchanged content needs no rebuild"; + Test_support.write_file source "let value = 2\n"; + let updated = + match Watch_snapshot.update_entries digest_cache previous modified with + | Updated entries -> Some entries + | Unchanged | Not_in_snapshot -> None + in + check (Option.is_some updated) "known content events update the retained snapshot"; check - (Option.is_none - (Watch_snapshot.update_entries digest_cache previous - [ - Watch_snapshot. - {path = Filename.concat root "B.res"; kind = Added}; - ])) + (match updated with + | Some entries -> + Watch_snapshot.update_entries digest_cache entries modified + = Unchanged + | None -> false) + "a repeated event after the update needs no rebuild"; + check + (Watch_snapshot.update_entries digest_cache previous + [Watch_snapshot.{path = Filename.concat root "B.res"; kind = Added}] + = Not_in_snapshot) "unknown content events request structural reconciliation"; Sys.remove source; Unix.mkdir source 0o755; check - (Option.is_some - (Watch_snapshot.update_entries digest_cache previous - [Watch_snapshot.{path = source; kind = Modified}])) + (Watch_snapshot.update_entries digest_cache previous modified + = Updated previous) "a transient snapshot read failure preserves the rebuild baseline"); Test_support.with_temp_dir "rewatch-snapshot-racy-" (fun root -> let root = Unix.realpath root in