From f0907fca16a84101d69fc53272a955e8bb53d9fe Mon Sep 17 00:00:00 2001
From: Weihang Lo <me@weihanglo.tw>
Date: Thu, 6 Mar 2025 23:12:42 -0500
Subject: [PATCH] fix(package): report if the lockfile is dirty

Lockfile might not be under the package root, but its entries
may be outdated and get packaged into the `.crate` tarball.
We take a conservative action that if the lockfile is dirty,
then all workspace members are considered dirty.
---
 src/cargo/ops/cargo_package/vcs.rs | 38 +++++++++++++++++++++++++++++-
 tests/testsuite/package.rs         | 26 +++++++++++++-------
 2 files changed, 55 insertions(+), 9 deletions(-)

diff --git a/src/cargo/ops/cargo_package/vcs.rs b/src/cargo/ops/cargo_package/vcs.rs
index 4b5bd942c..b936c6583 100644
--- a/src/cargo/ops/cargo_package/vcs.rs
+++ b/src/cargo/ops/cargo_package/vcs.rs
@@ -11,6 +11,7 @@ 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;
@@ -235,10 +236,15 @@ 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,
@@ -248,6 +254,8 @@ fn dirty_files_outside_pkg_root(
     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()
@@ -255,13 +263,41 @@ fn dirty_files_outside_pkg_root(
         .map(|path| paths::normalize_path(&pkg_root.join(path)))
         .collect();
 
-    let mut dirty_files = 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().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,
diff --git a/tests/testsuite/package.rs b/tests/testsuite/package.rs
index 6499ad809..4609bf79c 100644
--- a/tests/testsuite/package.rs
+++ b/tests/testsuite/package.rs
@@ -1465,9 +1465,13 @@ fn dirty_ws_lockfile_dirty() {
     // lockfile is untracked.
     p.cargo("generate-lockfile").run();
     p.cargo("package --workspace --no-verify")
+        .with_status(101)
         .with_stderr_data(str![[r#"
-[PACKAGING] isengard v0.0.0 ([ROOT]/foo/isengard)
-[PACKAGED] 5 files, [FILE_SIZE]B ([FILE_SIZE]B compressed)
+[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();
@@ -1522,10 +1526,13 @@ dependencies = [
 "#]])
         .run();
     p.cargo("package --workspace --no-verify")
+        .with_status(101)
         .with_stderr_data(str![[r#"
-[PACKAGING] isengard v0.0.0 ([ROOT]/foo/isengard)
-[UPDATING] `dummy-registry` index
-[PACKAGED] 5 files, [FILE_SIZE]B ([FILE_SIZE]B compressed)
+[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();
@@ -1534,10 +1541,13 @@ dependencies = [
     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#"
-[PACKAGING] isengard v0.0.0 ([ROOT]/foo/isengard)
-[UPDATING] `dummy-registry` index
-[PACKAGED] 5 files, [FILE_SIZE]B ([FILE_SIZE]B compressed)
+[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();