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.
2024-12-23 21:33:29 +01:00 · 2022-12-14 16:08:53 -05:00 · 2022-12-14 16:08:53 -05:00 · 8856415920
commit 8856415920
parent 755da8c3cf
5 changed files with 243 additions and 57 deletions
--- a/.licenses/npm/qs.dep.yml
+++ b/.licenses/npm/qs.dep.yml
@ -1,6 +1,6 @@
 ---
 name: qs
-version: 6.10.1
+version: 6.11.0
 type: npm
 summary: A querystring parser that supports nesting and arrays, with a depth limit
 homepage: https://github.com/ljharb/qs
--- a/test/git-command-manager.test.ts
+++ 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())
+  })
+})
--- 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,
--- 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"
      }
--- 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<GitOutput> {
    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
  }