From 3ddecff4fe6f62216d902a9a1e271ac33190876e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20R=C3=BC=C3=9Fler?= Date: Mon, 17 Apr 2023 18:32:56 +0200 Subject: [PATCH 1/3] 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 --- CHANGELOG.md | 1 + src/args.rs | 10 ++++++++++ src/main.rs | 53 +++++++++++++++++++++++++++++++++++++++++----------- 3 files changed, 53 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e885be8f9a..4ba39b96d1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,6 +44,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/src/args.rs b/src/args.rs index 9689164a51..69baca91cd 100644 --- a/src/args.rs +++ b/src/args.rs @@ -15,6 +15,7 @@ use std::{ pub struct CliArgs { pub theme: PathBuf, pub repo_path: RepoPath, + pub notify_watcher: bool, pub poll_watcher: bool, } @@ -54,11 +55,14 @@ pub fn process_cmdline() -> Result { get_app_config_path()?.join("theme.ron") }; + let notify_watcher: bool = + *arg_matches.get_one("watcher").unwrap_or(&false); let arg_poll: bool = *arg_matches.get_one("poll").unwrap_or(&false); Ok(CliArgs { theme, + notify_watcher, poll_watcher: arg_poll, repo_path, }) @@ -95,6 +99,12 @@ fn app() -> ClapApp { .long("logging") .num_args(0), ) + .arg( + Arg::new("watcher") + .help("Use notify-based file system watcher instead of tick-based update. This is more performant, but can cause issues in larger repositories") + .long("watcher") + .action(clap::ArgAction::SetTrue), + ) .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") diff --git a/src/main.rs b/src/main.rs index 2faa4f0317..25f8012917 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 tui::{ 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,13 @@ pub enum AsyncNotification { Git(AsyncGitNotification), } +#[derive(Clone, Copy, PartialEq)] +enum Updater { + Ticker, + PollWatcher, + NotifyWatcher, +} + fn main() -> Result<()> { let app_start = Instant::now(); @@ -145,6 +154,13 @@ fn main() -> Result<()> { let mut repo_path = cliargs.repo_path; let input = Input::new(); + let updater = match (cliargs.notify_watcher, cliargs.poll_watcher) + { + (true, true) => Updater::PollWatcher, + (true, false) => Updater::NotifyWatcher, + _ => Updater::Ticker, + }; + loop { let quit_state = run_app( app_start, @@ -152,7 +168,7 @@ fn main() -> Result<()> { theme, key_config.clone(), &input, - cliargs.poll_watcher, + updater, &mut terminal, )?; @@ -173,18 +189,27 @@ 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 | Updater::PollWatcher => { + let poll_watcher = updater == Updater::PollWatcher; + let repo_watcher = RepoWatcher::new( + repo_work_dir(&repo)?.as_str(), + poll_watcher, + ); + + (never(), repo_watcher.receiver()) + } + Updater::Ticker => (tick(TICK_INTERVAL), never()), + }; + let spinner_ticker = tick(SPINNER_INTERVAL); let mut app = App::new( @@ -210,6 +235,7 @@ fn run_app( &rx_input, &rx_git, &rx_app, + &rx_ticker, &rx_watcher, &spinner_ticker, )? @@ -235,7 +261,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 +337,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 +346,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 +361,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"), }?; From 00f1f8c32bf869c2459a234e136585d5fa3cb462 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20R=C3=BC=C3=9Fler?= Date: Sat, 29 Apr 2023 16:14:24 +0200 Subject: [PATCH 2/3] Add FAQ entry for --watcher --- FAQ.md | 5 +++++ src/args.rs | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/FAQ.md b/FAQ.md index d585c996f4..8711759dee 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 69baca91cd..49f7f211a7 100644 --- a/src/args.rs +++ b/src/args.rs @@ -101,7 +101,7 @@ fn app() -> ClapApp { ) .arg( Arg::new("watcher") - .help("Use notify-based file system watcher instead of tick-based update. This is more performant, but can cause issues in larger repositories") + .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), ) From 73d54bf9c351e20fbf6e2b77f18116b46c7f7fc3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20R=C3=BC=C3=9Fler?= Date: Sat, 29 Apr 2023 16:16:07 +0200 Subject: [PATCH 3/3] Remove --poll --- src/args.rs | 12 +----------- src/main.rs | 19 +++++++------------ src/watcher.rs | 47 +++++++++++++---------------------------------- 3 files changed, 21 insertions(+), 57 deletions(-) diff --git a/src/args.rs b/src/args.rs index 49f7f211a7..7dd3853c0e 100644 --- a/src/args.rs +++ b/src/args.rs @@ -16,7 +16,6 @@ pub struct CliArgs { pub theme: PathBuf, pub repo_path: RepoPath, pub notify_watcher: bool, - pub poll_watcher: bool, } pub fn process_cmdline() -> Result { @@ -57,14 +56,11 @@ pub fn process_cmdline() -> Result { let notify_watcher: bool = *arg_matches.get_one("watcher").unwrap_or(&false); - let arg_poll: bool = - *arg_matches.get_one("poll").unwrap_or(&false); Ok(CliArgs { theme, - notify_watcher, - poll_watcher: arg_poll, repo_path, + notify_watcher, }) } @@ -105,12 +101,6 @@ fn app() -> ClapApp { .long("watcher") .action(clap::ArgAction::SetTrue), ) - .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") - .action(clap::ArgAction::SetTrue), - ) .arg( Arg::new("bugreport") .help("Generate a bug report") diff --git a/src/main.rs b/src/main.rs index 25f8012917..cd1e0819f3 100644 --- a/src/main.rs +++ b/src/main.rs @@ -119,7 +119,6 @@ pub enum AsyncNotification { #[derive(Clone, Copy, PartialEq)] enum Updater { Ticker, - PollWatcher, NotifyWatcher, } @@ -154,11 +153,10 @@ fn main() -> Result<()> { let mut repo_path = cliargs.repo_path; let input = Input::new(); - let updater = match (cliargs.notify_watcher, cliargs.poll_watcher) - { - (true, true) => Updater::PollWatcher, - (true, false) => Updater::NotifyWatcher, - _ => Updater::Ticker, + let updater = if cliargs.notify_watcher { + Updater::NotifyWatcher + } else { + Updater::Ticker }; loop { @@ -198,12 +196,9 @@ fn run_app( let rx_input = input.receiver(); let (rx_ticker, rx_watcher) = match updater { - Updater::NotifyWatcher | Updater::PollWatcher => { - let poll_watcher = updater == Updater::PollWatcher; - let repo_watcher = RepoWatcher::new( - repo_work_dir(&repo)?.as_str(), - poll_watcher, - ); + Updater::NotifyWatcher => { + let repo_watcher = + RepoWatcher::new(repo_work_dir(&repo)?.as_str()); (never(), repo_watcher.receiver()) } diff --git a/src/watcher.rs b/src/watcher.rs index ee0c3aa070..42fe8dcfae 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"); - - 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); - }; + 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); }