Block publishing on dirty directories

Dearest reviewer,

This is part of #841 which is about the publishing and tagging flow.
This pull request just prevents publishing with uncommitted or untracked
files. I have included a flag for turning this off. There are two new
test.

More details about the full flow at
#2443 . I plan on doing more
work on the flow, however, I felt this was useful enough to do stand
alone. When I tried the flow before I was doing to much at once.

Thanks!
Becker

[Updated]
Moved from config to flag
Using open_ext to sub crates
Include the file names in error message
include flag in error
Move to discover off of open_ext
formatting
cut back on unwraps
rework file check logic with a bail

src/bin/publish.rs

@@ -8,6 +8,7 @@ pub struct Options {
     flag_token: Option<String>,
     flag_manifest_path: Option<String>,
     flag_verbose: Option<bool>,
+    flag_allow_untracked: bool,
     flag_quiet: Option<bool>,
     flag_color: Option<String>,
     flag_no_verify: bool,
@@ -26,6 +27,7 @@ Options:
     --no-verify              Don't verify package tarball before publish
     --manifest-path PATH     Path to the manifest of the package to publish
     -v, --verbose            Use verbose output
+    --allow-untracked        Allows publishing with untracked and uncommitted files
     -q, --quiet              No output printed to stdout
     --color WHEN             Coloring: auto, always, never
 
@@ -40,10 +42,11 @@ pub fn execute(options: Options, config: &Config) -> CliResult<Option<()>> {
         flag_host: host,
         flag_manifest_path,
         flag_no_verify: no_verify,
+        flag_allow_untracked: allow_untracked,
         ..
     } = options;
 
     let root = try!(find_root_manifest_for_wd(flag_manifest_path.clone(), config.cwd()));
-    try!(ops::publish(&root, config, token, host, !no_verify));
+    try!(ops::publish(&root, config, token, host, !no_verify, allow_untracked));
     Ok(None)
 }

src/cargo/ops/registry.rs

@@ -32,9 +32,13 @@ pub fn publish(manifest_path: &Path,
                config: &Config,
                token: Option<String>,
                index: Option<String>,
-               verify: bool) -> CargoResult<()> {
+               verify: bool, allow_untracked: bool) -> CargoResult<()> {
     let pkg = try!(Package::for_path(&manifest_path, config));
 
+    if !allow_untracked {
+        try!(check_directory_cleanliness());
+    }
+
     if !pkg.publish() {
         bail!("some crates cannot be published.\n\
                `{}` is marked as unpublishable", pkg.name());
@@ -55,6 +59,42 @@ pub fn publish(manifest_path: &Path,
     Ok(())
 }
 
+fn check_git_cleanliness(repo: &git2::Repository) -> CargoResult<()> {
+    let mut opts = git2::StatusOptions::new();
+    opts.include_untracked(true).recurse_untracked_dirs(true);
+    let status = try!(repo.statuses(Some(&mut opts)));
+    let files:Vec<String> = status.iter().map(|entry| {
+        let file = entry.index_to_workdir()
+                        .and_then(|dir| dir.old_file().path());
+        match file {
+            Some(file) => format!("{}", file.display()),
+            None => "file not displayable".to_string()
+        }
+    }).collect();
+
+    if !files.is_empty(){
+        bail!("{} uncommited or untacked files \
+        that need to be addressed before publishing. to force the \
+        publish command include --allow-untracked\nproblem files:\n{}",
+        files.len(), files.join("\n"));
+    }
+
+    Ok(())
+}
+
+fn open_git_repo() -> Result<git2::Repository, git2::Error> {
+    let current_path = Path::new(".");
+    git2::Repository::discover(current_path)
+}
+
+fn check_directory_cleanliness() -> CargoResult<()> {
+    if let Ok(repo) = open_git_repo() {
+        check_git_cleanliness(&repo)
+    } else {
+        Ok(())
+    }
+}
+
 fn verify_dependencies(pkg: &Package, registry_src: &SourceId)
                        -> CargoResult<()> {
     for dep in pkg.dependencies().iter() {

tests/test_bad_config.rs

@@ -1,5 +1,6 @@
-use support::{project, execs};
+use support::{project, execs, paths};
 use support::registry::Package;
+use support::git::repo;
 use hamcrest::assert_that;
 
 fn setup() {}
@@ -57,7 +58,8 @@ Caused by:
 });
 
 test!(bad3 {
-    let foo = project("foo")
+    let root = paths::root().join("bad3");
+    let p = repo(&root)
         .file("Cargo.toml", r#"
             [package]
             name = "foo"
@@ -69,7 +71,11 @@ test!(bad3 {
             [http]
               proxy = true
         "#);
-    assert_that(foo.cargo_process("publish").arg("-v"),
+    p.build();
+
+    let mut cargo = ::cargo_process();
+    cargo.cwd(p.root());
+    assert_that(cargo.arg("publish").arg("-v"),
                 execs().with_status(101).with_stderr("\
 [ERROR] invalid configuration for key `http.proxy`
 expected a string, but found a boolean in [..]config

tests/test_cargo_publish.rs

@@ -7,7 +7,7 @@ use flate2::read::GzDecoder;
 use tar::Archive;
 use url::Url;
 
-use support::{project, execs};
+use support::execs;
 use support::paths;
 use support::git::repo;
 
@@ -36,8 +36,100 @@ fn setup() {
         .build();
 }
 
+test!(uncommited_git_files_allowed {
+    let root = paths::root().join("uncommited_git_files_allowed");
+    let p = repo(&root)
+        .file("Cargo.toml", r#"
+            [project]
+            name = "foo"
+            version = "0.0.1"
+            authors = []
+            license = "MIT"
+            description = "foo"
+        "#)
+        .nocommit_file("bad","file")
+        .file("src/main.rs", "fn main() {}");
+    p.build();
+    let mut cargo = ::cargo_process();
+    cargo.cwd(p.root());
+    assert_that(cargo.arg("publish").arg("--allow-untracked").arg("--no-verify"),
+                execs().with_status(0).with_stdout(&format!("\
+[UPDATING] registry `{reg}`
+[PACKAGING] foo v0.0.1 ({dir})
+[UPLOADING] foo v0.0.1 ({dir})",
+        dir = p.url(),
+        reg = registry())));
+});
+
+test!(uncommited_git_files_error_from_sub_crate {
+    let root = paths::root().join("sub_uncommited_git");
+    let p = repo(&root)
+        .file("Cargo.toml", r#"
+            [project]
+            name = "foo"
+            version = "0.0.1"
+            authors = []
+            license = "MIT"
+            description = "foo"
+        "#)
+        .nocommit_file("bad.txt","file")
+        .file("src/main.rs", "fn main() {}")
+        .file("sub/lib.rs", "pub fn l() {}")
+        .file("sub/Cargo.toml", r#"
+            [package]
+            name = "crates-io"
+            version = "0.2.0"
+            authors = []
+            license = "MIT/Apache-2.0"
+            repository = "https://github.com/rust-lang/cargo"
+            description = """
+            """
+
+            [lib]
+            name = "crates_io"
+            path = "lib.rs"
+        "#);
+    p.build();
+
+    let mut cargo = ::cargo_process();
+    cargo.cwd(p.root().join("sub"));
+    assert_that(cargo.arg("publish").arg("--no-verify"),
+                execs().with_status(101).with_stderr(&"\
+[ERROR] 1 uncommited or untacked files that need to be addressed before \
+publishing. to force the publish command include --allow-untracked
+problem files:
+bad.txt",
+        ));
+});
+
+test!(uncommited_git_files_error {
+    let root = paths::root().join("uncommited_git_files_error");
+    let p = repo(&root)
+        .file("Cargo.toml", r#"
+            [project]
+            name = "foo"
+            version = "0.0.1"
+            authors = []
+            license = "MIT"
+            description = "foo"
+        "#)
+        .nocommit_file("bad.txt","file")
+        .file("src/main.rs", "fn main() {}");
+    p.build();
+
+    let mut cargo = ::cargo_process();
+    cargo.cwd(p.root());
+    assert_that(cargo.arg("publish").arg("--no-verify"),
+                execs().with_status(101).with_stderr_contains(&"\
+[ERROR] 1 uncommited or untacked files that need to be addressed before \
+publishing. to force the publish command include --allow-untracked
+problem files:",
+        ).with_stderr_contains(&"bad.txt"));
+});
+
 test!(simple {
-    let p = project("foo")
+    let root = paths::root().join("simple");
+    let p = repo(&root)
         .file("Cargo.toml", r#"
             [project]
             name = "foo"
@@ -47,8 +139,12 @@ test!(simple {
             description = "foo"
         "#)
         .file("src/main.rs", "fn main() {}");
+    p.build();
+
+    let mut cargo = ::cargo_process();
+    cargo.cwd(p.root());
 
-    assert_that(p.cargo_process("publish").arg("--no-verify"),
+    assert_that(cargo.arg("publish").arg("--no-verify"),
                 execs().with_status(0).with_stdout(&format!("\
 [UPDATING] registry `{reg}`
 [PACKAGING] foo v0.0.1 ({dir})
@@ -84,7 +180,8 @@ test!(simple {
 });
 
 test!(git_deps {
-    let p = project("foo")
+    let root = paths::root().join("git_deps");
+    let p = repo(&root)
         .file("Cargo.toml", r#"
             [project]
             name = "foo"
@@ -97,16 +194,20 @@ test!(git_deps {
             git = "git://path/to/nowhere"
         "#)
         .file("src/main.rs", "fn main() {}");
+    p.build();
 
-    assert_that(p.cargo_process("publish").arg("-v").arg("--no-verify"),
+    let mut cargo = ::cargo_process();
+    cargo.cwd(p.root());
+    assert_that(cargo.arg("publish").arg("-v").arg("--no-verify"),
                 execs().with_status(101).with_stderr("\
 [ERROR] all dependencies must come from the same source.
 dependency `foo` comes from git://path/to/nowhere instead
 "));
 });
 
 test!(path_dependency_no_version {
-    let p = project("foo")
+    let root = paths::root().join("path_dependency_no_version");
+    let p = repo(&root)
         .file("Cargo.toml", r#"
             [project]
             name = "foo"
@@ -126,16 +227,20 @@ test!(path_dependency_no_version {
             authors = []
         "#)
         .file("bar/src/lib.rs", "");
+    p.build();
 
-    assert_that(p.cargo_process("publish"),
+    let mut cargo = ::cargo_process();
+    cargo.cwd(p.root());
+    assert_that(cargo.arg("publish"),
                 execs().with_status(101).with_stderr("\
 [ERROR] all path dependencies must have a version specified when publishing.
 dependency `bar` does not specify a version
 "));
 });
 
 test!(unpublishable_crate {
-    let p = project("foo")
+    let root = paths::root().join("unpublishable_crate");
+    let p = repo(&root)
         .file("Cargo.toml", r#"
             [project]
             name = "foo"
@@ -146,8 +251,11 @@ test!(unpublishable_crate {
             publish = false
         "#)
         .file("src/main.rs", "fn main() {}");
+    p.build();
 
-    assert_that(p.cargo_process("publish"),
+    let mut cargo = ::cargo_process();
+    cargo.cwd(p.root());
+    assert_that(cargo.arg("publish"),
                 execs().with_status(101).with_stderr("\
 [ERROR] some crates cannot be published.
 `foo` is marked as unpublishable

tests/test_cargo_registry.rs

@@ -2,6 +2,7 @@ use std::fs::{self, File};
 use std::io::prelude::*;
 
 use support::{project, execs};
+use support::git::repo;
 use support::paths::{self, CargoPathExt};
 use support::registry::{self, Package};
 use support::git;
@@ -550,7 +551,8 @@ test!(login_with_no_cargo_dir {
 
 test!(bad_license_file {
     Package::new("foo", "1.0.0").publish();
-    let p = project("all")
+    let root = paths::root().join("bad_license_file");
+    let p = repo(&root)
         .file("Cargo.toml", r#"
             [project]
             name = "foo"
@@ -563,7 +565,11 @@ test!(bad_license_file {
         .file("src/main.rs", r#"
             fn main() {}
         "#);
-    assert_that(p.cargo_process("publish").arg("-v"),
+    p.build();
+
+    let mut cargo = ::cargo_process();
+    cargo.cwd(p.root());
+    assert_that(cargo.arg("publish").arg("-v"),
                 execs().with_status(101)
                        .with_stderr("\
 [ERROR] the license file `foo` does not exist"));