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.

#1444
#1310

* Add FAQ entry for --watcher

* Remove --poll

Author: cruessler

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`)

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. <a name="watcher"></a> Watching for changes <small><sup>[Top ▲](#table-of-contents)</sup></small>
+
+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.

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<CliArgs> {
@@ -54,13 +54,13 @@ pub fn process_cmdline() -> Result<CliArgs> {
 		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(

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,14 +153,20 @@ 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,
 			repo_path.clone(),
 			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<CrosstermBackend<io::Stdout>>,
 ) -> Result<QuitState, anyhow::Error> {
 	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<InputEvent>,
 	rx_git: &Receiver<AsyncGitNotification>,
 	rx_app: &Receiver<AsyncAppNotification>,
+	rx_ticker: &Receiver<Instant>,
 	rx_notify: &Receiver<()>,
 	rx_spinner: &Receiver<Instant>,
 ) -> Result<QueueEvent> {
@@ -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"),
 	}?;
 

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<DebouncedEvent>, Vec<Error>>,
@@ -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);
 }