Repository navigation
Conversation
649e9b7 to
ffe1bd4
Compare
ffe1bd4 to
422e8df
Compare
![ultimator-ai[bot]](https://avatars.githubusercontent.com/u/340985799?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ultimator's review of 422e8df
Replaces one-shot reads of the debug watch set with a Condvar-backed debug_wait_worktree_watches that the engine notifies on every publish, and uses it in the two git engine tests that raced the follow-up reconcile pass. The wait checks its predicate before blocking and under the same mutex the engine writes through, so no notification is lost, and the test helper compares canonical paths the same way worktree_watches does. I found no defects. I couldn't read the CI results for 422e8df from here, so they are unverified.
No issues found.
Reviewed in 1m 9s · Session · @ultimator-ai review reviews again · @ultimator-ai fix fixes the open issues
A status snapshot can reflect an index written after its pass reconciled the watch set; the set catches up on the pass that index event triggers. Tests that asserted the set right after the snapshot raced that pass. The debug hook now signals each published set, and the tests wait on it.
422e8df to
4ca6369
Compare
There was a problem hiding this comment.
Ultimator's review of 4ca6369
Round 2 (4ca6369, force-pushed): same change as 422e8df, except debug_wait_worktree_watches now blocks with wait_while and no timeout. The lost-wakeup reasoning still holds. But a watch that is never armed or disarmed now hangs the test until the CI job's own timeout, instead of failing within 5 s as the old inline loop and 422e8df did. The description still says the helper waits "for up to 5 s". CI for 4ca6369 has just restarted and has no results yet.
1 issue: 1 nit
Open issues: check the ones to fix, then Fix.
- ⚪
#1Watch wait lost its timeout: a regression hangs CI ·crates/git/src/state.rs:232-236 - 🔧 Fix the checked issues (all of them if none is checked)
Reviewed in 18s · Session · @ultimator-ai review reviews again · @ultimator-ai fix fixes the open issues
| pub fn debug_wait_worktree_watches(gitdir: &Path, mut done: impl FnMut(&[PathBuf]) -> bool) { | ||
| let (sets, published) = worktree_watch_sets(); | ||
| drop( | ||
| published | ||
| .wait_while(sets.lock().unwrap(), |sets| { |
There was a problem hiding this comment.
⚪ Nit · Watch wait lost its timeout: a regression hangs CI
422e8df used wait_timeout_while with the caller's 5 s, and the polling loop this replaces in reattached_status_selection_observes_changes_made_while_pruned asserted after 5 s. wait_while has no deadline. If the engine never publishes a matching set (the regression these tests exist to catch), wait_for_watch blocks forever. The test binary then sits until the CI job is killed, with no failing assertion or test name, and the Windows job is the one that flakes. The PR description still says it waits "for up to 5 s".
Ultimator · issue #1 · reply @ultimator-ai fix 1 to have it fixed
This branch has not been deployed
1 participant
index_changes_reconcile_tracked_exceptions_to_ignore_pruningfails intermittently on Windows CI, atassert!(watches().contains(.../generated/nested)).The engine reconciles its worktree watches before it cuts a snapshot. If
git add -ffinishes writing the index between those two steps, the snapshot already shows the file as added while the watch set still predates it, and the next pass arms the directory. The test read the debug watch set once, right after that snapshot.A new
Repository::wait_for_watchhelper polls the watch set for up to 5 s. This test now waits for the watch to be armed before writing intogenerated/nested, and for it to be disarmed aftergit rm --cached.reattached_status_selection_observes_changes_made_while_prunedalready did the same wait inline, so it now uses the helper too. Test-only change.Tag
@indentto continue the conversation here.