Skip to content

Commit de0fac2

Browse files
ericscipleericsciple
authored
Fix tag handling: preserve annotations and explicit fetch-tags (#2356)
This PR fixes several issues with tag handling in the checkout action: 1. fetch-tags: true now works (fixes #1471) - Tags refspec is now included in getRefSpec() when fetchTags=true - Previously tags were only fetched during a separate fetch that was overwritten by the main fetch 2. Tag checkout preserves annotations (fixes #290) - Tags are fetched via refspec (+refs/tags/*:refs/tags/*) instead of --tags flag - This fetches the actual tag objects, preserving annotations 3. Tag checkout with fetch-tags: true no longer fails (fixes #1467) - When checking out a tag with fetchTags=true, only the wildcard refspec is used (specific tag refspec is redundant) Changes: - src/ref-helper.ts: getRefSpec() now accepts fetchTags parameter and prepends tags refspec when true - src/git-command-manager.ts: fetch() simplified to always use --no-tags, tags are fetched explicitly via refspec - src/git-source-provider.ts: passes fetchTags to getRefSpec() - Added E2E test for fetch-tags option Related #1471, #1467, #290
1 parent 064fe7f commit de0fac2

File tree

8 files changed

+192
-61
lines changed

8 files changed

+192
-61
lines changed

‎.github/workflows/test.yml‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,17 @@ jobs:
8787
- name: Verify fetch filter
8888
run: __test__/verify-fetch-filter.sh
8989

90+
# Fetch tags
91+
- name: Checkout with fetch-tags
92+
uses: ./
93+
with:
94+
ref: test-data/v2/basic
95+
path: fetch-tags-test
96+
fetch-tags: true
97+
- name: Verify fetch-tags
98+
shell: bash
99+
run: __test__/verify-fetch-tags.sh
100+
90101
# Sparse checkout
91102
- name: Sparse checkout
92103
uses: ./

‎__test__/git-command-manager.test.ts‎

Lines changed: 21 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,7 @@ describe('Test fetchDepth and fetchTags options', () => {
108108
jest.restoreAllMocks()
109109
})
110110

111-
it('should call execGit with the correct arguments when fetchDepth is 0 and fetchTags is true', async () => {
111+
it('should call execGit with the correct arguments when fetchDepth is 0', async () => {
112112
jest.spyOn(exec, 'exec').mockImplementation(mockExec)
113113
const workingDirectory = 'test'
114114
const lfs = false
@@ -122,8 +122,7 @@ describe('Test fetchDepth and fetchTags options', () => {
122122
const refSpec = ['refspec1', 'refspec2']
123123
const options = {
124124
filter: 'filterValue',
125-
fetchDepth: 0,
126-
fetchTags: true
125+
fetchDepth: 0
127126
}
128127

129128
await git.fetch(refSpec, options)
@@ -134,6 +133,7 @@ describe('Test fetchDepth and fetchTags options', () => {
134133
'-c',
135134
'protocol.version=2',
136135
'fetch',
136+
'--no-tags',
137137
'--prune',
138138
'--no-recurse-submodules',
139139
'--filter=filterValue',
@@ -145,7 +145,7 @@ describe('Test fetchDepth and fetchTags options', () => {
145145
)
146146
})
147147

148-
it('should call execGit with the correct arguments when fetchDepth is 0 and fetchTags is false', async () => {
148+
it('should call execGit with the correct arguments when fetchDepth is 0 and refSpec includes tags', async () => {
149149
jest.spyOn(exec, 'exec').mockImplementation(mockExec)
150150

151151
const workingDirectory = 'test'
@@ -156,11 +156,10 @@ describe('Test fetchDepth and fetchTags options', () => {
156156
lfs,
157157
doSparseCheckout
158158
)
159-
const refSpec = ['refspec1', 'refspec2']
159+
const refSpec = ['refspec1', 'refspec2', '+refs/tags/*:refs/tags/*']
160160
const options = {
161161
filter: 'filterValue',
162-
fetchDepth: 0,
163-
fetchTags: false
162+
fetchDepth: 0
164163
}
165164

166165
await git.fetch(refSpec, options)
@@ -177,13 +176,14 @@ describe('Test fetchDepth and fetchTags options', () => {
177176
'--filter=filterValue',
178177
'origin',
179178
'refspec1',
180-
'refspec2'
179+
'refspec2',
180+
'+refs/tags/*:refs/tags/*'
181181
],
182182
expect.any(Object)
183183
)
184184
})
185185

186-
it('should call execGit with the correct arguments when fetchDepth is 1 and fetchTags is false', async () => {
186+
it('should call execGit with the correct arguments when fetchDepth is 1', async () => {
187187
jest.spyOn(exec, 'exec').mockImplementation(mockExec)
188188

189189
const workingDirectory = 'test'
@@ -197,8 +197,7 @@ describe('Test fetchDepth and fetchTags options', () => {
197197
const refSpec = ['refspec1', 'refspec2']
198198
const options = {
199199
filter: 'filterValue',
200-
fetchDepth: 1,
201-
fetchTags: false
200+
fetchDepth: 1
202201
}
203202

204203
await git.fetch(refSpec, options)
@@ -222,7 +221,7 @@ describe('Test fetchDepth and fetchTags options', () => {
222221
)
223222
})
224223

225-
it('should call execGit with the correct arguments when fetchDepth is 1 and fetchTags is true', async () => {
224+
it('should call execGit with the correct arguments when fetchDepth is 1 and refSpec includes tags', async () => {
226225
jest.spyOn(exec, 'exec').mockImplementation(mockExec)
227226

228227
const workingDirectory = 'test'
@@ -233,11 +232,10 @@ describe('Test fetchDepth and fetchTags options', () => {
233232
lfs,
234233
doSparseCheckout
235234
)
236-
const refSpec = ['refspec1', 'refspec2']
235+
const refSpec = ['refspec1', 'refspec2', '+refs/tags/*:refs/tags/*']
237236
const options = {
238237
filter: 'filterValue',
239-
fetchDepth: 1,
240-
fetchTags: true
238+
fetchDepth: 1
241239
}
242240

243241
await git.fetch(refSpec, options)
@@ -248,13 +246,15 @@ describe('Test fetchDepth and fetchTags options', () => {
248246
'-c',
249247
'protocol.version=2',
250248
'fetch',
249+
'--no-tags',
251250
'--prune',
252251
'--no-recurse-submodules',
253252
'--filter=filterValue',
254253
'--depth=1',
255254
'origin',
256255
'refspec1',
257-
'refspec2'
256+
'refspec2',
257+
'+refs/tags/*:refs/tags/*'
258258
],
259259
expect.any(Object)
260260
)
@@ -338,7 +338,7 @@ describe('Test fetchDepth and fetchTags options', () => {
338338
)
339339
})
340340

341-
it('should call execGit with the correct arguments when fetchTags is true and showProgress is true', async () => {
341+
it('should call execGit with the correct arguments when showProgress is true and refSpec includes tags', async () => {
342342
jest.spyOn(exec, 'exec').mockImplementation(mockExec)
343343

344344
const workingDirectory = 'test'
@@ -349,10 +349,9 @@ describe('Test fetchDepth and fetchTags options', () => {
349349
lfs,
350350
doSparseCheckout
351351
)
352-
const refSpec = ['refspec1', 'refspec2']
352+
const refSpec = ['refspec1', 'refspec2', '+refs/tags/*:refs/tags/*']
353353
const options = {
354354
filter: 'filterValue',
355-
fetchTags: true,
356355
showProgress: true
357356
}
358357

@@ -364,13 +363,15 @@ describe('Test fetchDepth and fetchTags options', () => {
364363
'-c',
365364
'protocol.version=2',
366365
'fetch',
366+
'--no-tags',
367367
'--prune',
368368
'--no-recurse-submodules',
369369
'--progress',
370370
'--filter=filterValue',
371371
'origin',
372372
'refspec1',
373-
'refspec2'
373+
'refspec2',
374+
'+refs/tags/*:refs/tags/*'
374375
],
375376
expect.any(Object)
376377
)

‎__test__/ref-helper.test.ts‎

Lines changed: 41 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,22 @@ describe('ref-helper tests', () => {
152152
it('getRefSpec sha + refs/tags/', async () => {
153153
const refSpec = refHelper.getRefSpec('refs/tags/my-tag', commit)
154154
expect(refSpec.length).toBe(1)
155-
expect(refSpec[0]).toBe(`+${commit}:refs/tags/my-tag`)
155+
expect(refSpec[0]).toBe(`+refs/tags/my-tag:refs/tags/my-tag`)
156+
})
157+
158+
it('getRefSpec sha + refs/tags/ with fetchTags', async () => {
159+
// When fetchTags is true, only include tags wildcard (specific tag is redundant)
160+
const refSpec = refHelper.getRefSpec('refs/tags/my-tag', commit, true)
161+
expect(refSpec.length).toBe(1)
162+
expect(refSpec[0]).toBe('+refs/tags/*:refs/tags/*')
163+
})
164+
165+
it('getRefSpec sha + refs/heads/ with fetchTags', async () => {
166+
// When fetchTags is true, include both the branch refspec and tags wildcard
167+
const refSpec = refHelper.getRefSpec('refs/heads/my/branch', commit, true)
168+
expect(refSpec.length).toBe(2)
169+
expect(refSpec[0]).toBe('+refs/tags/*:refs/tags/*')
170+
expect(refSpec[1]).toBe(`+${commit}:refs/remotes/origin/my/branch`)
156171
})
157172

158173
it('getRefSpec sha only', async () => {
@@ -168,6 +183,14 @@ describe('ref-helper tests', () => {
168183
expect(refSpec[1]).toBe('+refs/tags/my-ref*:refs/tags/my-ref*')
169184
})
170185

186+
it('getRefSpec unqualified ref only with fetchTags', async () => {
187+
// When fetchTags is true, skip specific tag pattern since wildcard covers all
188+
const refSpec = refHelper.getRefSpec('my-ref', '', true)
189+
expect(refSpec.length).toBe(2)
190+
expect(refSpec[0]).toBe('+refs/tags/*:refs/tags/*')
191+
expect(refSpec[1]).toBe('+refs/heads/my-ref*:refs/remotes/origin/my-ref*')
192+
})
193+
171194
it('getRefSpec refs/heads/ only', async () => {
172195
const refSpec = refHelper.getRefSpec('refs/heads/my/branch', '')
173196
expect(refSpec.length).toBe(1)
@@ -187,4 +210,21 @@ describe('ref-helper tests', () => {
187210
expect(refSpec.length).toBe(1)
188211
expect(refSpec[0]).toBe('+refs/tags/my-tag:refs/tags/my-tag')
189212
})
213+
214+
it('getRefSpec refs/tags/ only with fetchTags', async () => {
215+
// When fetchTags is true, only include tags wildcard (specific tag is redundant)
216+
const refSpec = refHelper.getRefSpec('refs/tags/my-tag', '', true)
217+
expect(refSpec.length).toBe(1)
218+
expect(refSpec[0]).toBe('+refs/tags/*:refs/tags/*')
219+
})
220+
221+
it('getRefSpec refs/heads/ only with fetchTags', async () => {
222+
// When fetchTags is true, include both the branch refspec and tags wildcard
223+
const refSpec = refHelper.getRefSpec('refs/heads/my/branch', '', true)
224+
expect(refSpec.length).toBe(2)
225+
expect(refSpec[0]).toBe('+refs/tags/*:refs/tags/*')
226+
expect(refSpec[1]).toBe(
227+
'+refs/heads/my/branch:refs/remotes/origin/my/branch'
228+
)
229+
})
190230
})

‎__test__/verify-fetch-tags.sh‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
#!/bin/sh
2+
3+
# Verify tags were fetched
4+
TAG_COUNT=$(git -C ./fetch-tags-test tag | wc -l)
5+
if [ "$TAG_COUNT" -eq 0 ]; then
6+
echo "Expected tags to be fetched, but found none"
7+
exit 1
8+
fi
9+
echo "Found $TAG_COUNT tags"

‎dist/index.js‎

Lines changed: 47 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -653,7 +653,6 @@ const fs = __importStar(__nccwpck_require__(7147));
653653
const fshelper = __importStar(__nccwpck_require__(7219));
654654
const io = __importStar(__nccwpck_require__(7436));
655655
const path = __importStar(__nccwpck_require__(1017));
656-
const refHelper = __importStar(__nccwpck_require__(8601));
657656
const regexpHelper = __importStar(__nccwpck_require__(3120));
658657
const retryHelper = __importStar(__nccwpck_require__(2155));
659658
const git_version_1 = __nccwpck_require__(3142);
@@ -831,9 +830,9 @@ class GitCommandManager {
831830
fetch(refSpec, options) {
832831
return __awaiter(this, void 0, void 0, function* () {
833832
const args = ['-c', 'protocol.version=2', 'fetch'];
834-
if (!refSpec.some(x => x === refHelper.tagsRefSpec) && !options.fetchTags) {
835-
args.push('--no-tags');
836-
}
833+
// Always use --no-tags for explicit control over tag fetching
834+
// Tags are fetched explicitly via refspec when needed
835+
args.push('--no-tags');
837836
args.push('--prune', '--no-recurse-submodules');
838837
if (options.showProgress) {
839838
args.push('--progress');
@@ -1539,13 +1538,26 @@ function getSource(settings) {
15391538
if (!(yield refHelper.testRef(git, settings.ref, settings.commit))) {
15401539
refSpec = refHelper.getRefSpec(settings.ref, settings.commit);
15411540
yield git.fetch(refSpec, fetchOptions);
1541+
// Verify the ref now matches. For branches, the targeted fetch above brings
1542+
// in the specific commit. For tags (fetched by ref), this will fail if
1543+
// the tag was moved after the workflow was triggered.
1544+
if (!(yield refHelper.testRef(git, settings.ref, settings.commit))) {
1545+
throw new Error(`The ref '${settings.ref}' does not point to the expected commit '${settings.commit}'. ` +
1546+
`The ref may have been updated after the workflow was triggered.`);
1547+
}
15421548
}
15431549
}
15441550
else {
15451551
fetchOptions.fetchDepth = settings.fetchDepth;
1546-
fetchOptions.fetchTags = settings.fetchTags;
1547-
const refSpec = refHelper.getRefSpec(settings.ref, settings.commit);
1552+
const refSpec = refHelper.getRefSpec(settings.ref, settings.commit, settings.fetchTags);
15481553
yield git.fetch(refSpec, fetchOptions);
1554+
// For tags, verify the ref still points to the expected commit.
1555+
// Tags are fetched by ref (not commit), so if a tag was moved after the
1556+
// workflow was triggered, we would silently check out the wrong commit.
1557+
if (!(yield refHelper.testRef(git, settings.ref, settings.commit))) {
1558+
throw new Error(`The ref '${settings.ref}' does not point to the expected commit '${settings.commit}'. ` +
1559+
`The ref may have been updated after the workflow was triggered.`);
1560+
}
15491561
}
15501562
core.endGroup();
15511563
// Checkout info
@@ -2284,53 +2296,67 @@ function getRefSpecForAllHistory(ref, commit) {
22842296
}
22852297
return result;
22862298
}
2287-
function getRefSpec(ref, commit) {
2299+
function getRefSpec(ref, commit, fetchTags) {
22882300
if (!ref && !commit) {
22892301
throw new Error('Args ref and commit cannot both be empty');
22902302
}
22912303
const upperRef = (ref || '').toUpperCase();
2304+
const result = [];
2305+
// When fetchTags is true, always include the tags refspec
2306+
if (fetchTags) {
2307+
result.push(exports.tagsRefSpec);
2308+
}
22922309
// SHA
22932310
if (commit) {
22942311
// refs/heads
22952312
if (upperRef.startsWith('REFS/HEADS/')) {
22962313
const branch = ref.substring('refs/heads/'.length);
2297-
return [`+${commit}:refs/remotes/origin/${branch}`];
2314+
result.push(`+${commit}:refs/remotes/origin/${branch}`);
22982315
}
22992316
// refs/pull/
23002317
else if (upperRef.startsWith('REFS/PULL/')) {
23012318
const branch = ref.substring('refs/pull/'.length);
2302-
return [`+${commit}:refs/remotes/pull/${branch}`];
2319+
result.push(`+${commit}:refs/remotes/pull/${branch}`);
23032320
}
23042321
// refs/tags/
23052322
else if (upperRef.startsWith('REFS/TAGS/')) {
2306-
return [`+${commit}:${ref}`];
2323+
if (!fetchTags) {
2324+
result.push(`+${ref}:${ref}`);
2325+
}
23072326
}
23082327
// Otherwise no destination ref
23092328
else {
2310-
return [commit];
2329+
result.push(commit);
23112330
}
23122331
}
23132332
// Unqualified ref, check for a matching branch or tag
23142333
else if (!upperRef.startsWith('REFS/')) {
2315-
return [
2316-
`+refs/heads/${ref}*:refs/remotes/origin/${ref}*`,
2317-
`+refs/tags/${ref}*:refs/tags/${ref}*`
2318-
];
2334+
result.push(`+refs/heads/${ref}*:refs/remotes/origin/${ref}*`);
2335+
if (!fetchTags) {
2336+
result.push(`+refs/tags/${ref}*:refs/tags/${ref}*`);
2337+
}
23192338
}
23202339
// refs/heads/
23212340
else if (upperRef.startsWith('REFS/HEADS/')) {
23222341
const branch = ref.substring('refs/heads/'.length);
2323-
return [`+${ref}:refs/remotes/origin/${branch}`];
2342+
result.push(`+${ref}:refs/remotes/origin/${branch}`);
23242343
}
23252344
// refs/pull/
23262345
else if (upperRef.startsWith('REFS/PULL/')) {
23272346
const branch = ref.substring('refs/pull/'.length);
2328-
return [`+${ref}:refs/remotes/pull/${branch}`];
2347+
result.push(`+${ref}:refs/remotes/pull/${branch}`);
23292348
}
23302349
// refs/tags/
2350+
else if (upperRef.startsWith('REFS/TAGS/')) {
2351+
if (!fetchTags) {
2352+
result.push(`+${ref}:${ref}`);
2353+
}
2354+
}
2355+
// Other refs
23312356
else {
2332-
return [`+${ref}:${ref}`];
2357+
result.push(`+${ref}:${ref}`);
23332358
}
2359+
return result;
23342360
}
23352361
/**
23362362
* Tests whether the initial fetch created the ref at the expected commit
@@ -2366,7 +2392,9 @@ function testRef(git, ref, commit) {
23662392
// refs/tags/
23672393
else if (upperRef.startsWith('REFS/TAGS/')) {
23682394
const tagName = ref.substring('refs/tags/'.length);
2369-
return ((yield git.tagExists(tagName)) && commit === (yield git.revParse(ref)));
2395+
// Use ^{commit} to dereference annotated tags to their underlying commit
2396+
return ((yield git.tagExists(tagName)) &&
2397+
commit === (yield git.revParse(`${ref}^{commit}`)));
23702398
}
23712399
// Unexpected
23722400
else {

0 commit comments

Comments
 (0)