Repository: Canop/bacon (language: rust)

Issue:
vim double-triggers bacon v2.21.0 when saving a file
When saving a file in vim (or neovim), vim will write to multiple files in the directory of the saved file. Here is a log for saving `src/main.rs` in neovim:

```console
$ inotifywait --monitor src
Setting up watches.
Watches established.
src/ CREATE 4913
src/ OPEN 4913
src/ ATTRIB 4913
src/ CLOSE_WRITE,CLOSE 4913
src/ DELETE 4913
src/ MOVED_FROM main.rs
src/ MOVED_TO main.rs~
src/ CREATE main.rs
src/ OPEN main.rs
src/ MODIFY main.rs
src/ CLOSE_WRITE,CLOSE main.rs
src/ ATTRIB main.rs
src/ DELETE main.rs~
```

(Note that this doesn’t happen on `tmpfs` filesystems, there neovim directly writes the file. I can reproduce the shown behaviour on `btrfs` and `ext4`; I don’t have any other filesystems at hand.)

This means that with the new default for `on_change_strategy` introduced in #207, each save will run the current job (e. g. running the tests) twice. The old behaviour works well, so feel free to ignore this issue, but it might be nice to avoid retriggering anything in this situation (maybe by adding a timeout of a couple milliseconds?).


Discussion / hints:
It looks like the problem you have is related to the write strategy of your vim installation. 

You should be able to fix this by adding this in you (n)vim config:

    set nobackup
This is not really a vim-specific problem, many editors write backup files. Just a quick test with some editors (default configurations):
* Writes backup file/double-triggers bacon
  * vim/nvim
  * helix
  * gedit
  * kate
  * geany
  * Qt Creator
* Does not double-trigger bacon
  * vi
  * nano
  * kakoune
  * zed
  
`set nobackup` does not work around the issue for me (the output of `inotifywait` is not changed. Actually, upon reading the docs, `nobackup` is the default), but `set writebackup` does. However, it is documented with
```
        WARNING: Switching this option off means that when Vim fails to write
        your buffer correctly and then, for whatever reason, Vim exits, you
        lose both the original file and what you were writing.  Only reset
        this option if your file system is almost full and it makes the write
        fail (and make sure not to exit Vim until the write was successful).
        See backup-table for another explanation.
```
which I agree with and which is probably why the other editors behave this way as well.
All those saves happen in less than 1 ms, right ?

I wonder if this couldn't be solved with
- having the task start with a 1ms wait
- disregarding inotify events occuring during this wait

Fix patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 13935ff..9686dc9 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -9,11 +9,12 @@ If you're running a test or nextest job and you want only the failing test to be
 If you want all tests to be executed again, hit `esc`.
 Fix #214
 #### Other features:
+- grace period (by default 5ms) after a file event before the real launch of the command and during which other file events may be disregarded. Helps when saving a file changes several ones (eg backup then rename).
 - new `exports` structure in configuration. New `analysis` export bound by default to `ctrl-e`. The old syntax defining locations export is still supported but won't appear in documentations anymore.
 - recognize panic location in test - Fix #208
 - lines to ignore can be specified as a set of regular expressions in a `ignored_lines` field either in the job or at the top of the prefs or bacon.toml - Fix #223
 - `toggle-backtrace` accepts an optional level: `toggle-backtrace(1)` or `toggle-backtrace(full)` - Experimental - Fix #210
-- configuration can be passed in `BACON_PREFS` and `BACON_CONFIG` env vars - Fix #76
+- configuration paths can be passed in `BACON_PREFS` and `BACON_CONFIG` env vars - Fix #76
 
 <a name="v2.21.0"></a>
 ### v2.21.0 - 2024/09/14
diff --git a/defaults/default-prefs.toml b/defaults/default-prefs.toml
index c8d5cdd..856aba2 100644
--- a/defaults/default-prefs.toml
+++ b/defaults/default-prefs.toml
@@ -16,6 +16,14 @@
 #
 # reverse = true
 
+# The grace period is a delay after a file event before the real
+# task is launched and during which other events will be ignored.
+# This is useful if several events are often sent quasi-simultaneously
+# (eg your editor backups before saving, then renames).
+# You can set it to "none" if it's useless for you.
+#
+# grace_period = "5ms"
+
 # Uncomment and change the value (true/false) to
 # specify whether bacon should show a help line.
 #
diff --git a/src/app.rs b/src/app.rs
index 6749f7a..043b813 100644
--- a/src/app.rs
+++ b/src/app.rs
@@ -47,22 +47,22 @@ pub fn run(
             Ok(we) => {
                 match we.kind {
                     EventKind::Modify(ModifyKind::Metadata(_)) => {
-                        info!("ignoring metadata change");
+                        debug!("ignoring metadata change");
                         return; // useless event
                     }
                     EventKind::Modify(ModifyKind::Data(DataChange::Any)) => {
-                        info!("ignoring 'any' data change");
+                        debug!("ignoring 'any' data change");
                         return; // probably useless event with no real change
                     }
                     EventKind::Access(AccessKind::Close(AccessMode::Write)) => {
-                        info!("close write event: {we:?}");
+                        debug!("close write event: {we:?}");
                     }
                     EventKind::Access(_) => {
-                        info!("ignoring access event: {we:?}");
+                        debug!("ignoring access event: {we:?}");
                         return; // probably useless event
                     }
                     _ => {
-                        info!("notify event: {we:?}");
+                        debug!("notify event: {we:?}");
                     }
                 }
                 if let Some(ignorer) = ignorer.as_mut() {
@@ -104,6 +104,11 @@ pub fn run(
         let mut action: Option<&Action> = None;
         select! {
             recv(watch_receiver) -> _ => {
+                debug!("watch event received");
+                if task_executor.is_in_grace_period() {
+                    debug!("ignoring notify event in grace period");
+                    continue;
+                }
                 state.receive_watch_event();
                 if state.auto_refresh.is_enabled() {
                     if !state.is_computing() || on_change_strategy == OnChangeStrategy::KillThenRestart {
diff --git a/src/config.rs b/src/config.rs
index e5f46e3..0dce6de 100644
--- a/src/config.rs
+++ b/src/config.rs
@@ -54,6 +54,11 @@ pub struct Config {
     /// Patterns of lines which should be ignored. Patterns of
     /// the prefs or bacon.toml can be overridden at the job
     pub ignored_lines: Option<Vec<LinePattern>>,
+
+    /// The delay between a file event and the real start of the
+    /// task. Other file events occuring during this period will be
+    /// ignored.
+    pub grace_period: Option<Period>,
 }
 
 impl Config {
diff --git a/src/exec/command_builder.rs b/src/exec/command_builder.rs
new file mode 100644
index 0000000..cba12b4
--- /dev/null
+++ b/src/exec/command_builder.rs
@@ -0,0 +1,119 @@
+use std::{
+    collections::HashMap,
+    ffi::{
+        OsStr,
+        OsString,
+    },
+    path::{
+        Path,
+        PathBuf,
+    },
+    process::{
+        Command,
+        Stdio,
+    },
+};
+
+#[derive(Debug, Clone)]
+pub struct CommandBuilder {
+    exe: String,
+    current_dir: Option<PathBuf>,
+    args: Vec<OsString>,
+    with_stdout: bool,
+    envs: HashMap<OsString, OsString>,
+}
+
+impl CommandBuilder {
+    pub fn new(exe: &str) -> Self {
+        Self {
+            exe: exe.to_string(),
+            current_dir: None,
+            args: Vec::new(),
+            with_stdout: false,
+            envs: Default::default(),
+        }
+    }
+    pub fn build(&self) -> Command {
+        let mut command = Command::new(&self.exe);
+        if let Some(dir) = &self.current_dir {
+            command.current_dir(dir);
+        }
+        command.args(&self.args);
+        command.envs(&self.envs);
+        command
+            .envs(&self.envs)
+            .stdin(Stdio::null())
+            .stderr(Stdio::piped())
+            .stdout(if self.with_stdout {
+                Stdio::piped()
+            } else {
+                Stdio::null()
+            });
+        command
+    }
+    pub fn with_stdout(
+        &mut self,
+        b: bool,
+    ) -> &mut Self {
+        self.with_stdout = b;
+        self
+    }
+    pub fn is_with_stdout(&self) -> bool {
+        self.with_stdout
+    }
+    pub fn current_dir<P: AsRef<Path>>(
+        &mut self,
+        dir: P,
+    ) -> &mut Self {
+        self.current_dir = Some(dir.as_ref().to_path_buf());
+        self
+    }
+    pub fn arg<S: AsRef<OsStr>>(
+        &mut self,
+        arg: S,
+    ) -> &mut Self {
+        self.args.push(arg.as_ref().to_os_string());
+        self
+    }
+    pub fn args<I, S>(
+        &mut self,
+        args: I,
+    ) -> &mut Self
+    where
+        I: IntoIterator<Item = S>,
+        S: AsRef<OsStr>,
+    {
+        for arg in args {
+            self.args.push(arg.as_ref().to_os_string());
+        }
+        self
+    }
+    pub fn env<K, V>(
+        &mut self,
+        key: K,
+        val: V,
+    ) -> &mut Self
+    where
+        K: AsRef<OsStr>,
+        V: AsRef<OsStr>,
+    {
+        self.envs
+            .insert(key.as_ref().to_os_string(), val.as_ref().to_os_string());
+        self
+    }
+    pub fn envs<I, K, V>(
+        &mut self,
+        vars: I,
+    ) -> &mut Self
+    where
+        I: IntoIterator<Item = (K, V)>,
+        K: AsRef<OsStr>,
+        V: AsRef<OsStr>,
+    {
+        for (k, v) in vars {
+            self.envs
+                .insert(k.as_ref().to_os_string(), v.as_ref().to_os_string());
+        }
+        self
+    }
+}
diff --git a/src/executor.rs b/src/exec/executor.rs
similarity index 81%
rename from src/executor.rs
rename to src/exec/executor.rs
index ac5257d..bcbb84c 100644
--- a/src/executor.rs
+++ b/src/exec/executor.rs
@@ -1,9 +1,5 @@
 use {
     crate::*,
-    anyhow::{
-        Context,
-        Result,
-    },
     crossbeam::channel::{
         Receiver,
         Sender,
@@ -17,9 +13,9 @@ use {
         process::{
             Child,
             Command,
-            Stdio,
         },
         thread,
+        time::Instant,
     },
 };
 
@@ -28,10 +24,8 @@ use {
 /// and finishing by None.
 /// Channel sizes are designed to avoid useless computations.
 pub struct MissionExecutor {
-    command: Command,
+    command_builder: CommandBuilder,
     kill_command: Option<Vec<String>>,
-    /// whether it's necessary to transmit stdout lines
-    with_stdout: bool,
     line_sender: Sender<CommandExecInfo>,
     pub line_receiver: Receiver<CommandExecInfo>,
 }
@@ -42,6 +36,8 @@ pub struct TaskExecutor {
     /// the thread running the current task
     child_thread: thread::JoinHandle<()>,
     stop_sender: Sender<StopMessage>,
+    grace_period_start: Option<Instant>, // forgotten at end of grace period
+    grace_period: Period,
 }
 
 /// A message sent to the child_thread on end
@@ -51,12 +47,6 @@ enum StopMessage {
     Kill,       // kill the process, don't bother about the status
 }
 
-/// Settings for one execution of job's command
-#[derive(Debug, Clone, Copy, PartialEq, Default)]
-pub struct Task {
-    pub backtrace: Option<&'static str>,
-}
-
 impl TaskExecutor {
     /// Interrupt the process
     pub fn interrupt(self) {
@@ -71,27 +61,26 @@ impl TaskExecutor {
             warn!("child_thread.join() failed"); // should not happen
         }
     }
+    pub fn is_in_grace_period(&mut self) -> bool {
+        if let Some(grace_period_start) = self.grace_period_start {
+            if grace_period_start.elapsed() < self.grace_period.duration {
+                return true;
+            }
+            self.grace_period_start = None;
+        }
+        false
+    }
 }
 
 impl MissionExecutor {
     /// Prepare the executor (no task/process/thread is started at this point)
-    pub fn new(mission: &Mission) -> Result<Self> {
-        let mut command = mission.get_command();
+    pub fn new(mission: &Mission) -> anyhow::Result<Self> {
+        let command_builder = mission.get_command();
         let kill_command = mission.kill_command();
-        let with_stdout = mission.need_stdout();
         let (line_sender, line_receiver) = crossbeam::channel::unbounded();
-        command
-            .stdin(Stdio::null())
-            .stderr(Stdio::piped())
-            .stdout(if with_stdout {
-                Stdio::piped()
-            } else {
-                Stdio::null()
-            });
         Ok(Self {
-            command,
+            command_builder,
             kill_command,
-            with_stdout,
             line_sender,
             line_receiver,
         })
@@ -101,21 +90,40 @@ impl MissionExecutor {
     pub fn start(
         &mut self,
         task: Task,
-    ) -> Result<TaskExecutor> {
+    ) -> anyhow::Result<TaskExecutor> {
         info!("start task {task:?}");
-        let mut child = self
-            .command
-            .env("RUST_BACKTRACE", task.backtrace.unwrap_or("0"))
-            .spawn()
-            .context("failed to launch command")?;
+        let grace_period = task.grace_period;
+        let grace_period_start = if grace_period.is_zero() {
+            None
+        } else {
+            Some(Instant::now())
+        };
+        let mut command_builder = self.command_builder.clone();
+        command_builder.env("RUST_BACKTRACE", task.backtrace.unwrap_or("0"));
         let kill_command = self.kill_command.clone();
-        let with_stdout = self.with_stdout;
+        let with_stdout = command_builder.is_with_stdout();
         let line_sender = self.line_sender.clone();
         let (stop_sender, stop_receiver) = crossbeam::channel::bounded(1);
         let err_stop_sender = stop_sender.clone();
 
         // Global task executor thread
         let child_thread = thread::spawn(move || {
+            // before starting the command, we wait some time, so that a bunch
+            // of quasi-simultaneous file events can be finished before the command
+            // starts (during this time, no other command is started by bacon in app.rs)
+            if !grace_period.is_zero() {
+                thread::sleep(grace_period.duration);
+            }
+
+            let child = command_builder.build().spawn();
+            let mut child = match child {
+                Ok(child) => child,
+                Err(e) => {
+                    let _ = line_sender.send(CommandExecInfo::Error(e.to_string()));
+                    return;
+                }
+            };
+
             // thread piping the stdout lines
             if with_stdout {
                 let sender = line_sender.clone();
@@ -208,6 +216,8 @@ impl MissionExecutor {
         Ok(TaskExecutor {
             child_thread,
             stop_sender,
+            grace_period_start,
+            grace_period,
         })
     }
 }
diff --git a/src/exec/mod.rs b/src/exec/mod.rs
new file mode 100644
index 0000000..55163ff
--- /dev/null
+++ b/src/exec/mod.rs
@@ -0,0 +1,11 @@
+mod command_builder;
+mod executor;
+mod period;
+mod task;
+
+pub use {
+    command_builder::CommandBuilder,
+    executor::*,
+    period::*,
+    task::Task,
+};
diff --git a/src/exec/period.rs b/src/exec/period.rs
new file mode 100644
index 0000000..918b67c
--- /dev/null
+++ b/src/exec/period.rs
@@ -0,0 +1,57 @@
+use {
+    anyhow::anyhow,
+    lazy_regex::*,
+    serde::{
+        Deserialize,
+        Deserializer,
+        de,
+    },
+    std::{
+        str::FromStr,
+        time::Duration,
+    },
+};
+
+/// A small wrapper over time::Duration, to allow reading from a string in
+/// config. There's no symetric serialization and the input format is
+/// quite crude (eg "25ms" or "254ns" or "none")
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub struct Period {
+    pub duration: Duration,
+}
+
+impl Period {
+    pub const fn is_zero(&self) -> bool {
+        self.duration.is_zero()
+    }
+}
+
+impl From<Duration> for Period {
+    fn from(duration: Duration) -> Self {
+        Self { duration }
+    }
+}
+
+impl FromStr for Period {
+    type Err = anyhow::Error;
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        let duration = regex_switch!(s,
+            r"^(?<n>\d+)\s*ns$" => Duration::from_nanos(n.parse()?),
+            r"^(?<n>\d+)\s*ms$" => Duration::from_millis(n.parse()?),
+            r"^(?<n>\d+)\s*s$" => Duration::from_secs(n.parse()?),
+            r"^[^1-9]*$" => Duration::new(0, 0), // eg "none", "0", "off"
+        )
+        .ok_or_else(|| anyhow!("Invalid period: {}", s))?;
+        Ok(Self { duration })
+    }
+}
+
+impl<'de> Deserialize<'de> for Period {
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    where
+        D: Deserializer<'de>,
+    {
+        let s = String::deserialize(deserializer)?;
+        FromStr::from_str(&s).map_err(de::Error::custom)
+    }
+}
diff --git a/src/exec/task.rs b/src/exec/task.rs
new file mode 100644
index 0000000..2a5786b
--- /dev/null
+++ b/src/exec/task.rs
@@ -0,0 +1,8 @@
+use crate::Period;
+
+/// Settings for one execution of a job's command
+#[derive(Debug, Clone, Copy, PartialEq)]
+pub struct Task {
+    pub backtrace: Option<&'static str>, // ("1" or "full")
+    pub grace_period: Period,
+}
diff --git a/src/lib.rs b/src/lib.rs
index d6808a8..92cdc6f 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -10,7 +10,7 @@ mod config;
 mod defaults;
 mod drawing;
 mod examples;
-mod executor;
+mod exec;
 mod export;
 mod failure;
 mod help_line;
@@ -53,7 +53,7 @@ pub use {
     defaults::*,
     drawing::*,
     examples::*,
-    executor::*,
+    exec::*,
     export::*,
     failure::*,
     help_line::*,
diff --git a/src/mission.rs b/src/mission.rs
index 89b49b9..9c5790c 100644
--- a/src/mission.rs
+++ b/src/mission.rs
@@ -8,10 +8,7 @@ use {
         Watcher,
     },
     rustc_hash::FxHashSet,
-    std::{
-        path::PathBuf,
-        process::Command,
-    },
+    std::path::PathBuf,
 };
 
 static DEFAULT_WATCHES: &[&str] = &["src", "tests", "benches", "examples", "build.rs"];
@@ -145,7 +142,7 @@ impl<'s> Mission<'s> {
     }
 
     /// build (and doesn't call) the external cargo command
-    pub fn get_command(&self) -> Command {
+    pub fn get_command(&self) -> CommandBuilder {
         let mut command = if self.job.expand_env_vars {
             self.job
                 .command
@@ -181,11 +178,11 @@ impl<'s> Mission<'s> {
             }
         }
 
-        info!("command: {command:#?}");
         let mut tokens = command.iter();
-        let mut command = Command::new(
+        let mut command = CommandBuilder::new(
             tokens.next().unwrap(), // implies a check in the job
         );
+        command.with_stdout(self.need_stdout());
 
         if !self.job.extraneous_args {
             command.args(tokens);
@@ -200,7 +197,7 @@ impl<'s> Mission<'s> {
         let mut last_is_features = false;
         let mut tokens = tokens.chain(&self.settings.additional_job_args);
         let mut has_double_dash = false;
-        while let Some(arg) = tokens.next() {
+        for arg in tokens.by_ref() {
             if arg == "--" {
                 // we'll defer addition of the following arguments to after
                 // the addition of the features stuff, so that the features
@@ -270,7 +267,7 @@ impl<'s> Mission<'s> {
         }
         command.current_dir(&self.cargo_execution_directory);
         command.envs(&self.job.env);
-        debug!("command: {:#?}", &command);
+        debug!("command builder: {:#?}", &command);
         command
     }
 
diff --git a/src/settings.rs b/src/settings.rs
index 9ceb519..0cacb48 100644
--- a/src/settings.rs
+++ b/src/settings.rs
@@ -1,7 +1,10 @@
 use {
     crate::*,
     anyhow::*,
-    std::collections::HashMap,
+    std::{
+        collections::HashMap,
+        time::Duration,
+    },
 };
 
 /// The settings used in the application.
@@ -31,6 +34,7 @@ pub struct Settings {
     pub show_changes_count: bool,
     pub on_change_strategy: Option<OnChangeStrategy>,
     pub ignored_lines: Option<Vec<LinePattern>>,
+    pub grace_period: Period,
 }
 
 impl Default for Settings {
@@ -53,6 +57,7 @@ impl Default for Settings {
             show_changes_count: false,
             on_change_strategy: None,
             ignored_lines: Default::default(),
+            grace_period: Duration::from_millis(5).into(),
         }
     }
 }
@@ -103,6 +108,9 @@ impl Settings {
         if let Some(b) = config.ignored_lines.as_ref() {
             self.ignored_lines = Some(b.clone());
         }
+        if let Some(p) = config.grace_period {
+            self.grace_period = p;
+        }
     }
     pub fn apply_args(
         &mut self,
diff --git a/src/state.rs b/src/state.rs
index 9802a31..ae64d1e 100644
--- a/src/state.rs
+++ b/src/state.rs
@@ -141,6 +141,7 @@ impl<'s> AppState<'s> {
     pub fn new_task(&self) -> Task {
         Task {
             backtrace: self.backtrace,
+            grace_period: self.mission.settings.grace_period,
         }
     }
     pub fn take_output(&mut self) -> Option<CommandOutput> {
diff --git a/src/task.rs b/src/task.rs
new file mode 100644
index 0000000..e69de29


Test patch: