Improved Monitor interface#303
Open
sourcefrog wants to merge 1 commit into
Open
Conversation
* Send stdout println through Monitor so it can manage progress bar synchronization internally * Pass around just `Monitor` and hold an Arc internally, like `Transport` already does * impl Clone for Counters so tests can get a copy of them * pass Monitor into a couple more places
There was a problem hiding this comment.
Pull request overview
This PR refactors the monitoring API by introducing a Monitor wrapper type (internally Arc-backed) around a MonitorImpl trait, and routes stdout output through the monitor so the terminal UI can synchronize progress bars with printed output. The change propagates throughout library APIs and tests, replacing Arc<dyn Monitor> / TestMonitor usage with the new Monitor/Collector patterns.
Changes:
- Replace
Arc<dyn Monitor>parameters across the crate with a clonableMonitorwrapper (backed byArc<dyn MonitorImpl>). - Add
Monitor::println()and update CLI/library call sites to print through the monitor for progress-bar safety. - Rename test monitor to
Collector, addCounters: Clone, and update tests to useMonitor::void()/Monitor::for_test().
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/proptest_changes.rs | Switch tests from TestMonitor to Monitor::void(). |
| tests/old_archives.rs | Update old-archive tests to use Monitor::void() and Monitor::for_test() collectors. |
| tests/mount.rs | Update mount tests to use Monitor::void() (but currently missing import / unused import issue). |
| tests/damage.rs | Update damage tests to use Monitor and collectors, remove Arc<TestMonitor>. |
| src/validate.rs | Change validation helpers to accept Monitor instead of Arc<dyn Monitor>. |
| src/test_fixtures.rs | Update fixtures to use Monitor::void() for helper backups. |
| src/termui/trace.rs | Adjust tracing setup to take &TermUiMonitor and write via its view. |
| src/termui/monitor.rs | Implement MonitorImpl for TermUiMonitor, add println() integration and a custom Debug. |
| src/stored_tree.rs | Update stored tree APIs to accept Monitor and tests to use Monitor::void(). |
| src/source.rs | Update source tree sizing/iteration APIs to accept Monitor. |
| src/show.rs | Route version listing output through monitor.println(). |
| src/restore.rs | Update restore APIs to accept Monitor, migrate tests to Monitor::for_test() collectors. |
| src/monitor/void.rs | Replace VoidMonitor with VoidMonitorImpl implementing MonitorImpl, add counters/error_count/println. |
| src/monitor/test.rs | Rename TestMonitor to Collector and implement MonitorImpl. |
| src/monitor/task.rs | Derive Debug for TaskList to satisfy MonitorImpl: Debug. |
| src/monitor/mod.rs | Remove the old monitor module file in favor of src/monitor.rs. |
| src/monitor.rs | Introduce new Monitor wrapper + MonitorImpl trait and helper constructors. |
| src/merge.rs | Update commented-out test scaffolding to reference Monitor::void(). |
| src/index/write.rs | Update IndexWriter to store a Monitor directly. |
| src/index/stitch.rs | Update stitcher to carry Monitor; adjust tests (currently has ineffective counter assertions for b1/b2). |
| src/index/mod.rs | Update index tests to use Monitor::void() / Monitor::for_test(). |
| src/gc_lock.rs | Update gc lock tests to use Monitor::void(). |
| src/diff.rs | Update diff API and tests to accept Monitor. |
| src/counters.rs | Add Clone impl for Counters (snapshot clone of atomics). |
| src/blockdir.rs | Update blockdir APIs and tests to use Monitor and collectors. |
| src/bin/conserve.rs | Wrap TermUiMonitor in Monitor, route stdout output via monitor.println(), propagate monitor into callbacks. |
| src/band.rs | Update band APIs (index_writer, validate) to accept Monitor. |
| src/backup.rs | Update backup APIs to accept Monitor and adjust internal plumbing/tests. |
| src/archive.rs | Update archive APIs (validate, block listing/deletion) to accept Monitor. |
| doc/design.md | Minor documentation wording fix about tracing/log options. |
Comments suppressed due to low confidence (1)
src/index/stitch.rs:296
- This section uses a
Voidmonitor for b1, but then asserts oncollectorfrom the earlier b0 monitor. That assertion can never observe b1’s writes, so it isn’t testing what it claims.
let monitor = Monitor::void();
let band = Band::create(&af).await?;
assert_eq!(band.id().to_string(), "b0001");
let mut ib = band.index_writer(monitor.clone());
ib.push_entry(symlink("/0", "b1"));
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
9
to
11
| use conserve::{ | ||
| BackupOptions, MountOptions, backup, monitor::test::TestMonitor, test_fixtures::TreeFixture, | ||
| BackupOptions, MountOptions, backup, monitor::test::Collector, test_fixtures::TreeFixture, | ||
| }; |
Comment on lines
+10
to
+14
| /// A monitor that does not capture any information aside from the error count and counters. | ||
| /// | ||
| /// This is convenient for use in tests that don't care about checking for side effects: | ||
| /// but most tests actually should use [`conserve::monitor::Monitor::for_tests`] and make | ||
| /// assertions about the result. |
Comment on lines
+32
to
+35
| /// Make a new void monitor that ignores all events. | ||
| pub fn void() -> Self { | ||
| Self::new(Arc::new(void::VoidMonitorImpl::new())) | ||
| } |
Comment on lines
34
to
+36
| /// Construct a new TestMonitor and wrap it in an Arc. | ||
| pub fn arc() -> Arc<TestMonitor> { | ||
| Arc::new(TestMonitor::new()) | ||
| pub fn arc() -> Arc<Collector> { | ||
| Arc::new(Collector::new()) |
Comment on lines
306
to
310
| // b2 | ||
| let monitor = TestMonitor::arc(); | ||
| let monitor = Monitor::void(); | ||
| let band = Band::create(&af).await?; | ||
| assert_eq!(band.id().to_string(), "b0002"); | ||
| let mut ib = band.index_writer(monitor.clone()); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Monitorand hold an Arc internally, likeTransportalready does