Repository: alacritty/alacritty
File: alacritty/src/config/monitor.rs (Rust)

Code before:
use std::sync::mpsc;
use std::time::Duration;

use log::{debug, error};
use notify::{watcher, DebouncedEvent, RecursiveMode, Watcher};

use alacritty_terminal::thread;

use crate::event::{Event, EventProxy};

#[cfg(any(target_os = "linux", target_os = "macos", target_os = "windows"))]
const DEBOUNCE_DELAY: Duration = Duration::from_millis(10);
#[cfg(not(any(target_os = "linux", target_os = "macos", target_os = "windows")))]
const DEBOUNCE_DELAY: Duration = Duration::from_millis(1000);

pub fn watch(paths: Vec<PathBuf>, event_proxy: EventProxy) {
    let paths = {
        let mut paths_tmp = Vec::new();
        for path in paths {
            if path.exists() {
                paths_tmp.push(path.clone());

                // Also watch the targets of symbolic links
                match path.symlink_metadata() {
                    Ok(metadata) if metadata.file_type().is_symlink() => {
                        match path.canonicalize() {
                            Ok(canonical_path) => paths_tmp.push(canonical_path),
                            Err(err) => {
                                error!("Unable to canonicalize config path {:?}: {}", path, err)
                            },
                        }
                    },
                    Ok(_) => {},
                    Err(err) => {
                        error!("Unable to get metadata for config path {:?}: {}", path, err)
                    },
                }
            } else {
                error!("Config path does not exist: {:?}", path);
            }
        }
        paths_tmp
    };

    // Don't monitor config if there is no path to watch.
    if paths.is_empty() {
        return;
    }

    // The Duration argument is a debouncing period.
    let (tx, rx) = mpsc::channel();

Review comment (suggestion):
By only canonicalizing paths that are symlinks, we're leaving us open for paths to be watched multiple times even though they're not symlinks.

Take the following example:
 - /tmp/../alacritty.yml
 - /alacritty.yml

Both are the same file, but only one of them needs to be watched.

Ultimately this should never happen without user error, but doing it "right" is basically the same effort as not, so I see no reason why we wouldn't do it properly.

Code after:
use std::sync::mpsc;
use std::time::Duration;

use log::{debug, error};
use notify::{watcher, DebouncedEvent, RecursiveMode, Watcher};

use alacritty_terminal::thread;

use crate::event::{Event, EventProxy};

#[cfg(any(target_os = "linux", target_os = "macos", target_os = "windows"))]
const DEBOUNCE_DELAY: Duration = Duration::from_millis(10);
#[cfg(not(any(target_os = "linux", target_os = "macos", target_os = "windows")))]
const DEBOUNCE_DELAY: Duration = Duration::from_millis(1000);

pub fn watch(mut paths: Vec<PathBuf>, event_proxy: EventProxy) {
    // Don't monitor config if there is no path to watch.
    if paths.is_empty() {
        return;
    }

    // Canonicalize paths, keeping the base paths for symlinks.
    for i in 0..paths.len() {
        if let Ok(canonical_path) = paths[i].canonicalize() {
            match paths[i].symlink_metadata() {
                Ok(metadata) if metadata.file_type().is_symlink() => paths.push(canonical_path),
                _ => paths[i] = canonical_path,
            }
        }
    }

    // The Duration argument is a debouncing period.
    let (tx, rx) = mpsc::channel();
    let mut watcher = match watcher(tx, DEBOUNCE_DELAY) {
        Ok(watcher) => watcher,
        Err(err) => {
            error!("Unable to watch config file: {}", err);
            return;
        },
    };

    thread::spawn_named("config watcher", move || {
        // Get all unique parent directories.
        let mut parents = paths
            .iter()
            .map(|path| {
                let mut path = path.clone();
                path.pop();
                path
            })
            .collect::<Vec<PathBuf>>();