rustpkg: Re-enable some tests

Re-enable test_install_invalid_external and test_install_invalid

Also improves an error message

Closes rust-lang#9994

Author: catamorphism

src/libextra/url.rs

@@ -573,7 +573,7 @@ fn get_authority(rawurl: &str) ->
 
 
 // returns the path and unparsed part of url, or an error
-fn get_path(rawurl: &str, authority: bool) ->
+pub fn get_path(rawurl: &str, authority: bool) ->
     Result<(~str, ~str), ~str> {
     let len = rawurl.len();
     let mut end = len;

src/librustpkg/package_id.rs

@@ -8,10 +8,12 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
+use extra::url;
 use version::{try_getting_version, try_getting_local_version,
               Version, NoVersion, split_version};
 use std::hash::Streaming;
 use std::hash;
+use messages::error;
 
 /// Path-fragment identifier of a package such as
 /// 'github.com/graydon/test'; path must be a relative
@@ -41,10 +43,60 @@ impl Eq for PkgId {
     }
 }
 
+/// Parses `s` as a URL and, if the parse was successful, returns the
+/// portion of the URL without the scheme
+/// for example: git://github.com/catamorphism/foo => github.com/catamorphism/foo
+fn drop_url_scheme<'a>(s: &'a str) -> Option<~str> {
+    match url::get_scheme(s) {
+        Ok((_, rest)) => {
+            // get_scheme leaves in the leading //
+            let len = rest.len();
+            let path_to_use = if len > 2 {
+                let no_slashes = rest.slice(2, len);
+                // Don't display the .git extension, since it's not part of the actual
+                // package ID
+                if no_slashes.ends_with(".git") {
+                    no_slashes.slice(0, len - 6).to_owned()
+                }
+                else {
+                    no_slashes.to_owned()
+                }
+            } else {
+                rest
+            };
+            Some(path_to_use)
+        }
+        Err(_)        => None
+    }
+}
+
+// Fails if this is not a legal package ID,
+// after printing a hint
+fn ensure_legal_package_id(s: &str) {
+    // url::get_path checks that the string contains characters
+    // that are legal in a URL ('#' is legal, so we're good there)
+    let maybe_intended_path = drop_url_scheme(s);
+    let legal = maybe_intended_path.is_none()
+        && url::get_path(s, false).is_ok();
+    if !legal {
+        for maybe_package_id in maybe_intended_path.iter() {
+            error(format!("rustpkg operates on package IDs; did you mean to write \
+                          `{}` instead of `{}`?",
+                  *maybe_package_id,
+                  s));
+        }
+        fail!("Can't parse {} as a package ID", s);
+    }
+}
+
 impl PkgId {
     pub fn new(s: &str) -> PkgId {
         use conditions::bad_pkg_id::cond;
 
+        // Make sure the path is a legal package ID -- it might not even
+        // be a legal path, so we do this first
+        ensure_legal_package_id(s);
+
         let mut given_version = None;
 
         // Did the user request a specific version?

src/librustpkg/tests.rs

@@ -15,8 +15,6 @@ use std::{os, run, str, task};
 use std::io;
 use std::io::fs;
 use std::io::File;
-use std::io::process;
-use std::io::process::ProcessExit;
 use extra::arc::Arc;
 use extra::arc::RWArc;
 use extra::tempfile::TempDir;
@@ -294,7 +292,7 @@ fn command_line_test_with_env(args: &[~str], cwd: &Path, env: Option<~[(~str, ~s
                    output.status);
     if !output.status.success() {
         debug!("Command {} {:?} failed with exit code {:?}; its output was --- {} ---",
-              cmd, args, output.status,
+               cmd, args, output.status,
               str::from_utf8(output.output) + str::from_utf8(output.error));
         Fail(output)
     }
@@ -615,7 +613,6 @@ fn test_install_valid() {
 }
 
 #[test]
-#[ignore]
 fn test_install_invalid() {
     let sysroot = test_sysroot();
     let pkgid = fake_pkg();
@@ -631,8 +628,11 @@ fn test_install_invalid() {
                                   pkgid.clone());
         ctxt.install(pkg_src, &WhatToBuild::new(MaybeCustom, Everything));
     };
-    assert!(result.unwrap_err()
-            .to_str().contains("supplied path for package dir does not exist"));
+    let x = result.unwrap_err();
+    assert!(x.is::<~str>());
+    let error_string = *x.move::<~str>().unwrap();
+    debug!("result error = {}", error_string);
+    assert!(error_string.contains("supplied path for package dir does not exist"));
 }
 
 #[test]
@@ -663,7 +663,6 @@ fn test_install_valid_external() {
 }
 
 #[test]
-#[ignore(reason = "9994")]
 fn test_install_invalid_external() {
     let cwd = os::getcwd();
     command_line_test_expect_fail([~"install", ~"foo"],
@@ -2442,6 +2441,21 @@ fn correct_error_dependency() {
     }
 }
 
+#[test]
+fn test_bad_package_id_url() {
+    use str::{is_utf8, from_utf8};
+
+    match command_line_test_partial([~"install", ~"git://github.com/mozilla/servo.git"],
+                                    &os::getcwd()) {
+        Fail(ProcessOutput{ error: _, output: output, _}) => {
+            assert!(is_utf8(output));
+            assert!(from_utf8(output).contains("rustpkg operates on package IDs; did you mean to \
+                                               write `github.com/mozilla/servo` instead"));
+        }
+        Success(*)  => fail!("test_bad_package_id_url: succeeded but should have failed")
+    }
+}
+
 /// Returns true if p exists and is executable
 fn is_executable(p: &Path) -> bool {
     p.exists() && p.stat().perm & io::UserExecute == io::UserExecute