fix(git-fetcher): ensure the specified commit is used after checkout (#10310)

* fix(git-fetcher): ensure the specified commit is used after checkout

* fix(git-resolver): always resolve to a full commit

* chore: add changeset heavy-dragons-start

* test: fix related test case

* test: fix some other test that gets stuck

* Update heavy-dragons-start.md with PR reference

Add reference to pull request #10310 for clarity.

.changeset/heavy-dragons-start.md

@@ -0,0 +1,7 @@
+---
+"pnpm": patch
+"@pnpm/git-fetcher": patch
+"@pnpm/git-resolver": patch
+---
+
+Always resolve git references to full commits and ensure `HEAD` points to the commit after checkout [#10310](https://github.com/pnpm/pnpm/pull/10310).

fetching/git-fetcher/package.json

@@ -32,6 +32,7 @@
     "compile": "tsc --build && pnpm run lint --fix"
   },
   "dependencies": {
+    "@pnpm/error": "workspace:*",
     "@pnpm/fetcher-base": "workspace:*",
     "@pnpm/fs.packlist": "catalog:",
     "@pnpm/prepare-package": "workspace:*",

fetching/git-fetcher/src/index.ts

@@ -6,6 +6,7 @@ import { packlist } from '@pnpm/fs.packlist'
 import { globalWarn } from '@pnpm/logger'
 import { preparePackage } from '@pnpm/prepare-package'
 import { addFilesFromDir } from '@pnpm/worker'
+import { PnpmError } from '@pnpm/error'
 import rimraf from '@zkochan/rimraf'
 import execa from 'execa'
 import { URL } from 'url'
@@ -31,6 +32,10 @@ export function createGitFetcher (createOpts: CreateGitFetcherOptions): { git: G
       await execGit(['clone', resolution.repo, tempLocation])
     }
     await execGit(['checkout', resolution.commit], { cwd: tempLocation })
+    const receivedCommit = await execGit(['rev-parse', 'HEAD'], { cwd: tempLocation })
+    if (receivedCommit.trim() !== resolution.commit) {
+      throw new PnpmError('GIT_CHECKOUT_FAILED', `received commit ${receivedCommit.trim()} does not match expected value ${resolution.commit}`)
+    }
     let pkgDir: string
     try {
       const prepareResult = await preparePackage({
@@ -85,7 +90,8 @@ function prefixGitArgs (): string[] {
   return process.platform === 'win32' ? ['-c', 'core.longpaths=true'] : []
 }
 
-async function execGit (args: string[], opts?: object): Promise<void> {
+async function execGit (args: string[], opts?: object): Promise<string> {
   const fullArgs = prefixGitArgs().concat(args || [])
-  await execa('git', fullArgs, opts)
+  const { stdout } = await execa('git', fullArgs, opts)
+  return stdout
 }

fetching/git-fetcher/test/index.ts

@@ -205,6 +205,23 @@ test('fail when preparing a git-hosted package', async () => {
   ).rejects.toThrow('Failed to prepare git-hosted package fetched from "https://github.com/pnpm-e2e/prepare-script-fails.git": @pnpm.e2e/prepare-script-fails@1.0.0 npm-install: `npm install`')
 })
 
+test('fail when preparing a git-hosted package with a partial commit', async () => {
+  const storeDir = temporaryDirectory()
+  const fetch = createGitFetcher({
+    rawConfig: {},
+  }).git
+  await expect(
+    fetch(createCafsStore(storeDir),
+      {
+        commit: 'deadbeef',
+        repo: 'https://github.com/pnpm-e2e/simple-pkg.git',
+        type: 'git',
+      }, {
+        filesIndexFile: path.join(storeDir, 'index.json'),
+      })
+  ).rejects.toThrow(/received commit [0-9a-f]{40} does not match expected value deadbeef/)
+})
+
 test('do not build the package when scripts are ignored', async () => {
   const storeDir = tempy.directory()
   const fetch = createGitFetcher({ ignoreScripts: true, rawConfig: {} }).git

fetching/git-fetcher/tsconfig.json

@@ -12,6 +12,9 @@
     {
       "path": "../../exec/prepare-package"
     },
+    {
+      "path": "../../packages/error"
+    },
     {
       "path": "../../packages/logger"
     },

pkg-manager/plugin-commands-installation/test/saveCatalog.ts

@@ -120,7 +120,7 @@ test('saveCatalogName works with different protocols', async () => {
           },
           'is-positive': {
             specifier: 'catalog:',
-            version: 'https://codeload.github.com/kevva/is-positive/tar.gz/97edff6',
+            version: 'https://codeload.github.com/kevva/is-positive/tar.gz/97edff6f525f192a3f83cea1944765f769ae2678',
           },
         },
       },

pnpm-lock.yaml

@@ -3152,6 +3152,9 @@ importers:
 
   fetching/git-fetcher:
     dependencies:
+      '@pnpm/error':
+        specifier: workspace:*
+        version: link:../../packages/error
       '@pnpm/fetcher-base':
         specifier: workspace:*
         version: link:../fetcher-base
@@ -7207,6 +7210,9 @@ importers:
 
   resolving/git-resolver:
     dependencies:
+      '@pnpm/error':
+        specifier: workspace:*
+        version: link:../../packages/error
       '@pnpm/fetch':
         specifier: workspace:*
         version: link:../../network/fetch

resolving/git-resolver/package.json

@@ -34,6 +34,7 @@
     "compile": "tsc --build && pnpm run lint --fix"
   },
   "dependencies": {
+    "@pnpm/error": "workspace:*",
     "@pnpm/fetch": "workspace:*",
     "@pnpm/resolver-base": "workspace:*",
     "graceful-git": "catalog:",

resolving/git-resolver/src/index.ts

@@ -4,6 +4,7 @@ import semver from 'semver'
 import { parseBareSpecifier, type HostedPackageSpec } from './parseBareSpecifier.js'
 import { createGitHostedPkgId } from './createGitHostedPkgId.js'
 import { type AgentOptions } from '@pnpm/network.agent'
+import { PnpmError } from '@pnpm/error'
 
 export { createGitHostedPkgId }
 
@@ -99,16 +100,21 @@ async function getRepoRefs (repo: string, ref: string | null): Promise<Record<st
 }
 
 async function resolveRef (repo: string, ref: string, range?: string): Promise<string> {
-  if (ref.match(/^[0-9a-f]{7,40}$/) != null) {
+  const committish = ref.match(/^[0-9a-f]{7,40}$/) !== null
+  if (committish && ref.length === 40) {
     return ref
   }
-  const refs = await getRepoRefs(repo, range ? null : ref)
-  return resolveRefFromRefs(refs, repo, ref, range)
+  const refs = await getRepoRefs(repo, (range ?? committish) ? null : ref)
+  const result = resolveRefFromRefs(refs, repo, ref, committish, range)
+  if (committish && !result.startsWith(ref)) {
+    throw new PnpmError('GIT_AMBIGUOUS_REF', `resolved commit ${result} from commit-ish reference ${ref}`)
+  }
+  return result
 }
 
-function resolveRefFromRefs (refs: { [ref: string]: string }, repo: string, ref: string, range?: string): string {
+function resolveRefFromRefs (refs: { [ref: string]: string }, repo: string, ref: string, committish: boolean, range?: string): string {
   if (!range) {
-    const commitId =
+    let commitId =
       refs[ref] ||
       refs[`refs/${ref}`] ||
       refs[`refs/tags/${ref}^{}`] || // prefer annotated tags
@@ -116,7 +122,13 @@ function resolveRefFromRefs (refs: { [ref: string]: string }, repo: string, ref:
       refs[`refs/heads/${ref}`]
 
     if (!commitId) {
-      throw new Error(`Could not resolve ${ref} to a commit of ${repo}.`)
+      // check for a partial commit
+      const commits = committish ? Object.values(refs).filter((value: string) => value.startsWith(ref)) : []
+      if (commits.length === 1) {
+        commitId = commits[0]
+      } else {
+        throw new Error(`Could not resolve ${ref} to a commit of ${repo}.`)
+      }
     }
 
     return commitId

resolving/git-resolver/test/index.ts

@@ -60,15 +60,21 @@ test('resolveFromGit() with no commit, when main branch is not master', async ()
 test('resolveFromGit() with partial commit', async () => {
   const resolveResult = await resolveFromGit({ bareSpecifier: 'zoli-forks/cmd-shim#a00a83a' })
   expect(resolveResult).toStrictEqual({
-    id: 'https://codeload.github.com/zoli-forks/cmd-shim/tar.gz/a00a83a',
+    id: 'https://codeload.github.com/zoli-forks/cmd-shim/tar.gz/a00a83a1593edb6e395d3ce41f2ef70edf7e2cf5',
     normalizedBareSpecifier: 'github:zoli-forks/cmd-shim#a00a83a',
     resolution: {
-      tarball: 'https://codeload.github.com/zoli-forks/cmd-shim/tar.gz/a00a83a',
+      tarball: 'https://codeload.github.com/zoli-forks/cmd-shim/tar.gz/a00a83a1593edb6e395d3ce41f2ef70edf7e2cf5',
     },
     resolvedVia: 'git-repository',
   })
 })
 
+test('resolveFromGit() with partial commit that is a branch name', async () => {
+  await expect(
+    resolveFromGit({ bareSpecifier: 'pnpm-e2e/simple-pkg#deadbeef' })
+  ).rejects.toThrow(/resolved commit [0-9a-f]{40} from commit-ish reference deadbeef/)
+})
+
 test('resolveFromGit() with branch', async () => {
   const resolveResult = await resolveFromGit({ bareSpecifier: 'zkochan/is-negative#canary' })
   expect(resolveResult).toStrictEqual({
@@ -421,6 +427,10 @@ test('resolveFromGit() normalizes full url (alternative form 2)', async () => {
 // This test relies on implementation detail.
 // current implementation does not try git ls-remote --refs on bareSpecifier with full commit hash, this fake repo url will pass.
 test('resolveFromGit() private repo with commit hash', async () => {
+  // parseBareSpecifier will try to access the repository with --exit-code
+  git.mockImplementation(() => {
+    throw new Error('private')
+  })
   mockFetchAsPrivate()
   const resolveResult = await resolveFromGit({ bareSpecifier: 'fake/private-repo#2fa0531ab04e300a24ef4fd7fb3a280eccb7ccc5' })
   expect(resolveResult).toStrictEqual({

resolving/git-resolver/tsconfig.json

@@ -12,6 +12,9 @@
     {
       "path": "../../network/fetch"
     },
+    {
+      "path": "../../packages/error"
+    },
     {
       "path": "../resolver-base"
     }