address comments

Author: 0xPoe

doc/src/basics.md

@@ -30,16 +30,17 @@ info: downloading self-updates
 
 ## Keeping `rustup` up to date
 
-## Keeping rustup up to date
+If your `rustup` build with the `no-self-update` feature, it will not be able to update itself.
 
-It is possible to control Rustup's automatic self update mechanism with the `auto-self-update` configuration
-variable.  This setting supports three values: `enable` and `disable` and `check-only`.
+If your `rustup` build without the `no-self-update` feature, 
+it is possible to control Rustup's automatic self update mechanism with the `auto-self-update` configuration variable. 
+This setting supports three values: `enable` and `disable` and `check-only`.
 
 * `disable` will ensure that no automatic self updating actions are taken.
 * `enable` will mean that `rustup update` and similar commands will also check-for, and install, any update to Rustup.
 * `check-only` will cause any automatic self update to check and report on any updates, but not to automatically install them.
 
-Whether automatic self-update is enabled or not, you can request that Rustup check for, and update itself to the
+Whether `auto-self-update` is `enable` or not, you can request that Rustup check for, and update itself to the
 latest version of `rustup` without updating any installed toolchains by running `rustup
 self update`:
 
@@ -50,7 +51,7 @@ info: downloading self-updates
 ```
 
 > ### Disable automatic self-updates with the parameter of the command
-> If auto-self-update is `enable`, `rustup` will automatically update itself at the end of any
+> Since the default value of auto-self-update is `enable`, `rustup` will automatically update itself at the end of any
 > toolchain installation as well.  You can prevent this automatic behaviour by
 > passing the `--no-self-update` argument when running `rustup update` or
 > `rustup toolchain install`.

src/cli/rustup_mode.rs

@@ -15,7 +15,7 @@ use super::term2::Terminal;
 use super::topical_doc;
 use super::{
     common,
-    self_update::{check_rustup_update, SELF_UPDATE_MODES},
+    self_update::{check_rustup_update, UpdateMode, SELF_UPDATE_MODES},
 };
 use crate::dist::dist::{
     PartialTargetTriple, PartialToolchainDesc, Profile, TargetTriple, ToolchainDesc,
@@ -937,7 +937,7 @@ fn update(cfg: &mut Cfg, m: &ArgMatches<'_>) -> Result<utils::ExitCode> {
     // and auto-self-update is configured to **enable**
     // and has **no** no-self-update parameter.
     let self_update = !self_update::NEVER_SELF_UPDATE
-        && self_update_mode == "enable"
+        && self_update_mode == UpdateMode::Enable
         && !m.is_present("no-self-update");
     let forced = m.is_present("force-non-host");
     if let Some(p) = m.value_of("profile") {
@@ -1024,7 +1024,7 @@ fn update(cfg: &mut Cfg, m: &ArgMatches<'_>) -> Result<utils::ExitCode> {
         cfg.temp_cfg.clean();
     }
 
-    if !self_update::NEVER_SELF_UPDATE && self_update_mode == "check-only" {
+    if !self_update::NEVER_SELF_UPDATE && self_update_mode == UpdateMode::CheckOnly {
         check_rustup_update()?;
     }
 
@@ -1587,7 +1587,7 @@ fn set_profile(cfg: &mut Cfg, m: &ArgMatches<'_>) -> Result<utils::ExitCode> {
 
 fn set_auto_self_update(cfg: &mut Cfg, m: &ArgMatches<'_>) -> Result<utils::ExitCode> {
     if self_update::NEVER_SELF_UPDATE {
-        warn!("Your rustup is built with the no-self-update feature, so setting auto-self-update will not have any effect.");
+        warn!("{} is built with the no-self-update feature: setting auto-self-update will not have any effect.",cfg.rustup_dir.display());
     }
     cfg.set_auto_self_update(&m.value_of("auto-self-update-mode").unwrap())?;
     Ok(utils::ExitCode(0))

src/cli/self_update.rs

@@ -45,6 +45,7 @@ mod os {
 }
 
 use std::borrow::Cow;
+use std::convert::TryFrom;
 use std::env;
 use std::env::consts::EXE_SUFFIX;
 use std::fs;
@@ -89,6 +90,35 @@ pub const NEVER_SELF_UPDATE: bool = true;
 pub const NEVER_SELF_UPDATE: bool = false;
 
 pub const SELF_UPDATE_MODES: [&str; 3] = ["enable", "disable", "check-only"];
+#[derive(Clone, Debug, PartialEq)]
+pub enum UpdateMode {
+    Enable,
+    Disable,
+    CheckOnly,
+}
+
+impl TryFrom<&str> for UpdateMode {
+    type Error = ();
+
+    fn try_from(value: &str) -> Result<Self, Self::Error> {
+        match value {
+            "enable" => Ok(Self::Enable),
+            "disable" => Ok(Self::Disable),
+            "check-only" => Ok(Self::CheckOnly),
+            _ => Err(()),
+        }
+    }
+}
+
+impl ToString for UpdateMode {
+    fn to_string(&self) -> String {
+        match self {
+            UpdateMode::Enable => SELF_UPDATE_MODES[0].to_string(),
+            UpdateMode::Disable => SELF_UPDATE_MODES[1].to_string(),
+            UpdateMode::CheckOnly => SELF_UPDATE_MODES[2].to_string(),
+        }
+    }
+}
 
 // The big installation messages. These are macros because the first
 // argument of format! needs to be a literal.

src/config.rs

@@ -1,4 +1,5 @@
 use std::borrow::Cow;
+use std::convert::TryFrom;
 use std::fmt::{self, Display};
 use std::io;
 use std::path::{Path, PathBuf};
@@ -11,7 +12,7 @@ use pgp::{Deserializable, SignedPublicKey};
 use serde::Deserialize;
 use thiserror::Error as ThisError;
 
-use crate::cli::self_update::{valid_self_update_modes, SELF_UPDATE_MODES};
+use crate::cli::self_update::{valid_self_update_modes, UpdateMode};
 use crate::dist::download::DownloadCfg;
 use crate::dist::{
     dist::{self, valid_profile_names},
@@ -406,21 +407,24 @@ impl Cfg {
         Ok(())
     }
 
-    // FIXME(hi-rustin): It is better to use enumerations rather than strings.
     pub fn set_auto_self_update(&mut self, mode: &str) -> Result<()> {
-        if !SELF_UPDATE_MODES.contains(&mode) {
-            return Err(anyhow!(
-                "unknown self update mode: '{}'; valid modes are {}",
-                mode.to_owned(),
-                valid_self_update_modes(),
-            ));
+        match UpdateMode::try_from(mode) {
+            Ok(update_mode) => {
+                self.settings_file.with_mut(|s| {
+                    s.auto_self_update = Some(update_mode);
+                    Ok(())
+                })?;
+                (self.notify_handler)(Notification::SetSelfUpdate(mode));
+                Ok(())
+            }
+            Err(_) => {
+                return Err(anyhow!(
+                    "unknown self update mode: '{}'; valid modes are {}",
+                    mode.to_owned(),
+                    valid_self_update_modes(),
+                ));
+            }
         }
-        self.settings_file.with_mut(|s| {
-            s.auto_self_update = Some(mode.to_owned());
-            Ok(())
-        })?;
-        (self.notify_handler)(Notification::SetSelfUpdate(mode));
-        Ok(())
     }
 
     pub fn set_toolchain_override(&mut self, toolchain_override: &str) {
@@ -448,11 +452,11 @@ impl Cfg {
         })
     }
 
-    pub fn get_self_update_mode(&self) -> Result<String> {
+    pub fn get_self_update_mode(&self) -> Result<UpdateMode> {
         self.settings_file.with(|s| {
             let mode = match &s.auto_self_update {
                 Some(mode) => mode.clone(),
-                None => SELF_UPDATE_MODES[0].to_string(),
+                None => UpdateMode::Enable,
             };
             Ok(mode)
         })

src/settings.rs

@@ -1,9 +1,11 @@
 use std::cell::RefCell;
 use std::collections::BTreeMap;
+use std::convert::TryFrom;
 use std::path::{Path, PathBuf};
 
 use anyhow::{Context, Result};
 
+use crate::cli::self_update::UpdateMode;
 use crate::errors::*;
 use crate::notifications::*;
 use crate::toml_utils::*;
@@ -77,7 +79,7 @@ pub struct Settings {
     pub profile: Option<String>,
     pub overrides: BTreeMap<String, String>,
     pub pgp_keys: Option<String>,
-    pub auto_self_update: Option<String>,
+    pub auto_self_update: Option<UpdateMode>,
 }
 
 impl Default for Settings {
@@ -89,7 +91,7 @@ impl Default for Settings {
             profile: Some("default".to_owned()),
             overrides: BTreeMap::new(),
             pgp_keys: None,
-            auto_self_update: Some("enable".to_owned()),
+            auto_self_update: Some(UpdateMode::Enable),
         }
     }
 }
@@ -148,14 +150,21 @@ impl Settings {
         if !SUPPORTED_METADATA_VERSIONS.contains(&&*version) {
             return Err(RustupError::UnknownMetadataVersion(version).into());
         }
+        let auto_self_update = match get_opt_string(&mut table, "auto_self_update", path)? {
+            Some(auto_self_update) => match UpdateMode::try_from(auto_self_update.as_str()) {
+                Ok(mode) => Some(mode),
+                Err(_) => None,
+            },
+            None => None,
+        };
         Ok(Self {
             version,
             default_host_triple: get_opt_string(&mut table, "default_host_triple", path)?,
             default_toolchain: get_opt_string(&mut table, "default_toolchain", path)?,
             profile: get_opt_string(&mut table, "profile", path)?,
             overrides: Self::table_to_overrides(&mut table, path)?,
             pgp_keys: get_opt_string(&mut table, "pgp_keys", path)?,
-            auto_self_update: get_opt_string(&mut table, "auto_self_update", path)?,
+            auto_self_update,
         })
     }
     pub fn into_toml(self) -> toml::value::Table {
@@ -180,7 +189,10 @@ impl Settings {
         }
 
         if let Some(v) = self.auto_self_update {
-            result.insert("auto_self_update".to_owned(), toml::Value::String(v));
+            result.insert(
+                "auto_self_update".to_owned(),
+                toml::Value::String(v.to_string()),
+            );
         }
 
         let overrides = Self::overrides_to_table(self.overrides);