From 370aff5fcc316bf381c650942065c06ffb5eb76e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20R=C3=BC=C3=9Fler?= Date: Sat, 29 Apr 2023 17:03:43 +0200 Subject: [PATCH] Default to tick-based updates (#1657) * Default to tick-based updates This commit reintroduces code that was previously removed in favor of a notify-based update trigger. It turned out that notify-based updates can cause issues in larger repositories, so tick-based updates seemed like a safer default. https://github.com/extrawurst/gitui/issues/1444 https://github.com/extrawurst/gitui/pull/1310 * Add FAQ entry for --watcher * Remove --poll --- CHANGELOG.md | 1 + FAQ.md | 5 +++++ src/args.rs | 14 +++++++------- src/main.rs | 48 +++++++++++++++++++++++++++++++++++++----------- src/watcher.rs | 45 ++++++++++++--------------------------------- 5 files changed, 62 insertions(+), 51 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 544e43e1..5f5ab8c7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -49,6 +49,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * commit hooks report "command not found" on Windows with wsl2 installed ([#1528](https://github.com/extrawurst/gitui/issues/1528)) * crashes on entering submodules ([#1510](https://github.com/extrawurst/gitui/issues/1510)) * fix race issue: revlog messages sometimes appear empty ([#1473](https://github.com/extrawurst/gitui/issues/1473)) +* default to tick-based updates [[@cruessler](https://github.com/cruessler)] ([#1444](https://github.com/extrawurst/gitui/issues/1444)) ### Changed * minimum supported rust version bumped to 1.64 (thank you `clap`) diff --git a/FAQ.md b/FAQ.md index d585c996..8711759d 100644 --- a/FAQ.md +++ b/FAQ.md @@ -18,3 +18,8 @@ Note that in some cases adding the line `ssh-add -K ~/.ssh/id_ed25519`(or whatev If you want to use `vi`-style keys or customize your key bindings in any other fassion see the specific docs on that: [key config](./KEY_CONFIG.md) +## 3. Watching for changes [Top ▲](#table-of-contents) + +By default, `gitui` polls for changes in the working directory every 5 seconds. If you supply `--watcher` as an argument, it uses a `notify`-based approach instead. This is usually faster and was for some time the default update strategy. It turned out, however, that `notify`-based updates can cause issues on some platforms, so tick-based updates seemed like a safer default. + +See #1444 for details. diff --git a/src/args.rs b/src/args.rs index 9689164a..7dd3853c 100644 --- a/src/args.rs +++ b/src/args.rs @@ -15,7 +15,7 @@ use std::{ pub struct CliArgs { pub theme: PathBuf, pub repo_path: RepoPath, - pub poll_watcher: bool, + pub notify_watcher: bool, } pub fn process_cmdline() -> Result { @@ -54,13 +54,13 @@ pub fn process_cmdline() -> Result { get_app_config_path()?.join("theme.ron") }; - let arg_poll: bool = - *arg_matches.get_one("poll").unwrap_or(&false); + let notify_watcher: bool = + *arg_matches.get_one("watcher").unwrap_or(&false); Ok(CliArgs { theme, - poll_watcher: arg_poll, repo_path, + notify_watcher, }) } @@ -96,9 +96,9 @@ fn app() -> ClapApp { .num_args(0), ) .arg( - Arg::new("poll") - .help("Poll folder for changes instead of using file system events. This can be useful if you run into issues with maximum # of file descriptors") - .long("polling") + Arg::new("watcher") + .help("Use notify-based file system watcher instead of tick-based update. This is more performant, but can cause issues on some platforms. See https://github.com/extrawurst/gitui/blob/master/FAQ.md#watcher for details.") + .long("watcher") .action(clap::ArgAction::SetTrue), ) .arg( diff --git a/src/main.rs b/src/main.rs index 26e4b5aa..b1c0ed1f 100644 --- a/src/main.rs +++ b/src/main.rs @@ -56,7 +56,7 @@ use asyncgit::{ AsyncGitNotification, }; use backtrace::Backtrace; -use crossbeam_channel::{tick, unbounded, Receiver, Select}; +use crossbeam_channel::{never, tick, unbounded, Receiver, Select}; use crossterm::{ terminal::{ disable_raw_mode, enable_raw_mode, EnterAlternateScreen, @@ -83,11 +83,13 @@ use std::{ use ui::style::Theme; use watcher::RepoWatcher; +static TICK_INTERVAL: Duration = Duration::from_secs(5); static SPINNER_INTERVAL: Duration = Duration::from_millis(80); /// #[derive(Clone)] pub enum QueueEvent { + Tick, Notify, SpinnerUpdate, AsyncEvent(AsyncNotification), @@ -114,6 +116,12 @@ pub enum AsyncNotification { Git(AsyncGitNotification), } +#[derive(Clone, Copy, PartialEq)] +enum Updater { + Ticker, + NotifyWatcher, +} + fn main() -> Result<()> { let app_start = Instant::now(); @@ -145,6 +153,12 @@ fn main() -> Result<()> { let mut repo_path = cliargs.repo_path; let input = Input::new(); + let updater = if cliargs.notify_watcher { + Updater::NotifyWatcher + } else { + Updater::Ticker + }; + loop { let quit_state = run_app( app_start, @@ -152,7 +166,7 @@ fn main() -> Result<()> { theme, key_config.clone(), &input, - cliargs.poll_watcher, + updater, &mut terminal, )?; @@ -173,18 +187,24 @@ fn run_app( theme: Theme, key_config: KeyConfig, input: &Input, - poll_watcher: bool, + updater: Updater, terminal: &mut Terminal>, ) -> Result { let (tx_git, rx_git) = unbounded(); let (tx_app, rx_app) = unbounded(); let rx_input = input.receiver(); - let watcher = RepoWatcher::new( - repo_work_dir(&repo)?.as_str(), - poll_watcher, - ); - let rx_watcher = watcher.receiver(); + + let (rx_ticker, rx_watcher) = match updater { + Updater::NotifyWatcher => { + let repo_watcher = + RepoWatcher::new(repo_work_dir(&repo)?.as_str()); + + (never(), repo_watcher.receiver()) + } + Updater::Ticker => (tick(TICK_INTERVAL), never()), + }; + let spinner_ticker = tick(SPINNER_INTERVAL); let mut app = App::new( @@ -210,6 +230,7 @@ fn run_app( &rx_input, &rx_git, &rx_app, + &rx_ticker, &rx_watcher, &spinner_ticker, )? @@ -235,7 +256,9 @@ fn run_app( } app.event(ev)?; } - QueueEvent::Notify => app.update()?, + QueueEvent::Tick | QueueEvent::Notify => { + app.update()?; + } QueueEvent::AsyncEvent(ev) => { if !matches!( ev, @@ -309,6 +332,7 @@ fn select_event( rx_input: &Receiver, rx_git: &Receiver, rx_app: &Receiver, + rx_ticker: &Receiver, rx_notify: &Receiver<()>, rx_spinner: &Receiver, ) -> Result { @@ -317,6 +341,7 @@ fn select_event( sel.recv(rx_input); sel.recv(rx_git); sel.recv(rx_app); + sel.recv(rx_ticker); sel.recv(rx_notify); sel.recv(rx_spinner); @@ -331,8 +356,9 @@ fn select_event( 2 => oper.recv(rx_app).map(|e| { QueueEvent::AsyncEvent(AsyncNotification::App(e)) }), - 3 => oper.recv(rx_notify).map(|_| QueueEvent::Notify), - 4 => oper.recv(rx_spinner).map(|_| QueueEvent::SpinnerUpdate), + 3 => oper.recv(rx_ticker).map(|_| QueueEvent::Notify), + 4 => oper.recv(rx_notify).map(|_| QueueEvent::Notify), + 5 => oper.recv(rx_spinner).map(|_| QueueEvent::SpinnerUpdate), _ => bail!("unknown select source"), }?; diff --git a/src/watcher.rs b/src/watcher.rs index ee0c3aa0..42fe8dcf 100644 --- a/src/watcher.rs +++ b/src/watcher.rs @@ -1,12 +1,7 @@ use anyhow::Result; use crossbeam_channel::{unbounded, Sender}; -use notify::{ - Config, Error, PollWatcher, RecommendedWatcher, RecursiveMode, - Watcher, -}; -use notify_debouncer_mini::{ - new_debouncer, new_debouncer_opt, DebouncedEvent, -}; +use notify::{Error, RecommendedWatcher, RecursiveMode, Watcher}; +use notify_debouncer_mini::{new_debouncer, DebouncedEvent}; use scopetime::scope_time; use std::{path::Path, thread, time::Duration}; @@ -15,9 +10,9 @@ pub struct RepoWatcher { } impl RepoWatcher { - pub fn new(workdir: &str, poll: bool) -> Self { + pub fn new(workdir: &str) -> Self { log::trace!( - "poll watcher: {poll} recommended: {:?}", + "recommended watcher: {:?}", RecommendedWatcher::kind() ); @@ -27,7 +22,7 @@ impl RepoWatcher { thread::spawn(move || { let timeout = Duration::from_secs(2); - create_watcher(poll, timeout, tx, &workdir); + create_watcher(timeout, tx, &workdir); }); let (out_tx, out_rx) = unbounded(); @@ -72,7 +67,6 @@ impl RepoWatcher { } fn create_watcher( - poll: bool, timeout: Duration, tx: std::sync::mpsc::Sender< Result, Vec>, @@ -81,27 +75,12 @@ fn create_watcher( ) { scope_time!("create_watcher"); - if poll { - let config = Config::default() - .with_poll_interval(Duration::from_secs(2)); - let mut bouncer = new_debouncer_opt::<_, PollWatcher>( - timeout, None, tx, config, - ) - .expect("Watch create error"); - bouncer - .watcher() - .watch(Path::new(&workdir), RecursiveMode::Recursive) - .expect("Watch error"); + let mut bouncer = + new_debouncer(timeout, None, tx).expect("Watch create error"); + bouncer + .watcher() + .watch(Path::new(&workdir), RecursiveMode::Recursive) + .expect("Watch error"); - std::mem::forget(bouncer); - } else { - let mut bouncer = new_debouncer(timeout, None, tx) - .expect("Watch create error"); - bouncer - .watcher() - .watch(Path::new(&workdir), RecursiveMode::Recursive) - .expect("Watch error"); - - std::mem::forget(bouncer); - }; + std::mem::forget(bouncer); }