Auto merge of #7245 - ehuss:warn-dirty-submodule, r=alexcrichton

Check in package/publish if a git submodule is dirty.

This extends the "dirty" check during `cargo publish` to check files within a submodule.

It is a little crude, since it unconditionally tries to open every submodule even if it is not relevant to the current package. The performance doesn't seem too bad (~2s for rust-lang/rust with 16 submodules).

It's also a little lax, by ignoring uninitialized submodules. Presumably the verification build will fail if the submodule is not initialized. It could be more aggressive, by requiring all submodules to be initialized?

Closes #3622

Author: bors

src/cargo/ops/cargo_package.rs

@@ -267,14 +267,32 @@ fn check_repo_state(
         allow_dirty: bool,
     ) -> CargoResult<Option<String>> {
         let workdir = repo.workdir().unwrap();
+
+        let mut sub_repos = Vec::new();
+        open_submodules(repo, &mut sub_repos)?;
+        // Sort so that longest paths are first, to check nested submodules first.
+        sub_repos.sort_unstable_by(|a, b| b.0.as_os_str().len().cmp(&a.0.as_os_str().len()));
+        let submodule_dirty = |path: &Path| -> bool {
+            sub_repos
+                .iter()
+                .filter(|(sub_path, _sub_repo)| path.starts_with(sub_path))
+                .any(|(sub_path, sub_repo)| {
+                    let relative = path.strip_prefix(sub_path).unwrap();
+                    sub_repo
+                        .status_file(relative)
+                        .map(|status| status != git2::Status::CURRENT)
+                        .unwrap_or(false)
+                })
+        };
+
         let dirty = src_files
             .iter()
             .filter(|file| {
                 let relative = file.strip_prefix(workdir).unwrap();
                 if let Ok(status) = repo.status_file(relative) {
                     status != git2::Status::CURRENT
                 } else {
-                    false
+                    submodule_dirty(file)
                 }
             })
             .map(|path| {
@@ -300,6 +318,22 @@ fn check_repo_state(
             Ok(None)
         }
     }
+
+    /// Helper to recursively open all submodules.
+    fn open_submodules(
+        repo: &git2::Repository,
+        sub_repos: &mut Vec<(PathBuf, git2::Repository)>,
+    ) -> CargoResult<()> {
+        for submodule in repo.submodules()? {
+            // Ignore submodules that don't open, they are probably not initialized.
+            // If its files are required, then the verification step should fail.
+            if let Ok(sub_repo) = submodule.open() {
+                open_submodules(&sub_repo, sub_repos)?;
+                sub_repos.push((sub_repo.workdir().unwrap().to_owned(), sub_repo));
+            }
+        }
+        Ok(())
+    }
 }
 
 // Checks for and `bail!` if a source file matches `ROOT/VCS_INFO_FILE`, since

tests/testsuite/git.rs

@@ -2691,3 +2691,117 @@ fn git_fetch_cli_env_clean() {
         .env("GIT_DIR", git_proj.root().join(".git"))
         .run();
 }
+
+#[cargo_test]
+fn dirty_submodule() {
+    // `cargo package` warns for dirty file in submodule.
+    let (git_project, repo) = git::new_repo("foo", |project| {
+        project
+            .file("Cargo.toml", &basic_manifest("foo", "0.5.0"))
+            // This is necessary because `git::add` is too eager.
+            .file(".gitignore", "/target")
+    });
+    let git_project2 = git::new("src", |project| {
+        project.no_manifest().file("lib.rs", "pub fn f() {}")
+    });
+
+    let url = path2url(git_project2.root()).to_string();
+    git::add_submodule(&repo, &url, Path::new("src"));
+
+    // Submodule added, but not committed.
+    git_project
+        .cargo("package --no-verify")
+        .with_status(101)
+        .with_stderr(
+            "\
+[WARNING] manifest has no [..]
+See [..]
+[ERROR] 1 files in the working directory contain changes that were not yet committed into git:
+
+.gitmodules
+
+to proceed despite [..]
+",
+        )
+        .run();
+
+    git::commit(&repo);
+    git_project.cargo("package --no-verify").run();
+
+    // Modify file, check for warning.
+    git_project.change_file("src/lib.rs", "");
+    git_project
+        .cargo("package --no-verify")
+        .with_status(101)
+        .with_stderr(
+            "\
+[WARNING] manifest has no [..]
+See [..]
+[ERROR] 1 files in the working directory contain changes that were not yet committed into git:
+
+src/lib.rs
+
+to proceed despite [..]
+",
+        )
+        .run();
+    // Commit the change.
+    let sub_repo = git2::Repository::open(git_project.root().join("src")).unwrap();
+    git::add(&sub_repo);
+    git::commit(&sub_repo);
+    git::add(&repo);
+    git::commit(&repo);
+    git_project.cargo("package --no-verify").run();
+
+    // Try with a nested submodule.
+    let git_project3 = git::new("bar", |project| project.no_manifest().file("mod.rs", ""));
+    let url = path2url(git_project3.root()).to_string();
+    git::add_submodule(&sub_repo, &url, Path::new("bar"));
+    git_project
+        .cargo("package --no-verify")
+        .with_status(101)
+        .with_stderr(
+            "\
+[WARNING] manifest has no [..]
+See [..]
+[ERROR] 1 files in the working directory contain changes that were not yet committed into git:
+
+src/.gitmodules
+
+to proceed despite [..]
+",
+        )
+        .run();
+
+    // Commit the submodule addition.
+    git::commit(&sub_repo);
+    git::add(&repo);
+    git::commit(&repo);
+    git_project.cargo("package --no-verify").run();
+    // Modify within nested submodule.
+    git_project.change_file("src/bar/mod.rs", "//test");
+    git_project
+        .cargo("package --no-verify")
+        .with_status(101)
+        .with_stderr(
+            "\
+[WARNING] manifest has no [..]
+See [..]
+[ERROR] 1 files in the working directory contain changes that were not yet committed into git:
+
+src/bar/mod.rs
+
+to proceed despite [..]
+",
+        )
+        .run();
+    // And commit the change.
+    let sub_sub_repo = git2::Repository::open(git_project.root().join("src/bar")).unwrap();
+    git::add(&sub_sub_repo);
+    git::commit(&sub_sub_repo);
+    git::add(&sub_repo);
+    git::commit(&sub_repo);
+    git::add(&repo);
+    git::commit(&repo);
+    git_project.cargo("package --no-verify").run();
+}

tests/testsuite/support/git.rs

@@ -129,11 +129,14 @@ impl Repository {
 /// Initialize a new repository at the given path.
 pub fn init(path: &Path) -> git2::Repository {
     let repo = t!(git2::Repository::init(path));
+    default_repo_cfg(&repo);
+    repo
+}
+
+fn default_repo_cfg(repo: &git2::Repository) {
     let mut cfg = t!(repo.config());
     t!(cfg.set_str("user.email", "foo@bar.com"));
     t!(cfg.set_str("user.name", "Foo Bar"));
-    drop(cfg);
-    repo
 }
 
 /// Create a new git repository with a project.
@@ -193,6 +196,7 @@ pub fn add_submodule<'a>(
     let path = path.to_str().unwrap().replace(r"\", "/");
     let mut s = t!(repo.submodule(url, Path::new(&path), false));
     let subrepo = t!(s.open());
+    default_repo_cfg(&subrepo);
     t!(subrepo.remote_add_fetch("origin", "refs/heads/*:refs/heads/*"));
     let mut origin = t!(subrepo.find_remote("origin"));
     t!(origin.fetch(&[], None, None));