Auto merge of #9478 - ehuss:faster-git-status, r=alexcrichton

Improve performance of git status check in `cargo package`.

The check for a dirty repository during packaging/publishing is quite slow. It was calling `status_file` for every packaged file, which is very expensive. I have a directory that had about 10,000 untracked files. Previously, cargo would hang for over 2 minutes without any output. With this PR, it finishes in 0.3 seconds.

The solution here is to collect the status information once, and then compare the package list against it.

One subtle point is that it does not use `recurse_untracked_dirs`, and instead relies on a primitive `starts_with` comparison, which I believe should be equivalent.

This still includes an inefficient n^2 algorithm, but I am too lazy to make a better approach.

I'm moderately confident this is pretty much the same as before (at least, all the scenarios I could think of).

Author: bors

src/cargo/ops/cargo_package.rs

@@ -394,76 +394,89 @@ fn check_repo_state(
         src_files: &[PathBuf],
         repo: &git2::Repository,
     ) -> 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
+        // This is a collection of any dirty or untracked files. This covers:
+        // - new/modified/deleted/renamed/type change (index or worktree)
+        // - untracked files (which are "new" worktree files)
+        // - ignored (in case the user has an `include` directive that
+        //   conflicts with .gitignore).
+        let mut dirty_files = Vec::new();
+        collect_statuses(repo, &mut dirty_files)?;
+        // Include each submodule so that the error message can provide
+        // specifically *which* files in a submodule are modified.
+        status_submodules(repo, &mut dirty_files)?;
+
+        // Find the intersection of dirty in git, and the src_files that would
+        // be packaged. This is a lazy n^2 check, but seems fine with
+        // thousands of files.
+        let dirty_src_files: Vec<String> = src_files
             .iter()
-            .filter(|file| {
-                let relative = file.strip_prefix(workdir).unwrap();
-                if let Ok(status) = repo.status_file(relative) {
-                    if status == git2::Status::CURRENT {
-                        false
-                    } else if relative.file_name().and_then(|s| s.to_str()).unwrap_or("")
-                        == "Cargo.lock"
-                    {
-                        // It is OK to include this file even if it is ignored.
-                        status != git2::Status::IGNORED
-                    } else {
-                        true
-                    }
-                } else {
-                    submodule_dirty(file)
-                }
-            })
+            .filter(|src_file| dirty_files.iter().any(|path| src_file.starts_with(path)))
             .map(|path| {
                 path.strip_prefix(p.root())
                     .unwrap_or(path)
                     .display()
                     .to_string()
             })
-            .collect::<Vec<_>>();
-        if dirty.is_empty() {
+            .collect();
+        if dirty_src_files.is_empty() {
             let rev_obj = repo.revparse_single("HEAD")?;
             Ok(Some(rev_obj.id().to_string()))
         } else {
             anyhow::bail!(
                 "{} files in the working directory contain changes that were \
                  not yet committed into git:\n\n{}\n\n\
                  to proceed despite this and include the uncommitted changes, pass the `--allow-dirty` flag",
-                dirty.len(),
-                dirty.join("\n")
+                dirty_src_files.len(),
+                dirty_src_files.join("\n")
             )
         }
     }
 
-    /// Helper to recursively open all submodules.
-    fn open_submodules(
+    // Helper to collect dirty statuses for a single repo.
+    fn collect_statuses(
+        repo: &git2::Repository,
+        dirty_files: &mut Vec<PathBuf>,
+    ) -> CargoResult<()> {
+        let mut status_opts = git2::StatusOptions::new();
+        // Exclude submodules, as they are being handled manually by recursing
+        // into each one so that details about specific files can be
+        // retrieved.
+        status_opts
+            .exclude_submodules(true)
+            .include_ignored(true)
+            .include_untracked(true);
+        let repo_statuses = repo.statuses(Some(&mut status_opts)).with_context(|| {
+            format!(
+                "failed to retrieve git status from repo {}",
+                repo.path().display()
+            )
+        })?;
+        let workdir = repo.workdir().unwrap();
+        let this_dirty = repo_statuses.iter().filter_map(|entry| {
+            let path = entry.path().expect("valid utf-8 path");
+            if path.ends_with("Cargo.lock") && entry.status() == git2::Status::IGNORED {
+                // It is OK to include Cargo.lock even if it is ignored.
+                return None;
+            }
+            // Use an absolute path, so that comparing paths is easier
+            // (particularly with submodules).
+            Some(workdir.join(path))
+        });
+        dirty_files.extend(this_dirty);
+        Ok(())
+    }
+
+    // Helper to collect dirty statuses while recursing into submodules.
+    fn status_submodules(
         repo: &git2::Repository,
-        sub_repos: &mut Vec<(PathBuf, git2::Repository)>,
+        dirty_files: &mut Vec<PathBuf>,
     ) -> 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));
+                status_submodules(&sub_repo, dirty_files)?;
+                collect_statuses(&sub_repo, dirty_files)?;
             }
         }
         Ok(())

tests/testsuite/git.rs

@@ -2760,7 +2760,7 @@ to proceed despite [..]
     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.change_file("src/bar/new_file.rs", "//test");
     git_project
         .cargo("package --no-verify")
         .with_status(101)
@@ -2770,7 +2770,7 @@ to proceed despite [..]
 See [..]
 [ERROR] 1 files in the working directory contain changes that were not yet committed into git:
 
-src/bar/mod.rs
+src/bar/new_file.rs
 
 to proceed despite [..]
 ",

tests/testsuite/package.rs

@@ -816,6 +816,59 @@ to proceed despite this and include the uncommitted changes, pass the `--allow-d
         .run();
 }
 
+#[cargo_test]
+fn dirty_ignored() {
+    // Cargo warns about an ignored file that will be published.
+    let (p, repo) = git::new_repo("foo", |p| {
+        p.file(
+            "Cargo.toml",
+            r#"
+                [package]
+                name = "foo"
+                version = "0.1.0"
+                description = "foo"
+                license = "foo"
+                documentation = "foo"
+                include = ["src", "build"]
+            "#,
+        )
+        .file("src/lib.rs", "")
+        .file(".gitignore", "build")
+    });
+    // Example of adding a file that is confusingly ignored by an overzealous
+    // gitignore rule.
+    p.change_file("src/build/mod.rs", "");
+    p.cargo("package --list")
+        .with_status(101)
+        .with_stderr(
+            "\
+error: 1 files in the working directory contain changes that were not yet committed into git:
+
+src/build/mod.rs
+
+to proceed despite this and include the uncommitted changes, pass the `--allow-dirty` flag
+",
+        )
+        .run();
+    // Add the ignored file and make sure it is included.
+    let mut index = t!(repo.index());
+    t!(index.add_path(Path::new("src/build/mod.rs")));
+    t!(index.write());
+    git::commit(&repo);
+    p.cargo("package --list")
+        .with_stderr("")
+        .with_stdout(
+            "\
+.cargo_vcs_info.json
+Cargo.toml
+Cargo.toml.orig
+src/build/mod.rs
+src/lib.rs
+",
+        )
+        .run();
+}
+
 #[cargo_test]
 fn generated_manifest() {
     registry::alt_init();