From a4fcbe27a1658fb9315194c131aa5d8d0748c20a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C3=A9r=C3=A8?= Date: Fri, 29 Dec 2023 13:51:56 -0800 Subject: [PATCH] Use treeless clone to fetch enough history for diff A treeless clone contains all git history (and so can always find the correct merge base), but is more efficient than a normal clone/fetch. In order to use both treeless clone and fetch operations, we now manage git clone, fetch and checkout operations ourselves rather than relying on actions/checkout. --- README.md | 29 ++++--- __tests__/main.test.ts | 15 +++- action.yml | 4 +- badges/coverage.svg | 2 +- dist/index.js | 178 ++++++++++++++++++++++++++--------------- src/git.ts | 71 ++++++++++++++++ src/initializer.ts | 80 +++--------------- src/main.ts | 28 ++++--- 8 files changed, 244 insertions(+), 163 deletions(-) create mode 100644 src/git.ts diff --git a/README.md b/README.md index 8287737..cbe6548 100644 --- a/README.md +++ b/README.md @@ -27,14 +27,11 @@ jobs: runs-on: ubuntu-latest steps: - # Check out a copy of this repository so that we can compute a diff to - # evaluate and load the Action's configuration. - - name: Checkout this repository - uses: actions/checkout@v4 - - # Run the estimation tool - name: Run sizeup # TODO: Replace the version below with your desired version. + # + # For more details please see: + # https://github.com/lerebear/sizeup-action/blob/main/README.md#versioning uses: lerebear/sizeup-action@v0.4.2 id: sizeup-action @@ -49,23 +46,29 @@ jobs: # This input is required. token: "${{ secrets.GITHUB_TOKEN }}" - # Optional arguments that will be forwarded to `git diff` when - # computing the diff to evaluate with this Action. + # Options that will be forwarded to `git diff` when computing the + # diff to evaluate with this workflow. # # Defaults to "--ignore-space-change", which ignores lines of the # diff in which the only change is to the amount of whitespace on the # line. - git-diff-args: "" + git-diff-options: "" - # Path to a YAML configuration file for this Action that is stored in - # this repository. + # Path to a YAML configuration file for this workflow that is stored + # in this repository. # - # This input defaults to "", which instructs the Action to use the + # This input defaults to "", which instructs the workflow to use the # built-in default configuration. configuration-file-path: ".github/workflows/sizeup/config.yaml" ``` -This will use `sizeup` to estimate the reviewability of any pull request opened on your repository using a YAML configuration file found at `.github/workflows/sizeup/config.yaml`. The format of the configuration file is described below. +This will use [`sizeup`](https://github.com/lerebear/sizeup-core) to estimate the reviewability of any pull request opened on your repository using a YAML configuration file found at `.github/workflows/sizeup/config.yaml`. The format of the configuration file is described below. + +Please note that the workflow configuration above does not use [`actions/checkout`](https://github.com/actions/checkout). This is because `actions/checkout` does not provide enough options to make a customized `git diff` command maximally efficient. Instead, this Action will perform its own clone, fetch, and checkout operations using the provided `token`. + +## Versioning + +This Action follows [semantic versioning conventions](https://semver.org), but is still <1.0. Thus, as is common for pre-1.0 software, breaking changes are sometimes introduced in minor version bumps (although patch version bumps will only contain backwards-compatible bug fixes). Please bear this in mind when choosing the version of this Action that you would like to use. ## Configuration diff --git a/__tests__/main.test.ts b/__tests__/main.test.ts index 7ed6295..315d725 100644 --- a/__tests__/main.test.ts +++ b/__tests__/main.test.ts @@ -9,21 +9,31 @@ import * as core from '@actions/core' import * as main from '../src/main' import * as initializer from '../src/initializer' +import { Git } from '../src/git' import * as github from '@actions/github' function pullRequestEventContext(overrides = {}): object { return { eventName: 'pull_request', + repo: { + owner: 'lerebear', + name: 'sizeup-action' + }, payload: { pull_request: { base: { + ref: 'main', repo: { + full_name: 'lerebear/sizeup-action', name: 'sizeup-action', owner: { login: 'lerebear' } } }, + head: { + ref: 'topic' + }, labels: [], number: 1, user: { @@ -69,9 +79,12 @@ describe('action', () => { // Shallow clone original @actions/github context const originalContext = { ...github.context } + // Mock cloning the repo + jest.spyOn(Git.prototype, 'clone').mockImplementation(async () => {}) + // Mock the diff that we use for evaluation. jest - .spyOn(initializer, 'fetchDiff') + .spyOn(Git.prototype, 'diff') .mockImplementation(async () => Promise.resolve( '--- README.md 2023-10-16 16:35:38\n+++ README-AGAIN.md 2023-10-16 16:36:07\n@@ -0,0 +1 @@\n+# Hello, World!' diff --git a/action.yml b/action.yml index d81583f..659bf8a 100644 --- a/action.yml +++ b/action.yml @@ -6,9 +6,9 @@ inputs: token: description: 'GitHub API token with read permissions for pull requests in this repository.' required: true - git-diff-args: + git-diff-options: description: | - Optional custom arguments to forward to `git diff` when retrieving the diff of the pull request that triggered this workflow. + Custom options to forward to `git diff` when retrieving the diff of the pull request that triggered this workflow. The result of that `git diff` command will be used for evaluation with sizeup. default: '--ignore-space-change' configuration-file-path: diff --git a/badges/coverage.svg b/badges/coverage.svg index f7c1988..8b66222 100644 --- a/badges/coverage.svg +++ b/badges/coverage.svg @@ -1 +1 @@ -Coverage: 61.59%Coverage61.59% \ No newline at end of file +Coverage: 64.7%Coverage64.7% \ No newline at end of file diff --git a/dist/index.js b/dist/index.js index 9ad1763..4e3f42e 100644 --- a/dist/index.js +++ b/dist/index.js @@ -16317,6 +16317,96 @@ function wrappy (fn, cb) { } +/***/ }), + +/***/ 6350: +/***/ (function(__unused_webpack_module, exports, __nccwpck_require__) { + +"use strict"; + +var __createBinding = (this && this.__createBinding) || (Object.create ? (function(o, m, k, k2) { + if (k2 === undefined) k2 = k; + var desc = Object.getOwnPropertyDescriptor(m, k); + if (!desc || ("get" in desc ? !m.__esModule : desc.writable || desc.configurable)) { + desc = { enumerable: true, get: function() { return m[k]; } }; + } + Object.defineProperty(o, k2, desc); +}) : (function(o, m, k, k2) { + if (k2 === undefined) k2 = k; + o[k2] = m[k]; +})); +var __setModuleDefault = (this && this.__setModuleDefault) || (Object.create ? (function(o, v) { + Object.defineProperty(o, "default", { enumerable: true, value: v }); +}) : function(o, v) { + o["default"] = v; +}); +var __importStar = (this && this.__importStar) || function (mod) { + if (mod && mod.__esModule) return mod; + var result = {}; + if (mod != null) for (var k in mod) if (k !== "default" && Object.prototype.hasOwnProperty.call(mod, k)) __createBinding(result, mod, k); + __setModuleDefault(result, mod); + return result; +}; +Object.defineProperty(exports, "__esModule", ({ value: true })); +exports.Git = void 0; +const core = __importStar(__nccwpck_require__(2186)); +const lib = __importStar(__nccwpck_require__(9103)); +class Git { + client; + constructor() { + const basicCredential = Buffer.from(`x-access-token:${core.getInput('token')}`, 'utf8').toString('base64'); + const authorizationHeader = `AUTHORIZATION: basic ${basicCredential}`; + core.setSecret(basicCredential); + this.client = lib.simpleGit('.', { + trimmed: true, + config: [`http.extraheader=${authorizationHeader}`] + }); + } + /** + * Clones the repository from which this workflow was triggered. + * + * @param repo The repository to clone in the format "/" + * @param headRef The single branch to clone, which should correspond to the + * head ref of the pull request that triggered this workflow. This is + * required for efficiency. + * @param targetDirectory The directory in which to clone the repository. + */ + async clone(repo, headRef, targetDirectory = '.') { + core.info(`Cloning ${repo} with the single branch "${headRef}"`); + await this.client.clone(`https://github.com/${repo}`, targetDirectory, [ + `--branch=${headRef}`, + '--filter=tree:0', + '--no-tags', + '--single-branch' + ]); + } + /** + * Retrieves the diff of the pull request that triggered this workflow which we + * will use for evaluation. + * + * @param baseRef The base branch relative to which we should produce a diff. This method assumes + * that the head ref containing changes relative to this base ref has already been fetched using + * the `headRef` argument to the `clone` method. + * @returns The diff of the given pull request or `undefined` if we failed to retrieve it. + */ + async diff(baseRef) { + core.debug(`Fetching base ref "${baseRef}"`); + await this.client.fetch([ + 'origin', + `+${baseRef}:${baseRef}`, + `--filter=tree:0`, + '--no-tags', + '--prune', + '--no-recurse-submodules' + ]); + const diffArgs = ['--merge-base', baseRef].concat(core.getInput('git-diff-options').split(/\s+/)); + core.info(`Retrieving diff with \`git diff ${diffArgs.join(' ')}\``); + return this.client.diff(diffArgs); + } +} +exports.Git = Git; + + /***/ }), /***/ 9477: @@ -16348,13 +16438,11 @@ var __importStar = (this && this.__importStar) || function (mod) { return result; }; Object.defineProperty(exports, "__esModule", ({ value: true })); -exports.pullRequestAuthorHasNotOptedIn = exports.workflowTriggeredForUnsupportedEvent = exports.fetchDiff = exports.loadConfiguration = void 0; +exports.pullRequestAuthorHasNotOptedIn = exports.loadConfiguration = void 0; const core = __importStar(__nccwpck_require__(2186)); -const github = __importStar(__nccwpck_require__(5438)); const fs = __importStar(__nccwpck_require__(7147)); const path = __importStar(__nccwpck_require__(1017)); const YAML = __importStar(__nccwpck_require__(4083)); -const simple_git_1 = __nccwpck_require__(9103); /** * Loads either the configuration file provided to the workflow, or the default * configuration file from "./config/default.yaml". @@ -16374,64 +16462,18 @@ function loadConfiguration() { } exports.loadConfiguration = loadConfiguration; /** - * Retrieves the diff of the pull request that triggered this workflow which we - * will use for evaluation. * - * @param pull The pull request that triggered this workflow. - * @returns The diff of the given pull request. + * @param pull The pull request that triggered this workflow + * @param config The configuration for this workflow + * @returns True if the pull request author has not opted into this workflow + * (meaning that we should quit early), false otherwise (meaning that we + * should proceed with evaluation of the pull request). */ -async function fetchDiff(pull) { - const git = (0, simple_git_1.simpleGit)('.', { trimmed: true }); - const diffArgs = ['--merge-base', `origin/${pull.base.ref}`].concat(core.getInput('git-diff-args').split(/\s+/)); - core.info(`Retrieving diff with \`git diff ${diffArgs.join(' ')}\``); - // Fetch all commits for the head branch back to where it diverged from the base. - core.debug(`Fetching ${pull.commits + 1} commits for ${pull.head.ref}`); - await git.fetch([ - 'origin', - `+${pull.head.ref}:${pull.head.ref}`, - `--depth=${pull.commits + 1}`, - '--no-tags', - '--prune', - '--no-recurse-submodules' - ]); - core.debug(`Switching to branch ${pull.head.ref}`); - await git.raw('switch', pull.head.ref); - // Fetch commits for the base branch that were made since the head diverged from it. - const divergedFrom = await git.raw('rev-list', '--max-parents=0', 'HEAD'); - const divergedAt = await git.show([ - '--quiet', - '--date=unix', - '--format=%cd', - divergedFrom - ]); - core.debug(`Retrieving history for origin/${pull.base.ref} since ${divergedFrom} which was committed at ${divergedAt}`); - await git.fetch([ - 'origin', - `+${pull.base.ref}:${pull.base.ref}`, - `--shallow-since=${divergedAt}`, - '--no-tags', - '--prune', - '--no-recurse-submodules' - ]); - // Now we have all relevant history from both base and head branches, so we - // can compute an accurate diff relative to the base branch. - return git.diff(diffArgs); -} -exports.fetchDiff = fetchDiff; -function workflowTriggeredForUnsupportedEvent() { - if (github.context.eventName !== 'pull_request') { - core.setFailed("This action is only supported on the 'pull_request' event, " + - `but it was triggered for '${github.context.eventName}'`); - return true; - } - return false; -} -exports.workflowTriggeredForUnsupportedEvent = workflowTriggeredForUnsupportedEvent; -function pullRequestAuthorHasNotOptedIn(config, pullRequest) { +function pullRequestAuthorHasNotOptedIn(pull, config) { const usersWhoHaveOptedin = config.optIns || []; if (usersWhoHaveOptedin.length && - !usersWhoHaveOptedin.find((login) => login === pullRequest.user.login)) { - core.info(`Skipping evaluation because pull request author @${pullRequest.user.login} has not opted` + + !usersWhoHaveOptedin.find((login) => login === pull.user.login)) { + core.info(`Skipping evaluation because pull request author @${pull.user.login} has not opted` + ' into this workflow'); return true; } @@ -16479,6 +16521,7 @@ const YAML = __importStar(__nccwpck_require__(4083)); const fs = __importStar(__nccwpck_require__(7147)); const path = __importStar(__nccwpck_require__(1017)); const initializer_1 = __nccwpck_require__(9477); +const git_1 = __nccwpck_require__(6350); const DEFAULT_LABEL_PREFIX = 'sizeup/'; const DEFAULT_COMMENT_TEMPLATE = ` 👋 @{{author}} this pull request exceeds the configured reviewability score threshold of {{threshold}}. Your actual score was {{score}}. @@ -16495,13 +16538,19 @@ const DEFAULT_SCORE_THRESHOLD = 100; */ async function run() { try { - if ((0, initializer_1.workflowTriggeredForUnsupportedEvent)()) + if (github.context.eventName !== 'pull_request') { + core.setFailed("This action is only supported on the 'pull_request' event, " + + `but it was triggered for '${github.context.eventName}'`); return; - const config = (0, initializer_1.loadConfiguration)(); + } const pullRequest = github.context.payload.pull_request; - if ((0, initializer_1.pullRequestAuthorHasNotOptedIn)(config, pullRequest)) + const git = new git_1.Git(); + await git.clone(pullRequest.base.repo.full_name, pullRequest.head.ref); + const config = (0, initializer_1.loadConfiguration)(); + if ((0, initializer_1.pullRequestAuthorHasNotOptedIn)(pullRequest, config)) return; - const score = await evaluatePullRequest(pullRequest, config); + const diff = await git.diff(pullRequest.base.ref); + const score = await evaluatePullRequest(pullRequest, diff, config); await applyCategoryLabel(pullRequest, score, config); await addScoreThresholdExceededComment(pullRequest, score, config); } @@ -16521,8 +16570,8 @@ exports.run = run; * to the `sizeup` library. * @returns The score that the pull request received */ -async function evaluatePullRequest(pull, config) { - const pullRequestNickname = `${pull.base.repo.owner.login}/${pull.base.repo.name}#${pull.number}`; +async function evaluatePullRequest(pull, diff, config) { + const pullRequestNickname = `${pull.base.repo.full_name}#${pull.number}`; core.info(`Evaluating pull request ${pullRequestNickname}`); let sizeupConfigFile = undefined; if (config.sizeup) { @@ -16530,12 +16579,11 @@ async function evaluatePullRequest(pull, config) { fs.mkdirSync(path.dirname(sizeupConfigFile)); fs.writeFileSync(sizeupConfigFile, YAML.stringify(config.sizeup)); } - const diff = await (0, initializer_1.fetchDiff)(pull); const score = sizeup_core_1.SizeUp.evaluate(diff, sizeupConfigFile); if (sizeupConfigFile) { fs.rmSync(sizeupConfigFile, { force: true, recursive: true }); } - core.info(`${pullRequestNickname} received a score of ${score.result} (${score.category.name})`); + core.info(`Pull request ${pullRequestNickname} received a score of ${score.result} (${score.category.name})`); core.info(`The score was computed as follows:\n${score.toString()}`); core.setOutput('score', score.result); core.setOutput('category', score.category.name); diff --git a/src/git.ts b/src/git.ts new file mode 100644 index 0000000..4578187 --- /dev/null +++ b/src/git.ts @@ -0,0 +1,71 @@ +import * as core from '@actions/core' +import * as lib from 'simple-git' + +export class Git { + private client: lib.SimpleGit + + constructor() { + const basicCredential = Buffer.from( + `x-access-token:${core.getInput('token')}`, + 'utf8' + ).toString('base64') + const authorizationHeader = `AUTHORIZATION: basic ${basicCredential}` + core.setSecret(basicCredential) + + this.client = lib.simpleGit('.', { + trimmed: true, + config: [`http.extraheader=${authorizationHeader}`] + }) + } + + /** + * Clones the repository from which this workflow was triggered. + * + * @param repo The repository to clone in the format "/" + * @param headRef The single branch to clone, which should correspond to the + * head ref of the pull request that triggered this workflow. This is + * required for efficiency. + * @param targetDirectory The directory in which to clone the repository. + */ + async clone( + repo: string, + headRef: string, + targetDirectory = '.' + ): Promise { + core.info(`Cloning ${repo} with the single branch "${headRef}"`) + + await this.client.clone(`https://github.com/${repo}`, targetDirectory, [ + `--branch=${headRef}`, + '--filter=tree:0', + '--no-tags', + '--single-branch' + ]) + } + + /** + * Retrieves the diff of the pull request that triggered this workflow which we + * will use for evaluation. + * + * @param baseRef The base branch relative to which we should produce a diff. This method assumes + * that the head ref containing changes relative to this base ref has already been fetched using + * the `headRef` argument to the `clone` method. + * @returns The diff of the given pull request or `undefined` if we failed to retrieve it. + */ + async diff(baseRef: string): Promise { + core.debug(`Fetching base ref "${baseRef}"`) + await this.client.fetch([ + 'origin', + `+${baseRef}:${baseRef}`, + `--filter=tree:0`, + '--no-tags', + '--prune', + '--no-recurse-submodules' + ]) + + const diffArgs = ['--merge-base', baseRef].concat( + core.getInput('git-diff-options').split(/\s+/) + ) + core.info(`Retrieving diff with \`git diff ${diffArgs.join(' ')}\``) + return this.client.diff(diffArgs) + } +} diff --git a/src/initializer.ts b/src/initializer.ts index 34e1992..58a1f97 100644 --- a/src/initializer.ts +++ b/src/initializer.ts @@ -1,10 +1,8 @@ import * as core from '@actions/core' -import * as github from '@actions/github' import { Configuration } from './configuration' import * as fs from 'fs' import * as path from 'path' import * as YAML from 'yaml' -import { simpleGit } from 'simple-git' import { PullRequest } from '@octokit/webhooks-types' // eslint-disable-line import/no-unresolved /** @@ -27,85 +25,25 @@ export function loadConfiguration(): Configuration { } /** - * Retrieves the diff of the pull request that triggered this workflow which we - * will use for evaluation. * - * @param pull The pull request that triggered this workflow. - * @returns The diff of the given pull request. + * @param pull The pull request that triggered this workflow + * @param config The configuration for this workflow + * @returns True if the pull request author has not opted into this workflow + * (meaning that we should quit early), false otherwise (meaning that we + * should proceed with evaluation of the pull request). */ -export async function fetchDiff(pull: PullRequest): Promise { - const git = simpleGit('.', { trimmed: true }) - const diffArgs = ['--merge-base', `origin/${pull.base.ref}`].concat( - core.getInput('git-diff-args').split(/\s+/) - ) - - core.info(`Retrieving diff with \`git diff ${diffArgs.join(' ')}\``) - - // Fetch all commits for the head branch back to where it diverged from the base. - core.debug(`Fetching ${pull.commits + 1} commits for ${pull.head.ref}`) - await git.fetch([ - 'origin', - `+${pull.head.ref}:${pull.head.ref}`, - `--depth=${pull.commits + 1}`, - '--no-tags', - '--prune', - '--no-recurse-submodules' - ]) - - core.debug(`Switching to branch ${pull.head.ref}`) - await git.raw('switch', pull.head.ref) - - // Fetch commits for the base branch that were made since the head diverged from it. - const divergedFrom = await git.raw('rev-list', '--max-parents=0', 'HEAD') - const divergedAt = await git.show([ - '--quiet', - '--date=unix', - '--format=%cd', - divergedFrom - ]) - core.debug( - `Retrieving history for origin/${pull.base.ref} since ${divergedFrom} which was committed at ${divergedAt}` - ) - await git.fetch([ - 'origin', - `+${pull.base.ref}:${pull.base.ref}`, - `--shallow-since=${divergedAt}`, - '--no-tags', - '--prune', - '--no-recurse-submodules' - ]) - - // Now we have all relevant history from both base and head branches, so we - // can compute an accurate diff relative to the base branch. - return git.diff(diffArgs) -} - -export function workflowTriggeredForUnsupportedEvent(): boolean { - if (github.context.eventName !== 'pull_request') { - core.setFailed( - "This action is only supported on the 'pull_request' event, " + - `but it was triggered for '${github.context.eventName}'` - ) - return true - } - - return false -} - export function pullRequestAuthorHasNotOptedIn( - config: Configuration, - pullRequest: PullRequest + pull: PullRequest, + config: Configuration ): boolean { const usersWhoHaveOptedin = config.optIns || [] if ( usersWhoHaveOptedin.length && - !usersWhoHaveOptedin.find( - (login: string) => login === pullRequest.user.login - ) + !usersWhoHaveOptedin.find((login: string) => login === pull.user.login) ) { core.info( - `Skipping evaluation because pull request author @${pullRequest.user.login} has not opted` + + `Skipping evaluation because pull request author @${pull.user.login} has not opted` + ' into this workflow' ) return true diff --git a/src/main.ts b/src/main.ts index 04f9c42..b384a46 100644 --- a/src/main.ts +++ b/src/main.ts @@ -7,11 +7,10 @@ import * as fs from 'fs' import * as path from 'path' import { Configuration } from './configuration' import { - fetchDiff, loadConfiguration, - pullRequestAuthorHasNotOptedIn, - workflowTriggeredForUnsupportedEvent + pullRequestAuthorHasNotOptedIn } from './initializer' +import { Git } from './git' const DEFAULT_LABEL_PREFIX = 'sizeup/' const DEFAULT_COMMENT_TEMPLATE = ` @@ -30,15 +29,24 @@ const DEFAULT_SCORE_THRESHOLD = 100 */ export async function run(): Promise { try { - if (workflowTriggeredForUnsupportedEvent()) return + if (github.context.eventName !== 'pull_request') { + core.setFailed( + "This action is only supported on the 'pull_request' event, " + + `but it was triggered for '${github.context.eventName}'` + ) + return + } - const config = loadConfiguration() const pullRequest = github.context.payload.pull_request as PullRequest - if (pullRequestAuthorHasNotOptedIn(config, pullRequest)) return + const git = new Git() + await git.clone(pullRequest.base.repo.full_name, pullRequest.head.ref) - const score = await evaluatePullRequest(pullRequest, config) + const config = loadConfiguration() + if (pullRequestAuthorHasNotOptedIn(pullRequest, config)) return + const diff = await git.diff(pullRequest.base.ref) + const score = await evaluatePullRequest(pullRequest, diff, config) await applyCategoryLabel(pullRequest, score, config) await addScoreThresholdExceededComment(pullRequest, score, config) } catch (error) { @@ -58,9 +66,10 @@ export async function run(): Promise { */ async function evaluatePullRequest( pull: PullRequest, + diff: string, config: Configuration ): Promise { - const pullRequestNickname = `${pull.base.repo.owner.login}/${pull.base.repo.name}#${pull.number}` + const pullRequestNickname = `${pull.base.repo.full_name}#${pull.number}` core.info(`Evaluating pull request ${pullRequestNickname}`) let sizeupConfigFile = undefined @@ -70,7 +79,6 @@ async function evaluatePullRequest( fs.writeFileSync(sizeupConfigFile, YAML.stringify(config.sizeup)) } - const diff = await fetchDiff(pull) const score = SizeUp.evaluate(diff, sizeupConfigFile) if (sizeupConfigFile) { @@ -78,7 +86,7 @@ async function evaluatePullRequest( } core.info( - `${pullRequestNickname} received a score of ${score.result} (${ + `Pull request ${pullRequestNickname} received a score of ${score.result} (${ score.category!.name })` )