From 885641592076c27bfb56c028cd5612cdad63e16d Mon Sep 17 00:00:00 2001 From: Cory Miller <13227161+cory-miller@users.noreply.github.com> Date: Wed, 14 Dec 2022 16:08:53 -0500 Subject: [PATCH] Implement branch list using callbacks from exec function (#1045) When trying to list local branches to figure out what needs cleaned up during runs on non-ephemeral Actions Runners, we use git rev-parse --symbolic-full-name to get a list of branches. This can lead to ambiguous ref name errors when there are branches and tags with similar names. Part of the reason we use rev-parse --symbolic-full-name vs git branch --list or git rev-parse --symbolic seems to related to a bug in Git 2.18. Until we can deprecate our usage of Git 2.18, I think we need to keep --symbolic-full-name. Since part of the problem is that these ambiguous ref name errors clog the Actions annotation limits, this is a mitigation to suppress those messages until we can get rid of the workaround. --- .licenses/npm/qs.dep.yml | Bin 1929 -> 1929 bytes __test__/git-command-manager.test.ts | 80 +++++++++++++++++ dist/index.js | 127 ++++++++++++++++++++------- package-lock.json | 12 +-- src/git-command-manager.ts | 79 ++++++++++++----- 5 files changed, 242 insertions(+), 56 deletions(-) create mode 100644 __test__/git-command-manager.test.ts diff --git a/.licenses/npm/qs.dep.yml b/.licenses/npm/qs.dep.yml index 2d4aded7024eae8cfe365bda114058fe654bdd40..13367703de673ecf8ac00fbdb0a56797cc29d74d 100644 GIT binary patch delta 14 VcmeC=@8q8#$!w@+uu&$L9RM9o1FZl6 delta 14 VcmeC=@8q8#$!wr!xKSpT9RM9m1FZl6 diff --git a/__test__/git-command-manager.test.ts b/__test__/git-command-manager.test.ts new file mode 100644 index 0000000..6944ff7 --- /dev/null +++ b/__test__/git-command-manager.test.ts @@ -0,0 +1,80 @@ +import * as exec from '@actions/exec' +import * as fshelper from '../lib/fs-helper' +import * as commandManager from '../lib/git-command-manager' + +let git: commandManager.IGitCommandManager +let mockExec = jest.fn() + +describe('git-auth-helper tests', () => { + beforeAll(async () => {}) + + beforeEach(async () => { + jest.spyOn(fshelper, 'fileExistsSync').mockImplementation(jest.fn()) + jest.spyOn(fshelper, 'directoryExistsSync').mockImplementation(jest.fn()) + }) + + afterEach(() => { + jest.restoreAllMocks() + }) + + afterAll(() => {}) + + it('branch list matches', async () => { + mockExec.mockImplementation((path, args, options) => { + console.log(args, options.listeners.stdout) + + if (args.includes('version')) { + options.listeners.stdout(Buffer.from('2.18')) + return 0 + } + + if (args.includes('rev-parse')) { + options.listeners.stdline(Buffer.from('refs/heads/foo')) + options.listeners.stdline(Buffer.from('refs/heads/bar')) + return 0 + } + + return 1 + }) + jest.spyOn(exec, 'exec').mockImplementation(mockExec) + const workingDirectory = 'test' + const lfs = false + git = await commandManager.createCommandManager(workingDirectory, lfs) + + let branches = await git.branchList(false) + + expect(branches).toHaveLength(2) + expect(branches.sort()).toEqual(['foo', 'bar'].sort()) + }) + + it('ambiguous ref name output is captured', async () => { + mockExec.mockImplementation((path, args, options) => { + console.log(args, options.listeners.stdout) + + if (args.includes('version')) { + options.listeners.stdout(Buffer.from('2.18')) + return 0 + } + + if (args.includes('rev-parse')) { + options.listeners.stdline(Buffer.from('refs/heads/foo')) + // If refs/tags/v1 and refs/heads/tags/v1 existed on this repository + options.listeners.errline( + Buffer.from("error: refname 'tags/v1' is ambiguous") + ) + return 0 + } + + return 1 + }) + jest.spyOn(exec, 'exec').mockImplementation(mockExec) + const workingDirectory = 'test' + const lfs = false + git = await commandManager.createCommandManager(workingDirectory, lfs) + + let branches = await git.branchList(false) + + expect(branches).toHaveLength(1) + expect(branches.sort()).toEqual(['foo'].sort()) + }) +}) diff --git a/dist/index.js b/dist/index.js index 96be2f8..ff064d5 100644 --- a/dist/index.js +++ b/dist/index.js @@ -7441,8 +7441,10 @@ class GitCommandManager { const result = []; // Note, this implementation uses "rev-parse --symbolic-full-name" because the output from // "branch --list" is more difficult when in a detached HEAD state. - // Note, this implementation uses "rev-parse --symbolic-full-name" because there is a bug - // in Git 2.18 that causes "rev-parse --symbolic" to output symbolic full names. + // TODO(https://github.com/actions/checkout/issues/786): this implementation uses + // "rev-parse --symbolic-full-name" because there is a bug + // in Git 2.18 that causes "rev-parse --symbolic" to output symbolic full names. When + // 2.18 is no longer supported, we can switch back to --symbolic. const args = ['rev-parse', '--symbolic-full-name']; if (remote) { args.push('--remotes=origin'); @@ -7450,18 +7452,42 @@ class GitCommandManager { else { args.push('--branches'); } - const output = yield this.execGit(args); - for (let branch of output.stdout.trim().split('\n')) { - branch = branch.trim(); - if (branch) { - if (branch.startsWith('refs/heads/')) { - branch = branch.substr('refs/heads/'.length); - } - else if (branch.startsWith('refs/remotes/')) { - branch = branch.substr('refs/remotes/'.length); - } - result.push(branch); + const stderr = []; + const errline = []; + const stdout = []; + const stdline = []; + const listeners = { + stderr: (data) => { + stderr.push(data.toString()); + }, + errline: (data) => { + errline.push(data.toString()); + }, + stdout: (data) => { + stdout.push(data.toString()); + }, + stdline: (data) => { + stdline.push(data.toString()); } + }; + // Suppress the output in order to avoid flooding annotations with innocuous errors. + yield this.execGit(args, false, true, listeners); + core.debug(`stderr callback is: ${stderr}`); + core.debug(`errline callback is: ${errline}`); + core.debug(`stdout callback is: ${stdout}`); + core.debug(`stdline callback is: ${stdline}`); + for (let branch of stdline) { + branch = branch.trim(); + if (!branch) { + continue; + } + if (branch.startsWith('refs/heads/')) { + branch = branch.substring('refs/heads/'.length); + } + else if (branch.startsWith('refs/remotes/')) { + branch = branch.substring('refs/remotes/'.length); + } + result.push(branch); } return result; }); @@ -7712,7 +7738,7 @@ class GitCommandManager { return result; }); } - execGit(args, allowAllExitCodes = false, silent = false) { + execGit(args, allowAllExitCodes = false, silent = false, customListeners = {}) { return __awaiter(this, void 0, void 0, function* () { fshelper.directoryExistsSync(this.workingDirectory, true); const result = new GitOutput(); @@ -7723,20 +7749,24 @@ class GitCommandManager { for (const key of Object.keys(this.gitEnv)) { env[key] = this.gitEnv[key]; } + const defaultListener = { + stdout: (data) => { + stdout.push(data.toString()); + } + }; + const mergedListeners = Object.assign(Object.assign({}, defaultListener), customListeners); const stdout = []; const options = { cwd: this.workingDirectory, env, silent, ignoreReturnCode: allowAllExitCodes, - listeners: { - stdout: (data) => { - stdout.push(data.toString()); - } - } + listeners: mergedListeners }; result.exitCode = yield exec.exec(`"${this.gitPath}"`, args, options); result.stdout = stdout.join(''); + core.debug(result.exitCode.toString()); + core.debug(result.stdout); return result; }); } @@ -13947,6 +13977,7 @@ var encode = function encode(str, defaultEncoder, charset, kind, format) { i += 1; c = 0x10000 + (((c & 0x3FF) << 10) | (string.charCodeAt(i) & 0x3FF)); + /* eslint operator-linebreak: [2, "before"] */ out += hexTable[0xF0 | (c >> 18)] + hexTable[0x80 | ((c >> 12) & 0x3F)] + hexTable[0x80 | ((c >> 6) & 0x3F)] @@ -17572,7 +17603,7 @@ var parseObject = function (chain, val, options, valuesParsed) { ) { obj = []; obj[index] = leaf; - } else { + } else if (cleanRoot !== '__proto__') { obj[cleanRoot] = leaf; } } @@ -34704,6 +34735,7 @@ var arrayPrefixGenerators = { }; var isArray = Array.isArray; +var split = String.prototype.split; var push = Array.prototype.push; var pushToArray = function (arr, valueOrArray) { push.apply(arr, isArray(valueOrArray) ? valueOrArray : [valueOrArray]); @@ -34740,10 +34772,13 @@ var isNonNullishPrimitive = function isNonNullishPrimitive(v) { || typeof v === 'bigint'; }; +var sentinel = {}; + var stringify = function stringify( object, prefix, generateArrayPrefix, + commaRoundTrip, strictNullHandling, skipNulls, encoder, @@ -34759,8 +34794,23 @@ var stringify = function stringify( ) { var obj = object; - if (sideChannel.has(object)) { - throw new RangeError('Cyclic object value'); + var tmpSc = sideChannel; + var step = 0; + var findFlag = false; + while ((tmpSc = tmpSc.get(sentinel)) !== void undefined && !findFlag) { + // Where object last appeared in the ref tree + var pos = tmpSc.get(object); + step += 1; + if (typeof pos !== 'undefined') { + if (pos === step) { + throw new RangeError('Cyclic object value'); + } else { + findFlag = true; // Break while + } + } + if (typeof tmpSc.get(sentinel) === 'undefined') { + step = 0; + } } if (typeof filter === 'function') { @@ -34787,6 +34837,14 @@ var stringify = function stringify( if (isNonNullishPrimitive(obj) || utils.isBuffer(obj)) { if (encoder) { var keyValue = encodeValuesOnly ? prefix : encoder(prefix, defaults.encoder, charset, 'key', format); + if (generateArrayPrefix === 'comma' && encodeValuesOnly) { + var valuesArray = split.call(String(obj), ','); + var valuesJoined = ''; + for (var i = 0; i < valuesArray.length; ++i) { + valuesJoined += (i === 0 ? '' : ',') + formatter(encoder(valuesArray[i], defaults.encoder, charset, 'value', format)); + } + return [formatter(keyValue) + (commaRoundTrip && isArray(obj) && valuesArray.length === 1 ? '[]' : '') + '=' + valuesJoined]; + } return [formatter(keyValue) + '=' + formatter(encoder(obj, defaults.encoder, charset, 'value', format))]; } return [formatter(prefix) + '=' + formatter(String(obj))]; @@ -34801,7 +34859,7 @@ var stringify = function stringify( var objKeys; if (generateArrayPrefix === 'comma' && isArray(obj)) { // we need to join elements in - objKeys = [{ value: obj.length > 0 ? obj.join(',') || null : undefined }]; + objKeys = [{ value: obj.length > 0 ? obj.join(',') || null : void undefined }]; } else if (isArray(filter)) { objKeys = filter; } else { @@ -34809,24 +34867,28 @@ var stringify = function stringify( objKeys = sort ? keys.sort(sort) : keys; } - for (var i = 0; i < objKeys.length; ++i) { - var key = objKeys[i]; - var value = typeof key === 'object' && key.value !== undefined ? key.value : obj[key]; + var adjustedPrefix = commaRoundTrip && isArray(obj) && obj.length === 1 ? prefix + '[]' : prefix; + + for (var j = 0; j < objKeys.length; ++j) { + var key = objKeys[j]; + var value = typeof key === 'object' && typeof key.value !== 'undefined' ? key.value : obj[key]; if (skipNulls && value === null) { continue; } var keyPrefix = isArray(obj) - ? typeof generateArrayPrefix === 'function' ? generateArrayPrefix(prefix, key) : prefix - : prefix + (allowDots ? '.' + key : '[' + key + ']'); + ? typeof generateArrayPrefix === 'function' ? generateArrayPrefix(adjustedPrefix, key) : adjustedPrefix + : adjustedPrefix + (allowDots ? '.' + key : '[' + key + ']'); - sideChannel.set(object, true); + sideChannel.set(object, step); var valueSideChannel = getSideChannel(); + valueSideChannel.set(sentinel, sideChannel); pushToArray(values, stringify( value, keyPrefix, generateArrayPrefix, + commaRoundTrip, strictNullHandling, skipNulls, encoder, @@ -34850,7 +34912,7 @@ var normalizeStringifyOptions = function normalizeStringifyOptions(opts) { return defaults; } - if (opts.encoder !== null && opts.encoder !== undefined && typeof opts.encoder !== 'function') { + if (opts.encoder !== null && typeof opts.encoder !== 'undefined' && typeof opts.encoder !== 'function') { throw new TypeError('Encoder has to be a function.'); } @@ -34923,6 +34985,10 @@ module.exports = function (object, opts) { } var generateArrayPrefix = arrayPrefixGenerators[arrayFormat]; + if (opts && 'commaRoundTrip' in opts && typeof opts.commaRoundTrip !== 'boolean') { + throw new TypeError('`commaRoundTrip` must be a boolean, or absent'); + } + var commaRoundTrip = generateArrayPrefix === 'comma' && opts && opts.commaRoundTrip; if (!objKeys) { objKeys = Object.keys(obj); @@ -34943,6 +35009,7 @@ module.exports = function (object, opts) { obj[key], key, generateArrayPrefix, + commaRoundTrip, options.strictNullHandling, options.skipNulls, options.encode ? options.encoder : null, diff --git a/package-lock.json b/package-lock.json index d0bd501..ab9e4f4 100644 --- a/package-lock.json +++ b/package-lock.json @@ -17110,9 +17110,9 @@ } }, "node_modules/qs": { - "version": "6.10.1", - "resolved": "https://registry.npmjs.org/qs/-/qs-6.10.1.tgz", - "integrity": "sha512-M528Hph6wsSVOBiYUnGf+K/7w0hNshs/duGsNXPUCLH5XAqjEtiPGwNONLV0tBH8NoGb0mvD5JubnUTrujKDTg==", + "version": "6.11.0", + "resolved": "https://registry.npmjs.org/qs/-/qs-6.11.0.tgz", + "integrity": "sha512-MvjoMCJwEarSbUYk5O+nmoSzSutSsTwF85zcHPQ9OrlFoZOYIjaqBAJIqIXjptyD5vThxGq52Xu/MaJzRkIk4Q==", "dependencies": { "side-channel": "^1.0.4" }, @@ -31553,9 +31553,9 @@ "dev": true }, "qs": { - "version": "6.10.1", - "resolved": "https://registry.npmjs.org/qs/-/qs-6.10.1.tgz", - "integrity": "sha512-M528Hph6wsSVOBiYUnGf+K/7w0hNshs/duGsNXPUCLH5XAqjEtiPGwNONLV0tBH8NoGb0mvD5JubnUTrujKDTg==", + "version": "6.11.0", + "resolved": "https://registry.npmjs.org/qs/-/qs-6.11.0.tgz", + "integrity": "sha512-MvjoMCJwEarSbUYk5O+nmoSzSutSsTwF85zcHPQ9OrlFoZOYIjaqBAJIqIXjptyD5vThxGq52Xu/MaJzRkIk4Q==", "requires": { "side-channel": "^1.0.4" } diff --git a/src/git-command-manager.ts b/src/git-command-manager.ts index 699a963..01aedfe 100644 --- a/src/git-command-manager.ts +++ b/src/git-command-manager.ts @@ -94,8 +94,11 @@ class GitCommandManager { // Note, this implementation uses "rev-parse --symbolic-full-name" because the output from // "branch --list" is more difficult when in a detached HEAD state. - // Note, this implementation uses "rev-parse --symbolic-full-name" because there is a bug - // in Git 2.18 that causes "rev-parse --symbolic" to output symbolic full names. + + // TODO(https://github.com/actions/checkout/issues/786): this implementation uses + // "rev-parse --symbolic-full-name" because there is a bug + // in Git 2.18 that causes "rev-parse --symbolic" to output symbolic full names. When + // 2.18 is no longer supported, we can switch back to --symbolic. const args = ['rev-parse', '--symbolic-full-name'] if (remote) { @@ -104,21 +107,49 @@ class GitCommandManager { args.push('--branches') } - const output = await this.execGit(args) + const stderr: string[] = [] + const errline: string[] = [] + const stdout: string[] = [] + const stdline: string[] = [] - for (let branch of output.stdout.trim().split('\n')) { - branch = branch.trim() - if (branch) { - if (branch.startsWith('refs/heads/')) { - branch = branch.substr('refs/heads/'.length) - } else if (branch.startsWith('refs/remotes/')) { - branch = branch.substr('refs/remotes/'.length) - } - - result.push(branch) + const listeners = { + stderr: (data: Buffer) => { + stderr.push(data.toString()) + }, + errline: (data: Buffer) => { + errline.push(data.toString()) + }, + stdout: (data: Buffer) => { + stdout.push(data.toString()) + }, + stdline: (data: Buffer) => { + stdline.push(data.toString()) } } + // Suppress the output in order to avoid flooding annotations with innocuous errors. + await this.execGit(args, false, true, listeners) + + core.debug(`stderr callback is: ${stderr}`) + core.debug(`errline callback is: ${errline}`) + core.debug(`stdout callback is: ${stdout}`) + core.debug(`stdline callback is: ${stdline}`) + + for (let branch of stdline) { + branch = branch.trim() + if (!branch) { + continue + } + + if (branch.startsWith('refs/heads/')) { + branch = branch.substring('refs/heads/'.length) + } else if (branch.startsWith('refs/remotes/')) { + branch = branch.substring('refs/remotes/'.length) + } + + result.push(branch) + } + return result } @@ -395,7 +426,8 @@ class GitCommandManager { private async execGit( args: string[], allowAllExitCodes = false, - silent = false + silent = false, + customListeners = {} ): Promise { fshelper.directoryExistsSync(this.workingDirectory, true) @@ -409,22 +441,29 @@ class GitCommandManager { env[key] = this.gitEnv[key] } - const stdout: string[] = [] + const defaultListener = { + stdout: (data: Buffer) => { + stdout.push(data.toString()) + } + } + const mergedListeners = {...defaultListener, ...customListeners} + + const stdout: string[] = [] const options = { cwd: this.workingDirectory, env, silent, ignoreReturnCode: allowAllExitCodes, - listeners: { - stdout: (data: Buffer) => { - stdout.push(data.toString()) - } - } + listeners: mergedListeners } result.exitCode = await exec.exec(`"${this.gitPath}"`, args, options) result.stdout = stdout.join('') + + core.debug(result.exitCode.toString()) + core.debug(result.stdout) + return result }