fix(package): report lockfile / workspace manifest is dirty (#15276)

### What does this PR try to resolve?

Workspace manifest and lockfile might not be under the package root,
but workspace inheritance and outdated lockfile entries
may still affect what is packaged into the `.crate.` tarball.
We take a conservative action that if any of them is dirty,
then all workspace members are considered dirty.

Fixes #14967

### How should we test and review this PR?

Workspace manifest case is simple.

Lockfile requires a bit more works.
It is allowed to be missing, and can be generated during packaging.
We skip checking when it is missing in both workdir and git index,
otherwise cargo will fail with git2 not found error.

Author: epage

src/cargo/ops/cargo_package/mod.rs

@@ -384,7 +384,7 @@ fn prepare_archive(
     let src_files = src.list_files(pkg)?;
 
     // Check (git) repository state, getting the current commit hash.
-    let vcs_info = vcs::check_repo_state(pkg, &src_files, gctx, &opts)?;
+    let vcs_info = vcs::check_repo_state(pkg, &src_files, ws, &opts)?;
 
     build_ar_list(ws, pkg, src_files, vcs_info)
 }

src/cargo/ops/cargo_package/vcs.rs

@@ -10,6 +10,8 @@ use serde::Serialize;
 use tracing::debug;
 
 use crate::core::Package;
+use crate::core::Workspace;
+use crate::ops::lockfile::LOCKFILE_NAME;
 use crate::sources::PathEntry;
 use crate::CargoResult;
 use crate::GlobalContext;
@@ -44,12 +46,16 @@ pub struct GitVcsInfo {
 pub fn check_repo_state(
     p: &Package,
     src_files: &[PathEntry],
-    gctx: &GlobalContext,
+    ws: &Workspace<'_>,
     opts: &PackageOpts<'_>,
 ) -> CargoResult<Option<VcsInfo>> {
+    let gctx = ws.gctx();
     let Ok(repo) = git2::Repository::discover(p.root()) else {
         gctx.shell().verbose(|shell| {
-            shell.warn(format!("no (git) VCS found for `{}`", p.root().display()))
+            shell.warn(format_args!(
+                "no (git) VCS found for `{}`",
+                p.root().display()
+            ))
         })?;
         // No Git repo found. Have to assume it is clean.
         return Ok(None);
@@ -69,7 +75,7 @@ pub fn check_repo_state(
     let path = paths::strip_prefix_canonical(path, workdir).unwrap_or_else(|_| path.to_path_buf());
     let Ok(status) = repo.status_file(&path) else {
         gctx.shell().verbose(|shell| {
-            shell.warn(format!(
+            shell.warn(format_args!(
                 "no (git) Cargo.toml found at `{}` in workdir `{}`",
                 path.display(),
                 workdir.display()
@@ -82,7 +88,7 @@ pub fn check_repo_state(
 
     if !(status & git2::Status::IGNORED).is_empty() {
         gctx.shell().verbose(|shell| {
-            shell.warn(format!(
+            shell.warn(format_args!(
                 "found (git) Cargo.toml ignored at `{}` in workdir `{}`",
                 path.display(),
                 workdir.display()
@@ -100,16 +106,17 @@ pub fn check_repo_state(
         path.display(),
         workdir.display(),
     );
+    let Some(git) = git(ws, p, src_files, &repo, &opts)? else {
+        // If the git repo lacks essensial field like `sha1`, and since this field exists from the beginning,
+        // then don't generate the corresponding file in order to maintain consistency with past behavior.
+        return Ok(None);
+    };
+
     let path_in_vcs = path
         .parent()
         .and_then(|p| p.to_str())
         .unwrap_or("")
         .replace("\\", "/");
-    let Some(git) = git(p, gctx, src_files, &repo, &opts)? else {
-        // If the git repo lacks essensial field like `sha1`, and since this field exists from the beginning,
-        // then don't generate the corresponding file in order to maintain consistency with past behavior.
-        return Ok(None);
-    };
 
     return Ok(Some(VcsInfo { git, path_in_vcs }));
 }
@@ -162,8 +169,8 @@ fn warn_symlink_checked_out_as_plain_text_file(
 
 /// The real git status check starts from here.
 fn git(
+    ws: &Workspace<'_>,
     pkg: &Package,
-    gctx: &GlobalContext,
     src_files: &[PathEntry],
     repo: &git2::Repository,
     opts: &PackageOpts<'_>,
@@ -184,12 +191,12 @@ fn git(
     // 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 cwd = gctx.cwd();
+    let cwd = ws.gctx().cwd();
     let mut dirty_src_files: Vec<_> = src_files
         .iter()
         .filter(|src_file| dirty_files.iter().any(|path| src_file.starts_with(path)))
         .map(|p| p.as_ref())
-        .chain(dirty_files_outside_pkg_root(pkg, repo, src_files)?.iter())
+        .chain(dirty_files_outside_pkg_root(ws, pkg, repo, src_files)?.iter())
         .map(|path| {
             pathdiff::diff_paths(path, cwd)
                 .as_ref()
@@ -228,41 +235,79 @@ fn git(
 ///
 /// * `package.readme` and `package.license-file` pointing to paths outside package root
 /// * symlinks targets reside outside package root
+/// * Any change in the root workspace manifest, regardless of what has changed.
+/// * Changes in the lockfile [^1].
 ///
 /// This is required because those paths may link to a file outside the
 /// current package root, but still under the git workdir, affecting the
 /// final packaged `.crate` file.
+///
+/// [^1]: Lockfile might be re-generated if it is too out of sync with the manifest.
+///       Therefore, even you have a modified lockfile,
+///       you might still get a new fresh one that matches what is in git index.
 fn dirty_files_outside_pkg_root(
+    ws: &Workspace<'_>,
     pkg: &Package,
     repo: &git2::Repository,
     src_files: &[PathEntry],
 ) -> CargoResult<HashSet<PathBuf>> {
     let pkg_root = pkg.root();
     let workdir = repo.workdir().unwrap();
 
+    let mut dirty_files = HashSet::new();
+
     let meta = pkg.manifest().metadata();
     let metadata_paths: Vec<_> = [&meta.license_file, &meta.readme]
         .into_iter()
         .filter_map(|p| p.as_deref())
         .map(|path| paths::normalize_path(&pkg_root.join(path)))
         .collect();
 
-    let mut dirty_symlinks = HashSet::new();
+    // Unlike other files, lockfile is allowed to be missing,
+    // and can be generated during packaging.
+    // We skip checking when it is missing in both workdir and git index,
+    // otherwise cargo will fail with git2 not found error.
+    let lockfile_path = ws.lock_root().as_path_unlocked().join(LOCKFILE_NAME);
+    let lockfile_path = if lockfile_path.exists() {
+        Some(lockfile_path)
+    } else if let Ok(rel_path) = paths::normalize_path(&lockfile_path).strip_prefix(workdir) {
+        // We don't canonicalize here because non-existing path can't be canonicalized.
+        match repo.status_file(&rel_path) {
+            Ok(s) if s != git2::Status::CURRENT => {
+                dirty_files.insert(lockfile_path);
+            }
+            // Unmodified
+            Ok(_) => {}
+            Err(e) => {
+                debug!(
+                    "check git status failed for `{}` in workdir `{}`: {e}",
+                    rel_path.display(),
+                    workdir.display(),
+                );
+            }
+        }
+        None
+    } else {
+        None
+    };
+
     for rel_path in src_files
         .iter()
         .filter(|p| p.is_symlink_or_under_symlink())
-        .map(|p| p.as_ref())
-        .chain(metadata_paths.iter())
+        .map(|p| p.as_ref().as_path())
+        .chain(metadata_paths.iter().map(AsRef::as_ref))
+        .chain([ws.root_manifest()])
+        .chain(lockfile_path.as_deref().into_iter())
         // If inside package root. Don't bother checking git status.
         .filter(|p| paths::strip_prefix_canonical(p, pkg_root).is_err())
         // Handle files outside package root but under git workdir,
         .filter_map(|p| paths::strip_prefix_canonical(p, workdir).ok())
     {
         if repo.status_file(&rel_path)? != git2::Status::CURRENT {
-            dirty_symlinks.insert(workdir.join(rel_path));
+            dirty_files.insert(workdir.join(rel_path));
         }
     }
-    Ok(dirty_symlinks)
+    Ok(dirty_files)
 }
 
 /// Helper to collect dirty statuses for a single repo.

tests/testsuite/package.rs

@@ -3,6 +3,7 @@
 use std::fs::{self, read_to_string, File};
 use std::path::Path;
 
+use cargo_test_support::compare::assert_e2e;
 use cargo_test_support::prelude::*;
 use cargo_test_support::publish::validate_crate_contents;
 use cargo_test_support::registry::{self, Package};
@@ -1212,15 +1213,6 @@ fn vcs_status_check_for_each_workspace_member() {
     });
     git::commit(&repo);
 
-    p.change_file(
-        "Cargo.toml",
-        r#"
-            [workspace]
-            members = ["isengard", "mordor"]
-            [workspace.package]
-            edition = "2021"
-        "#,
-    );
     // Dirty file outside won't affect packaging.
     p.change_file("hobbit", "changed!");
     p.change_file("mordor/src/lib.rs", "changed!");
@@ -1357,7 +1349,8 @@ fn dirty_file_outside_pkg_root_considered_dirty() {
     p.change_file("original-dir/file", "after");
     // * Changes in files outside pkg root that `license-file`/`readme` point to
     p.change_file("LICENSE", "after");
-    // * When workspace inheritance is involved and changed
+    // * When workspace root manifest has changned,
+    //   no matter whether workspace inheritance is involved.
     p.change_file(
         "Cargo.toml",
         r#"
@@ -1378,8 +1371,9 @@ fn dirty_file_outside_pkg_root_considered_dirty() {
     p.cargo("package --workspace --no-verify")
         .with_status(101)
         .with_stderr_data(str![[r#"
-[ERROR] 4 files in the working directory contain changes that were not yet committed into git:
+[ERROR] 5 files in the working directory contain changes that were not yet committed into git:
 
+Cargo.toml
 LICENSE
 README.md
 lib.rs
@@ -1432,6 +1426,133 @@ edition = "2021"
     );
 }
 
+#[cargo_test]
+fn dirty_ws_lockfile_dirty() {
+    let (p, repo) = git::new_repo("foo", |p| {
+        p.file(
+            "Cargo.toml",
+            r#"
+                [workspace]
+                members = ["isengard"]
+                resolver = "2"
+                [workspace.package]
+                edition = "2015"
+            "#,
+        )
+        .file(
+            "isengard/Cargo.toml",
+            r#"
+                [package]
+                name = "isengard"
+                edition.workspace = true
+                homepage = "saruman"
+                description = "saruman"
+                license = "MIT"
+            "#,
+        )
+        .file("isengard/src/lib.rs", "")
+    });
+    git::commit(&repo);
+
+    p.cargo("package --workspace --no-verify")
+        .with_stderr_data(str![[r#"
+[PACKAGING] isengard v0.0.0 ([ROOT]/foo/isengard)
+[PACKAGED] 5 files, [FILE_SIZE]B ([FILE_SIZE]B compressed)
+
+"#]])
+        .run();
+
+    // lockfile is untracked.
+    p.cargo("generate-lockfile").run();
+    p.cargo("package --workspace --no-verify")
+        .with_status(101)
+        .with_stderr_data(str![[r#"
+[ERROR] 1 files in the working directory contain changes that were not yet committed into git:
+
+Cargo.lock
+
+to proceed despite this and include the uncommitted changes, pass the `--allow-dirty` flag
+
+"#]])
+        .run();
+
+    // lockfile is tracked.
+    p.cargo("clean").run();
+    git::add(&repo);
+    git::commit(&repo);
+    p.cargo("package --workspace --no-verify")
+        .with_stderr_data(str![[r#"
+[PACKAGING] isengard v0.0.0 ([ROOT]/foo/isengard)
+[PACKAGED] 5 files, [FILE_SIZE]B ([FILE_SIZE]B compressed)
+
+"#]])
+        .run();
+
+    // Simulate that Cargo.lock had some outdated but SemVer compat dependency changes.
+    Package::new("dep", "1.0.0").publish();
+    Package::new("dep", "1.1.0").publish();
+    p.cargo("clean").run();
+    p.cargo("add dep@1").run();
+    git::add(&repo);
+    git::commit(&repo);
+    // make sure we have [email protected] in lockfile
+    assert_e2e().eq(
+        &p.read_lockfile(),
+        str![[r##"
+# This file is automatically @generated by Cargo.
+# It is not intended for manual editing.
+version = 4
+
+[[package]]
+name = "dep"
+version = "1.1.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "77d3d6a4f2203d590707cc803c94afbe36393bbdba757ef66986f39159eaab51"
+
+[[package]]
+name = "isengard"
+version = "0.0.0"
+dependencies = [
+ "dep",
+]
+
+"##]],
+    );
+    p.cargo("update dep --precise 1.0.0")
+        .with_stderr_data(str![[r#"
+[UPDATING] `dummy-registry` index
+[DOWNGRADING] dep v1.1.0 -> v1.0.0
+
+"#]])
+        .run();
+    p.cargo("package --workspace --no-verify")
+        .with_status(101)
+        .with_stderr_data(str![[r#"
+[ERROR] 1 files in the working directory contain changes that were not yet committed into git:
+
+Cargo.lock
+
+to proceed despite this and include the uncommitted changes, pass the `--allow-dirty` flag
+
+"#]])
+        .run();
+
+    // Now check if it is considered dirty when removed.
+    p.cargo("clean").run();
+    fs::remove_file(p.root().join("Cargo.lock")).unwrap();
+    p.cargo("package --workspace --no-verify")
+        .with_status(101)
+        .with_stderr_data(str![[r#"
+[ERROR] 1 files in the working directory contain changes that were not yet committed into git:
+
+Cargo.lock
+
+to proceed despite this and include the uncommitted changes, pass the `--allow-dirty` flag
+
+"#]])
+        .run();
+}
+
 #[cargo_test]
 fn issue_13695_allow_dirty_vcs_info() {
     let p = project()