From 7d202307cebba894a2c95e9a16b9f8e31b7501f2 Mon Sep 17 00:00:00 2001
From: Eric Huss <eric@huss.org>
Date: Wed, 17 Apr 2019 14:06:05 -0700
Subject: [PATCH] Include Cargo.lock in package checks.

This includes `Cargo.lock` in the git dirty check. It explicitly excludes
`Cargo.lock` as an untracked file, since it is not relevant for the dirty check;
it is only checked if it is committed to git.

This also adds `Cargo.lock` to the "did anything modify this" check during
verification. I don't see a reason to exclude it (particularly since ephemeral
workspaces do not save the lock file).

Also add "Archiving: Cargo.lock" to the verbose output.
---
 src/cargo/core/package.rs           |  6 ++++++
 src/cargo/ops/cargo_package.rs      | 28 ++++++++++++++++------------
 src/cargo/sources/path.rs           | 25 ++++++++++++++++---------
 tests/testsuite/publish_lockfile.rs | 18 ++++++++++++++++++
 4 files changed, 56 insertions(+), 21 deletions(-)

diff --git a/src/cargo/core/package.rs b/src/cargo/core/package.rs
index 389a5ab93ec..d5f7d89fafc 100644
--- a/src/cargo/core/package.rs
+++ b/src/cargo/core/package.rs
@@ -236,6 +236,12 @@ impl Package {
             toml
         ))
     }
+
+    /// Returns if package should include `Cargo.lock`.
+    pub fn include_lockfile(&self) -> bool {
+        self.manifest().publish_lockfile()
+            && self.targets().iter().any(|t| t.is_example() || t.is_bin())
+    }
 }
 
 impl fmt::Display for Package {
diff --git a/src/cargo/ops/cargo_package.rs b/src/cargo/ops/cargo_package.rs
index 937afd735fb..edb7aca9cbd 100644
--- a/src/cargo/ops/cargo_package.rs
+++ b/src/cargo/ops/cargo_package.rs
@@ -79,7 +79,8 @@ pub fn package(ws: &Workspace<'_>, opts: &PackageOpts<'_>) -> CargoResult<Option
             .iter()
             .map(|file| file.strip_prefix(root).unwrap().to_path_buf())
             .collect();
-        if include_lockfile(pkg) {
+        if pkg.include_lockfile() && !list.contains(&PathBuf::from("Cargo.lock")) {
+            // A generated Cargo.lock will be included.
             list.push("Cargo.lock".into());
         }
         if vcs_info.is_some() {
@@ -140,8 +141,7 @@ fn build_lock(ws: &Workspace<'_>) -> CargoResult<String> {
     // Regenerate Cargo.lock using the old one as a guide.
     let specs = vec![PackageIdSpec::from_package_id(new_pkg.package_id())];
     let tmp_ws = Workspace::ephemeral(new_pkg, ws.config(), None, true)?;
-    let (pkg_set, new_resolve) =
-        ops::resolve_ws_with_method(&tmp_ws, Method::Everything, &specs)?;
+    let (pkg_set, new_resolve) = ops::resolve_ws_with_method(&tmp_ws, Method::Everything, &specs)?;
 
     if let Some(orig_resolve) = orig_resolve {
         compare_resolve(config, tmp_ws.current()?, &orig_resolve, &new_resolve)?;
@@ -151,10 +151,6 @@ fn build_lock(ws: &Workspace<'_>) -> CargoResult<String> {
     ops::resolve_to_string(&tmp_ws, &new_resolve)
 }
 
-fn include_lockfile(pkg: &Package) -> bool {
-    pkg.manifest().publish_lockfile() && pkg.targets().iter().any(|t| t.is_example() || t.is_bin())
-}
-
 // Checks that the package has some piece of metadata that a human can
 // use to tell what the package is about.
 fn check_metadata(pkg: &Package, config: &Config) -> CargoResult<()> {
@@ -337,6 +333,10 @@ fn tar(
         let relative_str = relative.to_str().ok_or_else(|| {
             failure::format_err!("non-utf8 path in source directory: {}", relative.display())
         })?;
+        if relative_str == "Cargo.lock" {
+            // This is added manually below.
+            continue;
+        }
         config
             .shell()
             .verbose(|shell| shell.status("Archiving", &relative_str))?;
@@ -432,9 +432,12 @@ fn tar(
             .chain_err(|| internal(format!("could not archive source file `{}`", fnd)))?;
     }
 
-    if include_lockfile(pkg) {
+    if pkg.include_lockfile() {
         let new_lock = build_lock(&ws)?;
 
+        config
+            .shell()
+            .verbose(|shell| shell.status("Archiving", "Cargo.lock"))?;
         let path = format!(
             "{}-{}{}Cargo.lock",
             pkg.name(),
@@ -534,7 +537,10 @@ fn compare_resolve(
                 )
             }
         };
-        let msg = format!("package `{}` added to the packaged Cargo.lock file{}", pkg_id, extra);
+        let msg = format!(
+            "package `{}` added to the packaged Cargo.lock file{}",
+            pkg_id, extra
+        );
         config.shell().status_with_color("Note", msg, Color::Cyan)?;
     }
     Ok(())
@@ -625,9 +631,7 @@ fn hash_all(path: &Path) -> CargoResult<HashMap<PathBuf, u64>> {
     fn wrap(path: &Path) -> CargoResult<HashMap<PathBuf, u64>> {
         let mut result = HashMap::new();
         let walker = walkdir::WalkDir::new(path).into_iter();
-        for entry in walker.filter_entry(|e| {
-            !(e.depth() == 1 && (e.file_name() == "target" || e.file_name() == "Cargo.lock"))
-        }) {
+        for entry in walker.filter_entry(|e| !(e.depth() == 1 && e.file_name() == "target")) {
             let entry = entry?;
             let file_type = entry.file_type();
             if file_type.is_file() {
diff --git a/src/cargo/sources/path.rs b/src/cargo/sources/path.rs
index a425421b5ff..700f02391ad 100644
--- a/src/cargo/sources/path.rs
+++ b/src/cargo/sources/path.rs
@@ -240,8 +240,13 @@ impl<'cfg> PathSource<'cfg> {
                 }
             }
 
-            // Update to `ignore_should_package` for Stage 2.
-            Ok(glob_should_package)
+            let should_include = match path.file_name().and_then(|s| s.to_str()) {
+                Some("Cargo.lock") => pkg.include_lockfile(),
+                // Update to `ignore_should_package` for Stage 2.
+                _ => glob_should_package,
+            };
+
+            Ok(should_include)
         };
 
         // Attempt Git-prepopulate only if no `include` (see rust-lang/cargo#4135).
@@ -332,7 +337,11 @@ impl<'cfg> PathSource<'cfg> {
         }
         let statuses = repo.statuses(Some(&mut opts))?;
         let untracked = statuses.iter().filter_map(|entry| match entry.status() {
-            git2::Status::WT_NEW => Some((join(root, entry.path_bytes()), None)),
+            // Don't include Cargo.lock if it is untracked. Packaging will
+            // generate a new one as needed.
+            git2::Status::WT_NEW if entry.path() != Some("Cargo.lock") => {
+                Some((join(root, entry.path_bytes()), None))
+            }
             _ => None,
         });
 
@@ -342,17 +351,15 @@ impl<'cfg> PathSource<'cfg> {
             let file_path = file_path?;
 
             // Filter out files blatantly outside this package. This is helped a
-            // bit obove via the `pathspec` function call, but we need to filter
+            // bit above via the `pathspec` function call, but we need to filter
             // the entries in the index as well.
             if !file_path.starts_with(pkg_path) {
                 continue;
             }
 
             match file_path.file_name().and_then(|s| s.to_str()) {
-                // Filter out `Cargo.lock` and `target` always; we don't want to
-                // package a lock file no one will ever read and we also avoid
-                // build artifacts.
-                Some("Cargo.lock") | Some("target") => continue,
+                // The `target` directory is never included.
+                Some("target") => continue,
 
                 // Keep track of all sub-packages found and also strip out all
                 // matches we've found so far. Note, though, that if we find
@@ -467,7 +474,7 @@ impl<'cfg> PathSource<'cfg> {
             if is_root {
                 // Skip Cargo artifacts.
                 match name {
-                    Some("target") | Some("Cargo.lock") => continue,
+                    Some("target") => continue,
                     _ => {}
                 }
             }
diff --git a/tests/testsuite/publish_lockfile.rs b/tests/testsuite/publish_lockfile.rs
index 060aacb52a6..75c7e8dcf29 100644
--- a/tests/testsuite/publish_lockfile.rs
+++ b/tests/testsuite/publish_lockfile.rs
@@ -87,6 +87,23 @@ fn package_lockfile_git_repo() {
 Cargo.lock
 Cargo.toml
 src/main.rs
+",
+        )
+        .run();
+    cargo_process("package -v")
+        .cwd(g.root())
+        .masquerade_as_nightly_cargo()
+        .with_stderr(
+            "\
+[PACKAGING] foo v0.0.1 ([..])
+[ARCHIVING] Cargo.toml
+[ARCHIVING] src/main.rs
+[ARCHIVING] .cargo_vcs_info.json
+[ARCHIVING] Cargo.lock
+[VERIFYING] foo v0.0.1 ([..])
+[COMPILING] foo v0.0.1 ([..])
+[RUNNING] `rustc --crate-name foo src/main.rs [..]
+[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]
 ",
         )
         .run();
@@ -185,6 +202,7 @@ fn note_resolve_changes() {
 [PACKAGING] foo v0.0.1 ([..])
 [ARCHIVING] Cargo.toml
 [ARCHIVING] src/main.rs
+[ARCHIVING] Cargo.lock
 [UPDATING] `[..]` index
 [NOTE] package `mutli v0.1.0` added to the packaged Cargo.lock file, was originally sourced from `[..]/foo/mutli`
 [NOTE] package `patched v1.0.0` added to the packaged Cargo.lock file, was originally sourced from `[..]/foo/patched`