diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index fe2539f..0383c88 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -87,6 +87,17 @@ jobs: - name: Verify fetch filter run: __test__/verify-fetch-filter.sh + # Fetch tags + - name: Checkout with fetch-tags + uses: ./ + with: + ref: test-data/v2/basic + path: fetch-tags-test + fetch-tags: true + - name: Verify fetch-tags + shell: bash + run: __test__/verify-fetch-tags.sh + # Sparse checkout - name: Sparse checkout uses: ./ diff --git a/__test__/git-command-manager.test.ts b/__test__/git-command-manager.test.ts index cea73d4..0b3c16a 100644 --- a/__test__/git-command-manager.test.ts +++ b/__test__/git-command-manager.test.ts @@ -108,7 +108,7 @@ describe('Test fetchDepth and fetchTags options', () => { jest.restoreAllMocks() }) - it('should call execGit with the correct arguments when fetchDepth is 0 and fetchTags is true', async () => { + it('should call execGit with the correct arguments when fetchDepth is 0', async () => { jest.spyOn(exec, 'exec').mockImplementation(mockExec) const workingDirectory = 'test' const lfs = false @@ -122,45 +122,7 @@ describe('Test fetchDepth and fetchTags options', () => { const refSpec = ['refspec1', 'refspec2'] const options = { filter: 'filterValue', - fetchDepth: 0, - fetchTags: true - } - - await git.fetch(refSpec, options) - - expect(mockExec).toHaveBeenCalledWith( - expect.any(String), - [ - '-c', - 'protocol.version=2', - 'fetch', - '--prune', - '--no-recurse-submodules', - '--filter=filterValue', - 'origin', - 'refspec1', - 'refspec2' - ], - expect.any(Object) - ) - }) - - it('should call execGit with the correct arguments when fetchDepth is 0 and fetchTags is false', async () => { - jest.spyOn(exec, 'exec').mockImplementation(mockExec) - - const workingDirectory = 'test' - const lfs = false - const doSparseCheckout = false - git = await commandManager.createCommandManager( - workingDirectory, - lfs, - doSparseCheckout - ) - const refSpec = ['refspec1', 'refspec2'] - const options = { - filter: 'filterValue', - fetchDepth: 0, - fetchTags: false + fetchDepth: 0 } await git.fetch(refSpec, options) @@ -183,7 +145,45 @@ describe('Test fetchDepth and fetchTags options', () => { ) }) - it('should call execGit with the correct arguments when fetchDepth is 1 and fetchTags is false', async () => { + it('should call execGit with the correct arguments when fetchDepth is 0 and refSpec includes tags', async () => { + jest.spyOn(exec, 'exec').mockImplementation(mockExec) + + const workingDirectory = 'test' + const lfs = false + const doSparseCheckout = false + git = await commandManager.createCommandManager( + workingDirectory, + lfs, + doSparseCheckout + ) + const refSpec = ['refspec1', 'refspec2', '+refs/tags/*:refs/tags/*'] + const options = { + filter: 'filterValue', + fetchDepth: 0 + } + + await git.fetch(refSpec, options) + + expect(mockExec).toHaveBeenCalledWith( + expect.any(String), + [ + '-c', + 'protocol.version=2', + 'fetch', + '--no-tags', + '--prune', + '--no-recurse-submodules', + '--filter=filterValue', + 'origin', + 'refspec1', + 'refspec2', + '+refs/tags/*:refs/tags/*' + ], + expect.any(Object) + ) + }) + + it('should call execGit with the correct arguments when fetchDepth is 1', async () => { jest.spyOn(exec, 'exec').mockImplementation(mockExec) const workingDirectory = 'test' @@ -197,8 +197,7 @@ describe('Test fetchDepth and fetchTags options', () => { const refSpec = ['refspec1', 'refspec2'] const options = { filter: 'filterValue', - fetchDepth: 1, - fetchTags: false + fetchDepth: 1 } await git.fetch(refSpec, options) @@ -222,7 +221,7 @@ describe('Test fetchDepth and fetchTags options', () => { ) }) - it('should call execGit with the correct arguments when fetchDepth is 1 and fetchTags is true', async () => { + it('should call execGit with the correct arguments when fetchDepth is 1 and refSpec includes tags', async () => { jest.spyOn(exec, 'exec').mockImplementation(mockExec) const workingDirectory = 'test' @@ -233,11 +232,10 @@ describe('Test fetchDepth and fetchTags options', () => { lfs, doSparseCheckout ) - const refSpec = ['refspec1', 'refspec2'] + const refSpec = ['refspec1', 'refspec2', '+refs/tags/*:refs/tags/*'] const options = { filter: 'filterValue', - fetchDepth: 1, - fetchTags: true + fetchDepth: 1 } await git.fetch(refSpec, options) @@ -248,13 +246,15 @@ describe('Test fetchDepth and fetchTags options', () => { '-c', 'protocol.version=2', 'fetch', + '--no-tags', '--prune', '--no-recurse-submodules', '--filter=filterValue', '--depth=1', 'origin', 'refspec1', - 'refspec2' + 'refspec2', + '+refs/tags/*:refs/tags/*' ], expect.any(Object) ) @@ -338,7 +338,7 @@ describe('Test fetchDepth and fetchTags options', () => { ) }) - it('should call execGit with the correct arguments when fetchTags is true and showProgress is true', async () => { + it('should call execGit with the correct arguments when showProgress is true and refSpec includes tags', async () => { jest.spyOn(exec, 'exec').mockImplementation(mockExec) const workingDirectory = 'test' @@ -349,10 +349,9 @@ describe('Test fetchDepth and fetchTags options', () => { lfs, doSparseCheckout ) - const refSpec = ['refspec1', 'refspec2'] + const refSpec = ['refspec1', 'refspec2', '+refs/tags/*:refs/tags/*'] const options = { filter: 'filterValue', - fetchTags: true, showProgress: true } @@ -364,13 +363,15 @@ describe('Test fetchDepth and fetchTags options', () => { '-c', 'protocol.version=2', 'fetch', + '--no-tags', '--prune', '--no-recurse-submodules', '--progress', '--filter=filterValue', 'origin', 'refspec1', - 'refspec2' + 'refspec2', + '+refs/tags/*:refs/tags/*' ], expect.any(Object) ) diff --git a/__test__/ref-helper.test.ts b/__test__/ref-helper.test.ts index 5c8d76b..4943abd 100644 --- a/__test__/ref-helper.test.ts +++ b/__test__/ref-helper.test.ts @@ -152,7 +152,22 @@ describe('ref-helper tests', () => { it('getRefSpec sha + refs/tags/', async () => { const refSpec = refHelper.getRefSpec('refs/tags/my-tag', commit) expect(refSpec.length).toBe(1) - expect(refSpec[0]).toBe(`+${commit}:refs/tags/my-tag`) + expect(refSpec[0]).toBe(`+refs/tags/my-tag:refs/tags/my-tag`) + }) + + it('getRefSpec sha + refs/tags/ with fetchTags', async () => { + // When fetchTags is true, only include tags wildcard (specific tag is redundant) + const refSpec = refHelper.getRefSpec('refs/tags/my-tag', commit, true) + expect(refSpec.length).toBe(1) + expect(refSpec[0]).toBe('+refs/tags/*:refs/tags/*') + }) + + it('getRefSpec sha + refs/heads/ with fetchTags', async () => { + // When fetchTags is true, include both the branch refspec and tags wildcard + const refSpec = refHelper.getRefSpec('refs/heads/my/branch', commit, true) + expect(refSpec.length).toBe(2) + expect(refSpec[0]).toBe('+refs/tags/*:refs/tags/*') + expect(refSpec[1]).toBe(`+${commit}:refs/remotes/origin/my/branch`) }) it('getRefSpec sha only', async () => { @@ -168,6 +183,14 @@ describe('ref-helper tests', () => { expect(refSpec[1]).toBe('+refs/tags/my-ref*:refs/tags/my-ref*') }) + it('getRefSpec unqualified ref only with fetchTags', async () => { + // When fetchTags is true, skip specific tag pattern since wildcard covers all + const refSpec = refHelper.getRefSpec('my-ref', '', true) + expect(refSpec.length).toBe(2) + expect(refSpec[0]).toBe('+refs/tags/*:refs/tags/*') + expect(refSpec[1]).toBe('+refs/heads/my-ref*:refs/remotes/origin/my-ref*') + }) + it('getRefSpec refs/heads/ only', async () => { const refSpec = refHelper.getRefSpec('refs/heads/my/branch', '') expect(refSpec.length).toBe(1) @@ -187,4 +210,21 @@ describe('ref-helper tests', () => { expect(refSpec.length).toBe(1) expect(refSpec[0]).toBe('+refs/tags/my-tag:refs/tags/my-tag') }) + + it('getRefSpec refs/tags/ only with fetchTags', async () => { + // When fetchTags is true, only include tags wildcard (specific tag is redundant) + const refSpec = refHelper.getRefSpec('refs/tags/my-tag', '', true) + expect(refSpec.length).toBe(1) + expect(refSpec[0]).toBe('+refs/tags/*:refs/tags/*') + }) + + it('getRefSpec refs/heads/ only with fetchTags', async () => { + // When fetchTags is true, include both the branch refspec and tags wildcard + const refSpec = refHelper.getRefSpec('refs/heads/my/branch', '', true) + expect(refSpec.length).toBe(2) + expect(refSpec[0]).toBe('+refs/tags/*:refs/tags/*') + expect(refSpec[1]).toBe( + '+refs/heads/my/branch:refs/remotes/origin/my/branch' + ) + }) }) diff --git a/__test__/verify-fetch-tags.sh b/__test__/verify-fetch-tags.sh new file mode 100755 index 0000000..74cff1e --- /dev/null +++ b/__test__/verify-fetch-tags.sh @@ -0,0 +1,9 @@ +#!/bin/sh + +# Verify tags were fetched +TAG_COUNT=$(git -C ./fetch-tags-test tag | wc -l) +if [ "$TAG_COUNT" -eq 0 ]; then + echo "Expected tags to be fetched, but found none" + exit 1 +fi +echo "Found $TAG_COUNT tags" diff --git a/dist/index.js b/dist/index.js index b9b34d3..6a0c74c 100644 --- a/dist/index.js +++ b/dist/index.js @@ -653,7 +653,6 @@ const fs = __importStar(__nccwpck_require__(7147)); const fshelper = __importStar(__nccwpck_require__(7219)); const io = __importStar(__nccwpck_require__(7436)); const path = __importStar(__nccwpck_require__(1017)); -const refHelper = __importStar(__nccwpck_require__(8601)); const regexpHelper = __importStar(__nccwpck_require__(3120)); const retryHelper = __importStar(__nccwpck_require__(2155)); const git_version_1 = __nccwpck_require__(3142); @@ -831,9 +830,9 @@ class GitCommandManager { fetch(refSpec, options) { return __awaiter(this, void 0, void 0, function* () { const args = ['-c', 'protocol.version=2', 'fetch']; - if (!refSpec.some(x => x === refHelper.tagsRefSpec) && !options.fetchTags) { - args.push('--no-tags'); - } + // Always use --no-tags for explicit control over tag fetching + // Tags are fetched explicitly via refspec when needed + args.push('--no-tags'); args.push('--prune', '--no-recurse-submodules'); if (options.showProgress) { args.push('--progress'); @@ -1529,13 +1528,26 @@ function getSource(settings) { if (!(yield refHelper.testRef(git, settings.ref, settings.commit))) { refSpec = refHelper.getRefSpec(settings.ref, settings.commit); yield git.fetch(refSpec, fetchOptions); + // Verify the ref now matches. For branches, the targeted fetch above brings + // in the specific commit. For tags (fetched by ref), this will fail if + // the tag was moved after the workflow was triggered. + if (!(yield refHelper.testRef(git, settings.ref, settings.commit))) { + throw new Error(`The ref '${settings.ref}' does not point to the expected commit '${settings.commit}'. ` + + `The ref may have been updated after the workflow was triggered.`); + } } } else { fetchOptions.fetchDepth = settings.fetchDepth; - fetchOptions.fetchTags = settings.fetchTags; - const refSpec = refHelper.getRefSpec(settings.ref, settings.commit); + const refSpec = refHelper.getRefSpec(settings.ref, settings.commit, settings.fetchTags); yield git.fetch(refSpec, fetchOptions); + // For tags, verify the ref still points to the expected commit. + // Tags are fetched by ref (not commit), so if a tag was moved after the + // workflow was triggered, we would silently check out the wrong commit. + if (!(yield refHelper.testRef(git, settings.ref, settings.commit))) { + throw new Error(`The ref '${settings.ref}' does not point to the expected commit '${settings.commit}'. ` + + `The ref may have been updated after the workflow was triggered.`); + } } core.endGroup(); // Checkout info @@ -2274,53 +2286,67 @@ function getRefSpecForAllHistory(ref, commit) { } return result; } -function getRefSpec(ref, commit) { +function getRefSpec(ref, commit, fetchTags) { if (!ref && !commit) { throw new Error('Args ref and commit cannot both be empty'); } const upperRef = (ref || '').toUpperCase(); + const result = []; + // When fetchTags is true, always include the tags refspec + if (fetchTags) { + result.push(exports.tagsRefSpec); + } // SHA if (commit) { // refs/heads if (upperRef.startsWith('REFS/HEADS/')) { const branch = ref.substring('refs/heads/'.length); - return [`+${commit}:refs/remotes/origin/${branch}`]; + result.push(`+${commit}:refs/remotes/origin/${branch}`); } // refs/pull/ else if (upperRef.startsWith('REFS/PULL/')) { const branch = ref.substring('refs/pull/'.length); - return [`+${commit}:refs/remotes/pull/${branch}`]; + result.push(`+${commit}:refs/remotes/pull/${branch}`); } // refs/tags/ else if (upperRef.startsWith('REFS/TAGS/')) { - return [`+${commit}:${ref}`]; + if (!fetchTags) { + result.push(`+${ref}:${ref}`); + } } // Otherwise no destination ref else { - return [commit]; + result.push(commit); } } // Unqualified ref, check for a matching branch or tag else if (!upperRef.startsWith('REFS/')) { - return [ - `+refs/heads/${ref}*:refs/remotes/origin/${ref}*`, - `+refs/tags/${ref}*:refs/tags/${ref}*` - ]; + result.push(`+refs/heads/${ref}*:refs/remotes/origin/${ref}*`); + if (!fetchTags) { + result.push(`+refs/tags/${ref}*:refs/tags/${ref}*`); + } } // refs/heads/ else if (upperRef.startsWith('REFS/HEADS/')) { const branch = ref.substring('refs/heads/'.length); - return [`+${ref}:refs/remotes/origin/${branch}`]; + result.push(`+${ref}:refs/remotes/origin/${branch}`); } // refs/pull/ else if (upperRef.startsWith('REFS/PULL/')) { const branch = ref.substring('refs/pull/'.length); - return [`+${ref}:refs/remotes/pull/${branch}`]; + result.push(`+${ref}:refs/remotes/pull/${branch}`); } // refs/tags/ - else { - return [`+${ref}:${ref}`]; + else if (upperRef.startsWith('REFS/TAGS/')) { + if (!fetchTags) { + result.push(`+${ref}:${ref}`); + } } + // Other refs + else { + result.push(`+${ref}:${ref}`); + } + return result; } /** * Tests whether the initial fetch created the ref at the expected commit @@ -2356,7 +2382,9 @@ function testRef(git, ref, commit) { // refs/tags/ else if (upperRef.startsWith('REFS/TAGS/')) { const tagName = ref.substring('refs/tags/'.length); - return ((yield git.tagExists(tagName)) && commit === (yield git.revParse(ref))); + // Use ^{commit} to dereference annotated tags to their underlying commit + return ((yield git.tagExists(tagName)) && + commit === (yield git.revParse(`${ref}^{commit}`))); } // Unexpected else { diff --git a/src/git-command-manager.ts b/src/git-command-manager.ts index a45e15a..a4c2e8f 100644 --- a/src/git-command-manager.ts +++ b/src/git-command-manager.ts @@ -37,7 +37,6 @@ export interface IGitCommandManager { options: { filter?: string fetchDepth?: number - fetchTags?: boolean showProgress?: boolean } ): Promise @@ -280,14 +279,13 @@ class GitCommandManager { options: { filter?: string fetchDepth?: number - fetchTags?: boolean showProgress?: boolean } ): Promise { const args = ['-c', 'protocol.version=2', 'fetch'] - if (!refSpec.some(x => x === refHelper.tagsRefSpec) && !options.fetchTags) { - args.push('--no-tags') - } + // Always use --no-tags for explicit control over tag fetching + // Tags are fetched explicitly via refspec when needed + args.push('--no-tags') args.push('--prune', '--no-recurse-submodules') if (options.showProgress) { diff --git a/src/git-source-provider.ts b/src/git-source-provider.ts index 2d35138..ec87178 100644 --- a/src/git-source-provider.ts +++ b/src/git-source-provider.ts @@ -159,7 +159,6 @@ export async function getSource(settings: IGitSourceSettings): Promise { const fetchOptions: { filter?: string fetchDepth?: number - fetchTags?: boolean showProgress?: boolean } = {} @@ -182,12 +181,35 @@ export async function getSource(settings: IGitSourceSettings): Promise { if (!(await refHelper.testRef(git, settings.ref, settings.commit))) { refSpec = refHelper.getRefSpec(settings.ref, settings.commit) await git.fetch(refSpec, fetchOptions) + + // Verify the ref now matches. For branches, the targeted fetch above brings + // in the specific commit. For tags (fetched by ref), this will fail if + // the tag was moved after the workflow was triggered. + if (!(await refHelper.testRef(git, settings.ref, settings.commit))) { + throw new Error( + `The ref '${settings.ref}' does not point to the expected commit '${settings.commit}'. ` + + `The ref may have been updated after the workflow was triggered.` + ) + } } } else { fetchOptions.fetchDepth = settings.fetchDepth - fetchOptions.fetchTags = settings.fetchTags - const refSpec = refHelper.getRefSpec(settings.ref, settings.commit) + const refSpec = refHelper.getRefSpec( + settings.ref, + settings.commit, + settings.fetchTags + ) await git.fetch(refSpec, fetchOptions) + + // For tags, verify the ref still points to the expected commit. + // Tags are fetched by ref (not commit), so if a tag was moved after the + // workflow was triggered, we would silently check out the wrong commit. + if (!(await refHelper.testRef(git, settings.ref, settings.commit))) { + throw new Error( + `The ref '${settings.ref}' does not point to the expected commit '${settings.commit}'. ` + + `The ref may have been updated after the workflow was triggered.` + ) + } } core.endGroup() diff --git a/src/ref-helper.ts b/src/ref-helper.ts index 58f9290..5130f53 100644 --- a/src/ref-helper.ts +++ b/src/ref-helper.ts @@ -76,55 +76,75 @@ export function getRefSpecForAllHistory(ref: string, commit: string): string[] { return result } -export function getRefSpec(ref: string, commit: string): string[] { +export function getRefSpec( + ref: string, + commit: string, + fetchTags?: boolean +): string[] { if (!ref && !commit) { throw new Error('Args ref and commit cannot both be empty') } const upperRef = (ref || '').toUpperCase() + const result: string[] = [] + + // When fetchTags is true, always include the tags refspec + if (fetchTags) { + result.push(tagsRefSpec) + } // SHA if (commit) { // refs/heads if (upperRef.startsWith('REFS/HEADS/')) { const branch = ref.substring('refs/heads/'.length) - return [`+${commit}:refs/remotes/origin/${branch}`] + result.push(`+${commit}:refs/remotes/origin/${branch}`) } // refs/pull/ else if (upperRef.startsWith('REFS/PULL/')) { const branch = ref.substring('refs/pull/'.length) - return [`+${commit}:refs/remotes/pull/${branch}`] + result.push(`+${commit}:refs/remotes/pull/${branch}`) } // refs/tags/ else if (upperRef.startsWith('REFS/TAGS/')) { - return [`+${commit}:${ref}`] + if (!fetchTags) { + result.push(`+${ref}:${ref}`) + } } // Otherwise no destination ref else { - return [commit] + result.push(commit) } } // Unqualified ref, check for a matching branch or tag else if (!upperRef.startsWith('REFS/')) { - return [ - `+refs/heads/${ref}*:refs/remotes/origin/${ref}*`, - `+refs/tags/${ref}*:refs/tags/${ref}*` - ] + result.push(`+refs/heads/${ref}*:refs/remotes/origin/${ref}*`) + if (!fetchTags) { + result.push(`+refs/tags/${ref}*:refs/tags/${ref}*`) + } } // refs/heads/ else if (upperRef.startsWith('REFS/HEADS/')) { const branch = ref.substring('refs/heads/'.length) - return [`+${ref}:refs/remotes/origin/${branch}`] + result.push(`+${ref}:refs/remotes/origin/${branch}`) } // refs/pull/ else if (upperRef.startsWith('REFS/PULL/')) { const branch = ref.substring('refs/pull/'.length) - return [`+${ref}:refs/remotes/pull/${branch}`] + result.push(`+${ref}:refs/remotes/pull/${branch}`) } // refs/tags/ - else { - return [`+${ref}:${ref}`] + else if (upperRef.startsWith('REFS/TAGS/')) { + if (!fetchTags) { + result.push(`+${ref}:${ref}`) + } } + // Other refs + else { + result.push(`+${ref}:${ref}`) + } + + return result } /** @@ -170,8 +190,10 @@ export async function testRef( // refs/tags/ else if (upperRef.startsWith('REFS/TAGS/')) { const tagName = ref.substring('refs/tags/'.length) + // Use ^{commit} to dereference annotated tags to their underlying commit return ( - (await git.tagExists(tagName)) && commit === (await git.revParse(ref)) + (await git.tagExists(tagName)) && + commit === (await git.revParse(`${ref}^{commit}`)) ) } // Unexpected