Backslashes must be escaped on Windows

[mlschneid]

This was a small issue I noticed during the installation of Rust on Windows.

[tesuji]

Did you find that in this line https://ci.appveyor.com/project/rust-lang-libs/rustup/builds/29742913/job/97ad84eq8c239eb8#L54 ?

To get started you need Cargo's bin directory (%USERPROFILE%.cargo\bin) in your
PATH
environment variable. Future applications will automatically have the
correct environment, but you may need to restart your current shell.
set PATH=%PATH%;%USERPROFILE%\.cargo\bin

I think this patch is not right because it was the raw string so the \ doesn't need to be escaped.

Edit: I was wrong. This needs to be escaped because the string was passed to markdown.
Maybe add a comment to explain why.

[tesuji]

Checked it again. It might be wrong in other places not passed to markdown.

[mlschneid]

Grepping for %userprofile%\.cargo returned the line where I made the fix, and the appveyor file. I didn't think the appveyor file should be changed, since it seems to work as is. The other lines were, as you said, correct. Only the strings that are printed to the console need the escaped slashes.

I'll take another look tomorrow and be sure this is the only change necessary.

[mlschneid]

Added another two commits. Let me know if that works for you.

[kinnison]

I think rather than "serves as documentation" a comment along the lines of:

// Since this string will be interpolated into markdown which is then rendered
// for the user, backslashes must be escaped in order to make it through
// the markdown processing
``

[tesuji]

If you search for uses of this function (canonical_cargo_home). Not only its result passed
to md function but to raw stdout which doesn't need to be escaped.

[kinnison]

I see the function used in three places:

• https://github.com/rust-lang/rustup/blob/master/src/cli/self_update.rs#L319 -- where it is used to interpolate into a string which is then processed via the md() function
• https://github.com/rust-lang/rustup/blob/master/src/cli/self_update.rs#L817 -- where it is used similarly
• https://github.com/rust-lang/rustup/blob/master/src/cli/self_update.rs#L1155 -- where it is used as an interpolation into a shell string which is then only used under unix and thus irrelevant to this.

Could you please point out what I've missed?

[mlschneid]

If you search for uses of this function (canonical_cargo_home). Not only its result passed
to md function but to raw stdout which doesn't need to be escaped.

Its true that markdown doesn't need its slashes escaped. Markdown with render both a single backslash and double backslash correctly. However, a single backslash to stdout wasn't rendering in windows command prompt.

I think rather than "serves as documentation" a comment along the lines of

I do agree that I need to change the documentation to something closer to what @kinnison wrote. I'll wait for the discussion amongst collaborators/contributors to finish before adding another commit.

Let me know what you guys think. I'm totally fine with amending my PR in any way you see fit.

[kinnison]

Interesting, so you're saying that the single backslash makes it through the markdown okay, but that the Windows console output code somehow treats it as an escape? If so, perhaps we ought to add escaping into our output routines rather than here on the inputs to them? If I've misunderstood then please could you clarify?

[mlschneid]

@kinnison In order to answer your question, I was going to confirm this by writing directly to stdout and compare that to the output of what went into the md() function. I built the new changes and ran rustup self uninstall. I was hoping to run rustup-init.exe and see duplicate output, but...

It turns out my new commit produces a double slash in the console! (from this particular line). The reason why a double backslash appeared here but not in the install phase is that the uninstall phase uses `` around the paths. It is markdown that is treating the backslash as an escape, not the console.

We have yet another question that emerges. Should we add `` around all paths instead of adding escaping slashes?

[kinnison]

I don't mind either way. In markdown documents I'd expect to see paths with s around them simply because I'd want them rendered monospace -- that's less of an issue with the console. However for consistency perhaps we should ensure that all our console markdown is using s for paths, that way things ought to be easier to keep right.

[mlschneid]

@lzutao thoughts on the above?

[tesuji]

I may use this patch.

diff --git a/src/cli/self_update.rs b/src/cli/self_update.rs
index 62bbef4..0fc6bb7 100644
--- a/src/cli/self_update.rs
+++ b/src/cli/self_update.rs
@@ -215,18 +215,19 @@ static UPDATE_ROOT: &str = "https://static.rust-lang.org/rustup";
 /// substituted for the directory prefix
 fn canonical_cargo_home() -> Result<String> {
     let path = utils::cargo_home()?;
-    let mut path_str = path.to_string_lossy().to_string();
 
     let default_cargo_home = utils::home_dir()
         .unwrap_or_else(|| PathBuf::from("."))
         .join(".cargo");
-    if default_cargo_home == path {
+    let path_str = if default_cargo_home == path {
         if cfg!(unix) {
-            path_str = String::from("$HOME/.cargo");
+            String::from("$HOME/.cargo")
         } else {
-            path_str = String::from(r"%USERPROFILE%\.cargo");
-        }
+            String::from(r"%USERPROFILE%\.cargo")
         }
+    } else {
+        path.display().to_string()
+    };
 
     Ok(path_str)
 }
@@ -318,22 +319,19 @@ pub fn install(no_prompt: bool, verbose: bool, quiet: bool, mut opts: InstallOpt
     }
 
     let cargo_home = canonical_cargo_home()?;
-    let msg = if !opts.no_modify_path {
-        if cfg!(unix) {
-            format!(post_install_msg_unix!(), cargo_home = cargo_home)
-        } else {
-            format!(post_install_msg_win!(), cargo_home = cargo_home)
-        }
-    } else if cfg!(unix) {
-        format!(
+    #[cfg(windows)]
+    let cargo_home = cargo_home.replace('\\', r"\\");
+    let msg = match (opts.no_modify_path, cfg!(unix)) {
+        (false, true) => format!(post_install_msg_unix!(), cargo_home = cargo_home),
+        (false, false) => format!(post_install_msg_win!(), cargo_home = cargo_home),
+        (true, true) => format!(
             post_install_msg_unix_no_modify_path!(),
             cargo_home = cargo_home
-        )
-    } else {
-        format!(
+        ),
+        (true, false) => format!(
             post_install_msg_win_no_modify_path!(),
             cargo_home = cargo_home
-        )
+        ),
     };
     md(&mut term, msg);