fix regression and tests

Author: chris-olszewski

Cargo.lock

@@ -17,6 +17,7 @@ biome_diagnostics = { workspace = true }
 biome_json_parser = { workspace = true }
 biome_json_syntax = { workspace = true }
 
+either = { workspace = true }
 globwalk = { version = "0.1.0", path = "../turborepo-globwalk" }
 itertools = { workspace = true }
 lazy-regex = "2.5.0"

crates/turborepo-repository/Cargo.toml

@@ -24,6 +24,19 @@ pub trait PackageChangeMapper {
     fn detect_package(&self, file: &AnchoredSystemPath) -> PackageMapping;
 }
 
+impl<L, R> PackageChangeMapper for either::Either<L, R>
+where
+    L: PackageChangeMapper,
+    R: PackageChangeMapper,
+{
+    fn detect_package(&self, file: &AnchoredSystemPath) -> PackageMapping {
+        match self {
+            either::Either::Left(l) => l.detect_package(file),
+            either::Either::Right(r) => r.detect_package(file),
+        }
+    }
+}
+
 /// Detects package by checking if the file is inside the package.
 ///
 /// Does *not* use the `globalDependencies` in turbo.json.

crates/turborepo-repository/src/change_mapper/package.rs

@@ -1,4 +1,4 @@
-import { describe, it } from "node:test";
+import { beforeEach, describe, it } from "node:test";
 import { strict as assert } from "node:assert";
 import * as path from "node:path";
 import { Workspace, Package, PackageManager } from "../js/dist/index.js";
@@ -8,7 +8,6 @@ type PackageReduced = Pick<Package, "name" | "relativePath">;
 interface AffectedPackagesTestParams {
   description: string;
   files: string[];
-  changedLockfile?: string | undefined | null;
   expected: PackageReduced[];
 }
 
@@ -33,49 +32,22 @@ describe("affectedPackages", () => {
       ],
     },
     {
-      description:
-        "a lockfile change will only affect packages impacted by the change",
-      files: [],
-      changedLockfile: `lockfileVersion: '6.0'
-
-settings:
-  autoInstallPeers: true
-  excludeLinksFromLockfile: false
-
-importers:
-
-  .: {}
-
-  apps/app:
-    dependencies:
-      microdiff:
-        specifier: ^1.4.0
-        version: 1.5.0
-      ui:
-        specifier: workspace:*
-        version: link:../../packages/ui
-
-  packages/blank: {}
-
-  packages/ui: {}
-
-packages:
-
-  /[email protected]:
-    resolution: {integrity: sha512-Drq+/THMvDdzRYrK0oxJmOKiC24ayUV8ahrt8l3oRK51PWt6gdtrIGrlIH3pT/lFh1z93FbAcidtsHcWbnRz8Q==}
-    dev: false
-`,
-      expected: [{ name: "app-a", relativePath: "apps/app" }],
+      description: "a lockfile change will affect all packages",
+      files: ["pnpm-lock.yaml"],
+      expected: [
+        { name: "app-a", relativePath: "apps/app" },
+        { name: "ui", relativePath: "packages/ui" },
+      ],
     },
   ];
 
-  for (const { description, files, expected, changedLockfile } of tests) {
+  for (const { description, files, expected } of tests) {
     it(description, async () => {
       const dir = path.resolve(__dirname, "./fixtures/monorepo");
       const workspace = await Workspace.find(dir);
 
       const reduced: PackageReduced[] = (
-        await workspace.affectedPackages(files, changedLockfile)
+        await workspace.affectedPackages(files)
       ).map((pkg) => {
         return {
           name: pkg.name,
@@ -86,4 +58,15 @@ packages:
       assert.deepEqual(reduced, expected);
     });
   }
+
+  describe("optimizedLockfileUpdates", () => {
+    it("errors if not provided comparison ref", async () => {
+      const dir = path.resolve(__dirname, "./fixtures/monorepo");
+      const workspace = await Workspace.find(dir);
+
+      assert.rejects(
+        workspace.affectedPackages(["pnpm-lock.yaml"], null, true)
+      );
+    });
+  });
 });

packages/turbo-repository/__tests__/affected-packages.test.ts

@@ -11,6 +11,7 @@ crate-type = ["cdylib"]
 workspace = true
 
 [dependencies]
+either = { workspace = true }
 napi = { version = "2.14.0", features = ["tokio_rt"] }
 napi-derive = "2.14.0"
 thiserror = { workspace = true }

packages/turbo-repository/rust/Cargo.toml

@@ -3,13 +3,15 @@ use std::{
     hash::Hash,
 };
 
+use either::Either;
 use napi::Error;
 use napi_derive::napi;
 use tracing::debug;
 use turbopath::{AbsoluteSystemPath, AnchoredSystemPath, AnchoredSystemPathBuf};
 use turborepo_repository::{
     change_mapper::{
-        ChangeMapper, GlobalDepsPackageChangeMapper, LockfileContents, PackageChanges,
+        ChangeMapper, DefaultPackageChangeMapper, GlobalDepsPackageChangeMapper, LockfileContents,
+        PackageChanges,
     },
     inference::RepoState as WorkspaceState,
     package_graph::{PackageGraph, PackageName, PackageNode, WorkspacePackage, ROOT_PKG_NAME},
@@ -190,15 +192,15 @@ impl Workspace {
         &self,
         changed_files: &HashSet<AnchoredSystemPathBuf>,
         workspace_root: &AbsoluteSystemPath,
-        from_commit: Option<&str>,
+        from_commit: &str,
     ) -> LockfileContents {
         let lockfile_name = self.graph.package_manager().lockfile_name();
         changed_files
             .contains(AnchoredSystemPath::new(&lockfile_name).unwrap())
             .then(|| {
                 let git = SCM::new(workspace_root);
                 let anchored_path = workspace_root.join_component(lockfile_name);
-                git.previous_content(from_commit, &anchored_path)
+                git.previous_content(Some(from_commit), &anchored_path)
                     .map(LockfileContents::Changed)
                     .inspect_err(|e| debug!("{e}"))
                     .ok()
@@ -218,6 +220,16 @@ impl Workspace {
         comparison: Option<&str>, // this is required when optimize_global_invalidations is true
         optimize_global_invalidations: Option<bool>,
     ) -> Result<Vec<Package>, Error> {
+        let comparison = optimize_global_invalidations
+            .unwrap_or(false)
+            .then(|| {
+                comparison.ok_or_else(|| {
+                    Error::from_reason(
+                        "optimizeGlobalInvalidations true, but no comparison commit given",
+                    )
+                })
+            })
+            .transpose()?;
         let workspace_root = match AbsoluteSystemPath::new(&self.absolute_path) {
             Ok(path) => path,
             Err(e) => return Err(Error::from_reason(e.to_string())),
@@ -232,12 +244,24 @@ impl Workspace {
             .collect();
 
         // Create a ChangeMapper with no ignore patterns
-        let global_deps_package_detector =
-            GlobalDepsPackageChangeMapper::new(&self.graph, std::iter::empty::<&str>()).unwrap();
-        let mapper = ChangeMapper::new(&self.graph, vec![], global_deps_package_detector);
+        let change_detector = comparison
+            .is_some()
+            .then(|| {
+                Either::Left(
+                    GlobalDepsPackageChangeMapper::new(&self.graph, std::iter::empty::<&str>())
+                        .unwrap(),
+                )
+            })
+            .unwrap_or_else(|| Either::Right(DefaultPackageChangeMapper::new(&self.graph)));
+        let mapper = ChangeMapper::new(&self.graph, vec![], change_detector);
 
-        let lockfile_contents = if let Some(true) = optimize_global_invalidations {
-            self.get_lockfile_contents(&changed_files, &workspace_root, comparison)
+        let lockfile_contents = if let Some(comparison) = comparison {
+            self.get_lockfile_contents(&changed_files, workspace_root, comparison)
+        } else if changed_files.contains(
+            AnchoredSystemPath::new(self.graph.package_manager().lockfile_name())
+                .expect("the lockfile name will not be an absolute path"),
+        ) {
+            LockfileContents::UnknownChange
         } else {
             LockfileContents::Unknown
         };