Fix version migrations being triggered backwards

Fixes #539 ("Ensure versions migrations are suggested only when current
version is newer than the previous one")

Resolves: <https://git.meli-email.org/meli/meli/issues/539>
Signed-off-by: Manos Pitsidianakis <manos@pitsidianak.is>

meli/src/conf.rs

@@ -338,7 +338,11 @@ impl FileSettings {
                 path_string
             ));
             #[cfg(not(test))]
-            if ask.run() {
+            let mut stdout = std::io::stdout();
+            #[cfg(not(test))]
+            let stdin = std::io::stdin();
+            #[cfg(not(test))]
+            if ask.run(&mut stdout, &mut stdin.lock()) {
                 create_config_file(&config_path)?;
                 return Err(
                     Error::new("Edit the sample configuration and relaunch meli.")
@@ -353,7 +357,9 @@ impl FileSettings {
             );
         }
 
-        crate::version_migrations::version_setup(&config_path)?;
+        let mut stdout = std::io::stdout();
+        let stdin = std::io::stdin();
+        crate::version_migrations::version_setup(&config_path, &mut stdout, &mut stdin.lock())?;
         Self::validate(config_path, false)
     }
 

meli/src/terminal.rs

@@ -36,10 +36,7 @@ pub mod embedded;
 mod tests;
 pub mod text_editing;
 
-use std::{
-    borrow::Cow,
-    io::{BufRead, Write},
-};
+use std::borrow::Cow;
 
 pub use braille::BraillePixelIter;
 pub use screen::{Area, Screen, ScreenGeneration, StateStdout, Tty, Virtual};
@@ -450,10 +447,8 @@ impl<'m> Ask<'m> {
         }
     }
 
-    pub fn run(self) -> bool {
+    pub fn run(self, writer: &mut impl std::io::Write, reader: &mut impl std::io::BufRead) -> bool {
         let mut buffer = String::new();
-        let stdin = std::io::stdin();
-        let mut handle = stdin.lock();
 
         let default = match self.default {
             None => "y/n",
@@ -461,11 +456,11 @@ impl<'m> Ask<'m> {
             Some(false) => "y/N",
         };
 
-        print!("{} [{default}] ", self.message.as_ref());
-        let _ = std::io::stdout().flush();
+        _ = write!(writer, "{} [{default}] ", self.message.as_ref());
+        _ = writer.flush();
         loop {
             buffer.clear();
-            handle
+            reader
                 .read_line(&mut buffer)
                 .expect("Could not read from stdin.");
 
@@ -478,8 +473,8 @@ impl<'m> Ask<'m> {
                     return false;
                 }
                 _ => {
-                    print!("\n{} [{default}] ", self.message.as_ref());
-                    let _ = std::io::stdout().flush();
+                    _ = write!(writer, "\n{} [{default}] ", self.message.as_ref());
+                    _ = writer.flush();
                 }
             }
         }

meli/src/version_migrations/mod.rs

@@ -197,7 +197,10 @@ decl_version_mods! {
     v0_8_9::V0_8_9_ID => v0_8_9::V0_8_9
 }
 
-use std::{cmp::Ordering, path::Path};
+use std::{
+    cmp::Ordering,
+    path::{Path, PathBuf},
+};
 
 use indexmap::{self, IndexMap};
 use melib::{error::*, log};
@@ -344,139 +347,188 @@ impl std::fmt::Debug for dyn Migration + Send + Sync {
     }
 }
 
+/// Return the path to the `.version` file, a plain text file that contains the
+/// version of meli that "owns" the configuration and data files.
+pub fn version_file() -> Result<PathBuf> {
+    let xdg_dirs = xdg::BaseDirectories::with_prefix("meli")?;
+    Ok(xdg_dirs.place_data_file(".version")?)
+}
+
 /// Inspect current/previous version setup, perform migrations if necessary,
 /// etc.
-pub fn version_setup(config: &Path) -> Result<()> {
-    if let Ok(xdg_dirs) = xdg::BaseDirectories::with_prefix("meli") {
-        let version_file = match xdg_dirs.place_data_file(".version") {
-            Ok(v) => v,
-            Err(err) => {
-                log::debug!(
-                    "Could not place file with version metadata, .version, in your \
-                     ${{XDG_DATA_HOME}}: {}",
-                    err
-                );
-                return Ok(());
-            }
-        };
-        let stored_version = if !version_file.try_exists().unwrap_or(false) {
-            None
-        } else {
-            let mut stored_version =
-                std::fs::read_to_string(&version_file).chain_err_related_path(&version_file)?;
-            while stored_version.ends_with(['\r', '\n', ' ', '\t']) {
-                stored_version.pop();
-            }
-            if LATEST.as_str() == stored_version {
-                return Ok(());
-            }
-            Some(stored_version)
-        };
-        let version_map = versions();
-        let migrations = calculate_migrations(stored_version.as_deref(), version_map);
-        if !migrations.is_empty() {
-            if let Some(prev) = stored_version {
-                println!(
+pub fn version_setup(
+    config: &Path,
+    writer: &mut impl std::io::Write,
+    reader: &mut impl std::io::BufRead,
+) -> Result<()> {
+    let version_file = match version_file() {
+        Ok(v) => v,
+        Err(err) => {
+            log::debug!(
+                "Could not place file with version metadata, .version, in your \
+                 ${{XDG_DATA_HOME}}: {}",
+                err
+            );
+            return Ok(());
+        }
+    };
+    let stored_version = if !version_file.try_exists().unwrap_or(false) {
+        None
+    } else {
+        let mut stored_version =
+            std::fs::read_to_string(&version_file).chain_err_related_path(&version_file)?;
+        while stored_version.ends_with(['\r', '\n', ' ', '\t']) {
+            stored_version.pop();
+        }
+        if LATEST.as_str() == stored_version {
+            return Ok(());
+        }
+        Some(stored_version)
+    };
+    let version_map = versions();
+    let migrations = calculate_migrations(stored_version.as_deref(), version_map);
+    if !migrations.is_empty() {
+        if let Some(prev) = stored_version {
+            if prev.as_str() < LATEST.as_str() {
+                writeln!(
+                    writer,
                     "meli appears updated; file {} contains the value {:?} and the latest version \
                      is {}",
                     version_file.display(),
                     prev,
                     LATEST
-                );
+                )?;
+                writer.flush()?;
             } else {
-                // Check if any migrations are applicable; they might not be any (for example if
-                // user runs meli for the first time).
-                if !migrations.iter().any(|(_, migrs)| {
-                    migrs
-                        .iter()
-                        .any(|migr| migr.is_applicable(config) != Some(false))
-                }) {
-                    log::info!(
-                        "Creating version info file {} with value {}",
-                        version_file.display(),
-                        LATEST
-                    );
+                writeln!(
+                    writer,
+                    "This version of meli, {}, appears to be older than the previously used one \
+                     stored in the file {}: {}.",
+                    LATEST,
+                    version_file.display(),
+                    prev,
+                )?;
+                writeln!(
+                    writer,
+                    "Certain configuration options might not be compatible with this version, \
+                     refer to release changelogs if you need to troubleshoot configuration \
+                     options problems."
+                )?;
+                writer.flush()?;
+                let ask = Ask::new(
+                    "Update .version file to make this warning go away? (CAUTION: current \
+                     configuration and stored data might not be compatible with this version!!)",
+                )
+                .yes_by_default(false);
+                if ask.run(writer, reader) {
                     std::fs::write(&version_file, LATEST.as_str())
                         .chain_err_related_path(&version_file)?;
                     return Ok(());
                 }
-                println!(
-                    "meli appears updated; version file {} was not found and there are potential \
-                     migrations to be made.",
-                    version_file.display()
+                return Ok(());
+            }
+        } else {
+            // Check if any migrations are applicable; they might not be any (for example if
+            // user runs meli for the first time).
+            if !migrations.iter().any(|(_, migrs)| {
+                migrs
+                    .iter()
+                    .any(|migr| migr.is_applicable(config) != Some(false))
+            }) {
+                log::info!(
+                    "Creating version info file {} with value {}",
+                    version_file.display(),
+                    LATEST
                 );
+                std::fs::write(&version_file, LATEST.as_str())
+                    .chain_err_related_path(&version_file)?;
+                return Ok(());
             }
-            println!(
-                "You might need to migrate your configuration data for the new version to \
-                 work.\nYou can skip any changes you don't want to happen and you can quit at any \
-                 time."
-            );
-            println!(
-                "{} migration{} {} about to be performed:",
-                migrations.len(),
-                if migrations.len() == 1 { "" } else { "s" },
-                if migrations.len() == 1 { "is" } else { "are" }
-            );
-            for (vers, migrs) in &migrations {
-                for m in migrs {
-                    println!("v{}/{}: {}", vers, m.id(), m.description());
-                }
+            writeln!(
+                writer,
+                "meli appears updated; version file {} was not found and there are potential \
+                 migrations to be made.",
+                version_file.display()
+            )?;
+            writer.flush()?;
+        }
+        writeln!(
+            writer,
+            "You might need to migrate your configuration data for the new version to work.\nYou \
+             can skip any changes you don't want to happen and you can quit at any time."
+        )?;
+        writeln!(
+            writer,
+            "{} migration{} {} about to be performed:",
+            migrations.len(),
+            if migrations.len() == 1 { "" } else { "s" },
+            if migrations.len() == 1 { "is" } else { "are" }
+        )?;
+        for (vers, migrs) in &migrations {
+            for m in migrs {
+                writeln!(writer, "v{}/{}: {}", vers, m.id(), m.description())?;
             }
-            let ask = Ask::new(format!(
-                "Perform {} migration{}?",
-                migrations.len(),
-                if migrations.len() == 1 { "" } else { "s" }
-            ));
-            if !ask.run() {
-                let ask = Ask::new("Update .version file despite not attempting migrations?")
-                    .yes_by_default(false);
-                if ask.run() {
-                    std::fs::write(&version_file, LATEST.as_str())
-                        .chain_err_related_path(&version_file)?;
-                    return Ok(());
-                }
+        }
+        writer.flush()?;
+        let ask = Ask::new(format!(
+            "Perform {} migration{}?",
+            migrations.len(),
+            if migrations.len() == 1 { "" } else { "s" }
+        ));
+        if !ask.run(writer, reader) {
+            let ask = Ask::new("Update .version file despite not attempting migrations?")
+                .yes_by_default(false);
+            if ask.run(writer, reader) {
+                std::fs::write(&version_file, LATEST.as_str())
+                    .chain_err_related_path(&version_file)?;
                 return Ok(());
             }
-            let mut perform_history: Vec<Box<dyn Migration + 'static>> = vec![];
-            for (vers, migrs) in migrations {
-                println!("Updating to {}...", vers);
-                'migrations: for m in migrs {
-                    let ask = Ask::new(m.question());
-                    if ask.run() {
-                        if let Err(err) = m.perform(config, false, true) {
-                            println!("\nCould not perform migration: {}", err);
-                            let ask = Ask::new("Continue?");
-                            if ask.run() {
-                                continue 'migrations;
-                            }
-                            if !perform_history.is_empty() {
-                                let ask =
-                                    Ask::new("Undo already performed migrations before exiting?")
-                                        .without_default();
-                                if ask.run() {
-                                    while let Some(m) = perform_history.pop() {
-                                        print!("Undoing {}...", m.id());
-                                        if let Err(err) = m.revert(config, false, true) {
-                                            println!(
-                                                " [ERROR] could not revert migration: {}",
-                                                err
-                                            );
-                                        } else {
-                                            println!(" [OK]");
-                                        }
+            return Ok(());
+        }
+        let mut perform_history: Vec<Box<dyn Migration + 'static>> = vec![];
+        for (vers, migrs) in migrations {
+            writeln!(writer, "Updating to {}...", vers)?;
+            writer.flush()?;
+            'migrations: for m in migrs {
+                let ask = Ask::new(m.question());
+                if ask.run(writer, reader) {
+                    if let Err(err) = m.perform(config, false, true) {
+                        writeln!(writer, "\nCould not perform migration: {}", err)?;
+                        writer.flush()?;
+                        let ask = Ask::new("Continue?");
+                        if ask.run(writer, reader) {
+                            continue 'migrations;
+                        }
+                        if !perform_history.is_empty() {
+                            let ask = Ask::new("Undo already performed migrations before exiting?")
+                                .without_default();
+                            if ask.run(writer, reader) {
+                                while let Some(m) = perform_history.pop() {
+                                    write!(writer, "Undoing {}...", m.id())?;
+                                    writer.flush()?;
+                                    if let Err(err) = m.revert(config, false, true) {
+                                        writeln!(
+                                            writer,
+                                            " [ERROR] could not revert migration: {}",
+                                            err
+                                        )?;
+                                    } else {
+                                        writeln!(writer, " [OK]")?;
                                     }
+                                    writer.flush()?;
                                 }
                             }
-                            return Ok(());
                         }
-                        println!("v{}/{} [OK]", vers, m.id());
-                        perform_history.push(m);
+                        return Ok(());
                     }
+                    writeln!(writer, "v{}/{} [OK]", vers, m.id())?;
+                    writer.flush()?;
+                    perform_history.push(m);
                 }
             }
         }
-        std::fs::write(&version_file, LATEST.as_str()).chain_err_related_path(&version_file)?;
     }
+    std::fs::write(&version_file, LATEST.as_str()).chain_err_related_path(&version_file)?;
 
     Ok(())
 }

meli/src/version_migrations/tests.rs

@@ -20,6 +20,8 @@
 //
 // SPDX-License-Identifier: EUPL-1.2 OR GPL-3.0-or-later
 
+use rusty_fork::rusty_fork_test;
+
 use super::*;
 
 #[test]
@@ -61,3 +63,83 @@ fn test_version_migrations_returns_correct_migration() {
         "Calculated migrations between no version and 0.8.8 are empty",
     );
 }
+
+rusty_fork_test! {
+#[test]
+fn test_version_migrations_ignores_newer_version() {
+    const MAX: VersionIdentifier = VersionIdentifier {
+        string: "255.255.255",
+        major: u8::MAX,
+        minor: u8::MAX,
+        patch: u8::MAX,
+        pre: "",
+    };
+    let tempdir = tempfile::tempdir().unwrap();
+    for var in [
+        "MELI_CONFIG",
+        "HOME",
+        "XDG_CACHE_HOME",
+        "XDG_STATE_HOME",
+        "XDG_CONFIG_DIRS",
+        "XDG_CONFIG_HOME",
+        "XDG_DATA_DIRS",
+        "XDG_DATA_HOME",
+    ] {
+        std::env::remove_var(var);
+    }
+    std::env::set_var("HOME", tempdir.path());
+    std::env::set_var("XDG_DATA_HOME", tempdir.path());
+    let version_file = version_file().unwrap();
+    std::fs::write(&version_file, MAX.as_str()).unwrap();
+    let config_path = tempdir.path().join("meli.toml");
+    std::env::set_var("MELI_CONFIG", config_path.as_path());
+    std::fs::write(&config_path,
+br#"
+[accounts.imap]
+root_mailbox = "INBOX"
+format = "imap"
+send_mail = 'false'
+identity="username@example.com"
+server_username = "null"
+server_hostname = "example.com"
+server_password_command = "false"
+"#).unwrap();
+
+    {
+        let mut stdout = vec![];
+        let mut stdin = &b"y\n"[..];
+        let mut stdin_buf_reader = std::io::BufReader::new(&mut stdin);
+        version_setup(&config_path, &mut stdout, &mut stdin_buf_reader).unwrap();
+        let expected_output = format!("This version of meli, {latest}, appears to be older than the previously used one stored in the file {version_file}: {max_version}.\nCertain configuration options might not be compatible with this version, refer to release changelogs if you need to troubleshoot configuration options problems.\nUpdate .version file to make this warning go away? (CAUTION: current configuration and stored data might not be compatible with this version!!) [y/N] ", latest = LATEST.as_str(), version_file = version_file.display(), max_version = MAX.as_str());
+        assert_eq!(String::from_utf8_lossy(&stdout).as_ref(), &expected_output);
+        assert_eq!(stdin_buf_reader.buffer(), b"");
+        let updated_version =
+            std::fs::read_to_string(&version_file).unwrap();
+        assert_eq!(updated_version.trim(), LATEST.as_str());
+    }
+    {
+        use std::io::BufRead;
+
+        let mut stdout = vec![];
+        let mut stdin = &b"N\n"[..];
+        let mut stdin_buf_reader = std::io::BufReader::new(&mut stdin);
+
+        version_setup(&config_path, &mut stdout, &mut stdin_buf_reader).unwrap();
+        assert_eq!(String::from_utf8_lossy(&stdout).as_ref(), "");
+        assert_eq!(stdin_buf_reader.fill_buf().unwrap(), b"N\n");
+    }
+    {
+        std::fs::write(&version_file, MAX.as_str()).unwrap();
+        let mut stdout = vec![];
+        let mut stdin = &b"n\n"[..];
+        let mut stdin_buf_reader = std::io::BufReader::new(&mut stdin);
+        version_setup(&config_path, &mut stdout, &mut stdin_buf_reader).unwrap();
+        let expected_output = format!("This version of meli, {latest}, appears to be older than the previously used one stored in the file {version_file}: {max_version}.\nCertain configuration options might not be compatible with this version, refer to release changelogs if you need to troubleshoot configuration options problems.\nUpdate .version file to make this warning go away? (CAUTION: current configuration and stored data might not be compatible with this version!!) [y/N] ", latest = LATEST.as_str(), version_file = version_file.display(), max_version = MAX.as_str());
+        assert_eq!(String::from_utf8_lossy(&stdout).as_ref(), &expected_output);
+        assert_eq!(stdin_buf_reader.buffer(), b"");
+        let stored_version =
+            std::fs::read_to_string(&version_file).unwrap();
+        assert_eq!(stored_version.trim(), MAX.as_str());
+    }
+}
+}