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, SelfUpdateMode},
 };
 use crate::dist::dist::{
     PartialTargetTriple, PartialToolchainDesc, Profile, TargetTriple, ToolchainDesc,
@@ -718,8 +718,8 @@ pub fn cli() -> App<'static, 'static> {
                         .arg(
                             Arg::with_name("auto-self-update-mode")
                                 .required(true)
-                                .possible_values(&SELF_UPDATE_MODES)
-                                .default_value(SELF_UPDATE_MODES[0]),
+                                .possible_values(SelfUpdateMode::modes())
+                                .default_value(SelfUpdateMode::default_mode()),
                         ),
                 ),
         );
@@ -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 == SelfUpdateMode::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 == SelfUpdateMode::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

@@ -51,6 +51,7 @@ use std::fs;
 use std::io::Write;
 use std::path::{Component, Path, PathBuf, MAIN_SEPARATOR};
 use std::process::Command;
+use std::str::FromStr;
 
 use anyhow::{anyhow, Context, Result};
 use cfg_if::cfg_if;
@@ -88,7 +89,50 @@ pub const NEVER_SELF_UPDATE: bool = true;
 #[cfg(not(feature = "no-self-update"))]
 pub const NEVER_SELF_UPDATE: bool = false;
 
-pub const SELF_UPDATE_MODES: [&str; 3] = ["enable", "disable", "check-only"];
+#[derive(Clone, Debug, PartialEq)]
+pub enum SelfUpdateMode {
+    Enable,
+    Disable,
+    CheckOnly,
+}
+
+impl SelfUpdateMode {
+    pub fn modes() -> &'static [&'static str] {
+        &["enable", "disable", "check-only"]
+    }
+
+    pub fn default_mode() -> &'static str {
+        "enable"
+    }
+}
+
+impl FromStr for SelfUpdateMode {
+    type Err = anyhow::Error;
+
+    fn from_str(mode: &str) -> Result<Self> {
+        match mode {
+            "enable" => Ok(Self::Enable),
+            "disable" => Ok(Self::Disable),
+            "check-only" => Ok(Self::CheckOnly),
+            _ => Err(anyhow!(format!(
+                "unknown self update mode: '{}'; valid modes are {}",
+                mode,
+                valid_self_update_modes(),
+            ))),
+        }
+    }
+}
+
+impl ToString for SelfUpdateMode {
+    fn to_string(&self) -> String {
+        let modes = Self::modes();
+        match self {
+            SelfUpdateMode::Enable => modes[0].to_string(),
+            SelfUpdateMode::Disable => modes[1].to_string(),
+            SelfUpdateMode::CheckOnly => modes[2].to_string(),
+        }
+    }
+}
 
 // The big installation messages. These are macros because the first
 // argument of format! needs to be a literal.
@@ -494,7 +538,6 @@ fn do_pre_install_sanity_checks(no_prompt: bool) -> Result<()> {
 }
 
 fn do_pre_install_options_sanity_checks(opts: &InstallOpts<'_>) -> Result<()> {
-    use std::str::FromStr;
     // Verify that the installation options are vaguely sane
     (|| {
         let host_triple = opts
@@ -1144,7 +1187,7 @@ pub fn cleanup_self_updater() -> Result<()> {
 }
 
 pub fn valid_self_update_modes() -> String {
-    SELF_UPDATE_MODES
+    SelfUpdateMode::modes()
         .iter()
         .map(|s| format!("'{}'", s))
         .collect::<Vec<_>>()

src/config.rs

@@ -11,7 +11,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::SelfUpdateMode;
 use crate::dist::download::DownloadCfg;
 use crate::dist::{
     dist::{self, valid_profile_names},
@@ -406,21 +406,18 @@ 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 SelfUpdateMode::from_str(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(err) => Err(err),
         }
-        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 +445,11 @@ impl Cfg {
         })
     }
 
-    pub fn get_self_update_mode(&self) -> Result<String> {
+    pub fn get_self_update_mode(&self) -> Result<SelfUpdateMode> {
         self.settings_file.with(|s| {
             let mode = match &s.auto_self_update {
                 Some(mode) => mode.clone(),
-                None => SELF_UPDATE_MODES[0].to_string(),
+                None => SelfUpdateMode::Enable,
             };
             Ok(mode)
         })

src/settings.rs

@@ -1,9 +1,11 @@
 use std::cell::RefCell;
 use std::collections::BTreeMap;
 use std::path::{Path, PathBuf};
+use std::str::FromStr;
 
 use anyhow::{Context, Result};
 
+use crate::cli::self_update::SelfUpdateMode;
 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<SelfUpdateMode>,
 }
 
 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(SelfUpdateMode::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 SelfUpdateMode::from_str(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);