From 0b18612ac3421416efb1c3622162ce72416623f1 Mon Sep 17 00:00:00 2001 From: Michal Dorner Date: Mon, 12 Apr 2021 16:29:29 +0200 Subject: [PATCH] Improve support for PR events + verify base and ref are used correctly --- action.yml | 1 - dist/index.js | 126 +++++++++++++++++++++++++++++++++----------------- src/git.ts | 59 +++++++++++++++-------- src/main.ts | 78 +++++++++++++++++++++---------- 4 files changed, 176 insertions(+), 88 deletions(-) diff --git a/action.yml b/action.yml index f8a96e16..dc515281 100644 --- a/action.yml +++ b/action.yml @@ -13,7 +13,6 @@ inputs: description: | Git reference (e.g. branch name) from which the changes will be detected. This option is ignored if action is triggered by pull_request event. - default: ${{ github.ref }} required: false base: description: | diff --git a/dist/index.js b/dist/index.js index 914be199..df8c2022 100644 --- a/dist/index.js +++ b/dist/index.js @@ -3830,19 +3830,15 @@ async function getChangesInLastCommit() { return parseGitDiffOutput(output); } exports.getChangesInLastCommit = getChangesInLastCommit; -async function getChanges(baseRef) { - if (!(await hasCommit(baseRef))) { - // Fetch single commit - core.startGroup(`Fetching ${baseRef} from origin`); - await exec_1.default('git', ['fetch', '--depth=1', '--no-tags', 'origin', baseRef]); - core.endGroup(); - } +async function getChanges(base, head) { + const baseRef = await ensureRefAvailable(base); + const headRef = await ensureRefAvailable(head); // Get differences between ref and HEAD - core.startGroup(`Change detection ${baseRef}..HEAD`); + core.startGroup(`Change detection ${base}..${head}`); let output = ''; try { // Two dots '..' change detection - directly compares two versions - output = (await exec_1.default('git', ['diff', '--no-renames', '--name-status', '-z', `${baseRef}..HEAD`])).stdout; + output = (await exec_1.default('git', ['diff', '--no-renames', '--name-status', '-z', `${baseRef}..${headRef}`])).stdout; } finally { fixStdOutNullTermination(); @@ -3877,24 +3873,24 @@ async function getChangesSinceMergeBase(base, head, initialFetchDepth) { let noMergeBase = false; core.startGroup(`Searching for merge-base ${base}...${head}`); try { - baseRef = await getFullRef(base); - headRef = await getFullRef(head); + baseRef = await getLocalRef(base); + headRef = await getLocalRef(head); if (!(await hasMergeBase())) { await exec_1.default('git', ['fetch', '--no-tags', `--depth=${initialFetchDepth}`, 'origin', base, head]); if (baseRef === undefined || headRef === undefined) { - baseRef = baseRef !== null && baseRef !== void 0 ? baseRef : (await getFullRef(base)); - headRef = headRef !== null && headRef !== void 0 ? headRef : (await getFullRef(head)); + baseRef = baseRef !== null && baseRef !== void 0 ? baseRef : (await getLocalRef(base)); + headRef = headRef !== null && headRef !== void 0 ? headRef : (await getLocalRef(head)); if (baseRef === undefined || headRef === undefined) { await exec_1.default('git', ['fetch', '--tags', '--depth=1', 'origin', base, head], { ignoreReturnCode: true // returns exit code 1 if tags on remote were updated - we can safely ignore it }); - baseRef = baseRef !== null && baseRef !== void 0 ? baseRef : (await getFullRef(base)); - headRef = headRef !== null && headRef !== void 0 ? headRef : (await getFullRef(head)); + baseRef = baseRef !== null && baseRef !== void 0 ? baseRef : (await getLocalRef(base)); + headRef = headRef !== null && headRef !== void 0 ? headRef : (await getLocalRef(head)); if (baseRef === undefined) { - throw new Error(`Could not determine what is ${base} - fetch works but it's not a branch or tag`); + throw new Error(`Could not determine what is ${base} - fetch works but it's not a branch, tag or commit SHA`); } if (headRef === undefined) { - throw new Error(`Could not determine what is ${head} - fetch works but it's not a branch or tag`); + throw new Error(`Could not determine what is ${head} - fetch works but it's not a branch, tag or commit SHA`); } } } @@ -4018,9 +4014,9 @@ async function getCommitCount() { const count = parseInt(output); return isNaN(count) ? 0 : count; } -async function getFullRef(shortName) { +async function getLocalRef(shortName) { if (isGitSha(shortName)) { - return shortName; + return hasCommit(shortName) ? shortName : undefined; } const output = (await exec_1.default('git', ['show-ref', shortName], { ignoreReturnCode: true })).stdout; const refs = output @@ -4036,6 +4032,27 @@ async function getFullRef(shortName) { } return refs[0]; } +async function ensureRefAvailable(name) { + core.startGroup(`Ensuring ${name} is fetched from origin`); + try { + let ref = await getLocalRef(name); + if (ref === undefined) { + await exec_1.default('git', ['fetch', '--depth=1', '--no-tags', 'origin', name]); + } + ref = await getLocalRef(name); + if (ref === undefined) { + await exec_1.default('git', ['fetch', '--depth=1', '--tags', 'origin', name]); + } + ref = await getLocalRef(name); + if (ref === undefined) { + throw new Error(`Could not determine what is ${name} - fetch works but it's not a branch, tag or commit SHA`); + } + return ref; + } + finally { + core.endGroup(); + } +} function fixStdOutNullTermination() { // Previous command uses NULL as delimiters and output is printed to stdout. // We have to make sure next thing written to stdout will start on new line. @@ -4736,13 +4753,29 @@ async function getChangedFiles(token, base, ref, initialFetchDepth) { // if base is 'HEAD' only local uncommitted changes will be detected // This is the simplest case as we don't need to fetch more commits or evaluate current/before refs if (base === git.HEAD) { + if (ref) { + core.warning(`'ref' input parameter is ignored when 'base' is set to HEAD`); + } return await git.getChangesOnHead(); } - if (github.context.eventName === 'pull_request' || github.context.eventName === 'pull_request_target') { + const prEvents = ['pull_request', 'pull_request_review', 'pull_request_review_comment', 'pull_request_target']; + if (prEvents.includes(github.context.eventName)) { + if (ref) { + core.warning(`'ref' input parameter is ignored when 'base' is set to HEAD`); + } + if (base) { + core.warning(`'base' input parameter is ignored when action is triggered by pull request event`); + } const pr = github.context.payload.pull_request; if (token) { return await getChangedFilesFromApi(token, pr); } + if (github.context.eventName === 'pull_request_target') { + // pull_request_target is executed in context of base branch and GITHUB_SHA points to last commit in base branch + // Therefor it's not possible to look at changes in last commit + // At the same time we don't want to fetch any code from forked repository + throw new Error(`'token' input parameter is required if action is triggered by 'pull_request_target' event`); + } core.info('Github token is not available - changes will be detected from PRs merge commit'); return await git.getChangesInLastCommit(); } @@ -4752,57 +4785,64 @@ async function getChangedFiles(token, base, ref, initialFetchDepth) { } async function getChangedFilesFromGit(base, head, initialFetchDepth) { var _a; - const defaultRef = (_a = github.context.payload.repository) === null || _a === void 0 ? void 0 : _a.default_branch; + const defaultBranch = (_a = github.context.payload.repository) === null || _a === void 0 ? void 0 : _a.default_branch; const beforeSha = github.context.eventName === 'push' ? github.context.payload.before : null; - const ref = git.getShortName(head || github.context.ref) || - (core.warning(`'ref' field is missing in event payload - using current branch, tag or commit SHA`), - await git.getCurrentRef()); - const baseRef = git.getShortName(base) || defaultRef; - if (!baseRef) { + const currentRef = await git.getCurrentRef(); + head = git.getShortName(head || github.context.ref || currentRef); + base = git.getShortName(base || defaultBranch); + if (!head) { + throw new Error("This action requires 'head' input to be configured, 'ref' to be set in the event payload or branch/tag checked out in current git repository"); + } + if (!base) { throw new Error("This action requires 'base' input to be configured or 'repository.default_branch' to be set in the event payload"); } - const isBaseRefSha = git.isGitSha(baseRef); - const isBaseRefSameAsRef = baseRef === ref; + const isBaseSha = git.isGitSha(base); + const isBaseSameAsHead = base === head; // If base is commit SHA we will do comparison against the referenced commit // Or if base references same branch it was pushed to, we will do comparison against the previously pushed commit - if (isBaseRefSha || isBaseRefSameAsRef) { - if (!isBaseRefSha && !beforeSha) { + if (isBaseSha || isBaseSameAsHead) { + const baseSha = isBaseSha ? base : beforeSha; + if (!baseSha) { core.warning(`'before' field is missing in event payload - changes will be detected from last commit`); + if (head !== currentRef) { + core.warning(`Ref ${head} is not checked out - results might be incorrect!`); + } return await git.getChangesInLastCommit(); } - const baseSha = isBaseRefSha ? baseRef : beforeSha; // If there is no previously pushed commit, // we will do comparison against the default branch or return all as added if (baseSha === git.NULL_SHA) { - if (defaultRef && baseRef !== defaultRef) { - core.info(`First push of a branch detected - changes will be detected against the default branch ${defaultRef}`); - return await git.getChangesSinceMergeBase(defaultRef, ref, initialFetchDepth); + if (defaultBranch && base !== defaultBranch) { + core.info(`First push of a branch detected - changes will be detected against the default branch ${defaultBranch}`); + return await git.getChangesSinceMergeBase(defaultBranch, head, initialFetchDepth); } else { core.info('Initial push detected - all files will be listed as added'); + if (head !== currentRef) { + core.warning(`Ref ${head} is not checked out - results might be incorrect!`); + } return await git.listAllFilesAsAdded(); } } - core.info(`Changes will be detected against commit (${baseSha})`); - return await git.getChanges(baseSha); + core.info(`Changes will be detected between ${baseSha} and ${head}`); + return await git.getChanges(baseSha, head); } - // Changes introduced by current branch against the base branch - core.info(`Changes will be detected against ${baseRef}`); - return await git.getChangesSinceMergeBase(baseRef, ref, initialFetchDepth); + core.info(`Changes will be detected between ${base} and ${head}`); + return await git.getChangesSinceMergeBase(base, head, initialFetchDepth); } // Uses github REST api to get list of files changed in PR -async function getChangedFilesFromApi(token, pullRequest) { - core.startGroup(`Fetching list of changed files for PR#${pullRequest.number} from Github API`); +async function getChangedFilesFromApi(token, prNumber) { + core.startGroup(`Fetching list of changed files for PR#${prNumber.number} from Github API`); try { const client = new github.GitHub(token); const per_page = 100; const files = []; for (let page = 1;; page++) { - core.info(`Invoking listFiles(pull_number: ${pullRequest.number}, page: ${page}, per_page: ${per_page})`); + core.info(`Invoking listFiles(pull_number: ${prNumber.number}, page: ${page}, per_page: ${per_page})`); const response = await client.pulls.listFiles({ owner: github.context.repo.owner, repo: github.context.repo.repo, - pull_number: pullRequest.number, + pull_number: prNumber.number, per_page, page }); diff --git a/src/git.ts b/src/git.ts index df563087..e1853bda 100644 --- a/src/git.ts +++ b/src/git.ts @@ -18,20 +18,16 @@ export async function getChangesInLastCommit(): Promise { return parseGitDiffOutput(output) } -export async function getChanges(baseRef: string): Promise { - if (!(await hasCommit(baseRef))) { - // Fetch single commit - core.startGroup(`Fetching ${baseRef} from origin`) - await exec('git', ['fetch', '--depth=1', '--no-tags', 'origin', baseRef]) - core.endGroup() - } +export async function getChanges(base: string, head: string): Promise { + const baseRef = await ensureRefAvailable(base) + const headRef = await ensureRefAvailable(head) // Get differences between ref and HEAD - core.startGroup(`Change detection ${baseRef}..HEAD`) + core.startGroup(`Change detection ${base}..${head}`) let output = '' try { // Two dots '..' change detection - directly compares two versions - output = (await exec('git', ['diff', '--no-renames', '--name-status', '-z', `${baseRef}..HEAD`])).stdout + output = (await exec('git', ['diff', '--no-renames', '--name-status', '-z', `${baseRef}..${headRef}`])).stdout } finally { fixStdOutNullTermination() core.endGroup() @@ -67,24 +63,28 @@ export async function getChangesSinceMergeBase(base: string, head: string, initi let noMergeBase = false core.startGroup(`Searching for merge-base ${base}...${head}`) try { - baseRef = await getFullRef(base) - headRef = await getFullRef(head) + baseRef = await getLocalRef(base) + headRef = await getLocalRef(head) if (!(await hasMergeBase())) { await exec('git', ['fetch', '--no-tags', `--depth=${initialFetchDepth}`, 'origin', base, head]) if (baseRef === undefined || headRef === undefined) { - baseRef = baseRef ?? (await getFullRef(base)) - headRef = headRef ?? (await getFullRef(head)) + baseRef = baseRef ?? (await getLocalRef(base)) + headRef = headRef ?? (await getLocalRef(head)) if (baseRef === undefined || headRef === undefined) { await exec('git', ['fetch', '--tags', '--depth=1', 'origin', base, head], { ignoreReturnCode: true // returns exit code 1 if tags on remote were updated - we can safely ignore it }) - baseRef = baseRef ?? (await getFullRef(base)) - headRef = headRef ?? (await getFullRef(head)) + baseRef = baseRef ?? (await getLocalRef(base)) + headRef = headRef ?? (await getLocalRef(head)) if (baseRef === undefined) { - throw new Error(`Could not determine what is ${base} - fetch works but it's not a branch or tag`) + throw new Error( + `Could not determine what is ${base} - fetch works but it's not a branch, tag or commit SHA` + ) } if (headRef === undefined) { - throw new Error(`Could not determine what is ${head} - fetch works but it's not a branch or tag`) + throw new Error( + `Could not determine what is ${head} - fetch works but it's not a branch, tag or commit SHA` + ) } } } @@ -212,9 +212,9 @@ async function getCommitCount(): Promise { return isNaN(count) ? 0 : count } -async function getFullRef(shortName: string): Promise { +async function getLocalRef(shortName: string): Promise { if (isGitSha(shortName)) { - return shortName + return hasCommit(shortName) ? shortName : undefined } const output = (await exec('git', ['show-ref', shortName], {ignoreReturnCode: true})).stdout @@ -235,6 +235,27 @@ async function getFullRef(shortName: string): Promise { return refs[0] } +async function ensureRefAvailable(name: string): Promise { + core.startGroup(`Ensuring ${name} is fetched from origin`) + try { + let ref = await getLocalRef(name) + if (ref === undefined) { + await exec('git', ['fetch', '--depth=1', '--no-tags', 'origin', name]) + } + ref = await getLocalRef(name) + if (ref === undefined) { + await exec('git', ['fetch', '--depth=1', '--tags', 'origin', name]) + } + ref = await getLocalRef(name) + if (ref === undefined) { + throw new Error(`Could not determine what is ${name} - fetch works but it's not a branch, tag or commit SHA`) + } + return ref + } finally { + core.endGroup() + } +} + function fixStdOutNullTermination(): void { // Previous command uses NULL as delimiters and output is printed to stdout. // We have to make sure next thing written to stdout will start on new line. diff --git a/src/main.ts b/src/main.ts index 466d695e..d2cb6784 100644 --- a/src/main.ts +++ b/src/main.ts @@ -61,14 +61,30 @@ async function getChangedFiles(token: string, base: string, ref: string, initial // if base is 'HEAD' only local uncommitted changes will be detected // This is the simplest case as we don't need to fetch more commits or evaluate current/before refs if (base === git.HEAD) { + if (ref) { + core.warning(`'ref' input parameter is ignored when 'base' is set to HEAD`) + } return await git.getChangesOnHead() } - if (github.context.eventName === 'pull_request' || github.context.eventName === 'pull_request_target') { + const prEvents = ['pull_request', 'pull_request_review', 'pull_request_review_comment', 'pull_request_target'] + if (prEvents.includes(github.context.eventName)) { + if (ref) { + core.warning(`'ref' input parameter is ignored when 'base' is set to HEAD`) + } + if (base) { + core.warning(`'base' input parameter is ignored when action is triggered by pull request event`) + } const pr = github.context.payload.pull_request as Webhooks.WebhookPayloadPullRequestPullRequest if (token) { return await getChangedFilesFromApi(token, pr) } + if (github.context.eventName === 'pull_request_target') { + // pull_request_target is executed in context of base branch and GITHUB_SHA points to last commit in base branch + // Therefor it's not possible to look at changes in last commit + // At the same time we don't want to fetch any code from forked repository + throw new Error(`'token' input parameter is required if action is triggered by 'pull_request_target' event`) + } core.info('Github token is not available - changes will be detected from PRs merge commit') return await git.getChangesInLastCommit() } else { @@ -77,73 +93,85 @@ async function getChangedFiles(token: string, base: string, ref: string, initial } async function getChangedFilesFromGit(base: string, head: string, initialFetchDepth: number): Promise { - const defaultRef = github.context.payload.repository?.default_branch + const defaultBranch = github.context.payload.repository?.default_branch const beforeSha = github.context.eventName === 'push' ? (github.context.payload as Webhooks.WebhookPayloadPush).before : null - const ref = - git.getShortName(head || github.context.ref) || - (core.warning(`'ref' field is missing in event payload - using current branch, tag or commit SHA`), - await git.getCurrentRef()) + const currentRef = await git.getCurrentRef() + + head = git.getShortName(head || github.context.ref || currentRef) + base = git.getShortName(base || defaultBranch) - const baseRef = git.getShortName(base) || defaultRef - if (!baseRef) { + if (!head) { + throw new Error( + "This action requires 'head' input to be configured, 'ref' to be set in the event payload or branch/tag checked out in current git repository" + ) + } + + if (!base) { throw new Error( "This action requires 'base' input to be configured or 'repository.default_branch' to be set in the event payload" ) } - const isBaseRefSha = git.isGitSha(baseRef) - const isBaseRefSameAsRef = baseRef === ref + const isBaseSha = git.isGitSha(base) + const isBaseSameAsHead = base === head // If base is commit SHA we will do comparison against the referenced commit // Or if base references same branch it was pushed to, we will do comparison against the previously pushed commit - if (isBaseRefSha || isBaseRefSameAsRef) { - if (!isBaseRefSha && !beforeSha) { + if (isBaseSha || isBaseSameAsHead) { + const baseSha = isBaseSha ? base : beforeSha + if (!baseSha) { core.warning(`'before' field is missing in event payload - changes will be detected from last commit`) + if (head !== currentRef) { + core.warning(`Ref ${head} is not checked out - results might be incorrect!`) + } return await git.getChangesInLastCommit() } - const baseSha = isBaseRefSha ? baseRef : beforeSha // If there is no previously pushed commit, // we will do comparison against the default branch or return all as added if (baseSha === git.NULL_SHA) { - if (defaultRef && baseRef !== defaultRef) { - core.info(`First push of a branch detected - changes will be detected against the default branch ${defaultRef}`) - return await git.getChangesSinceMergeBase(defaultRef, ref, initialFetchDepth) + if (defaultBranch && base !== defaultBranch) { + core.info( + `First push of a branch detected - changes will be detected against the default branch ${defaultBranch}` + ) + return await git.getChangesSinceMergeBase(defaultBranch, head, initialFetchDepth) } else { core.info('Initial push detected - all files will be listed as added') + if (head !== currentRef) { + core.warning(`Ref ${head} is not checked out - results might be incorrect!`) + } return await git.listAllFilesAsAdded() } } - core.info(`Changes will be detected against commit (${baseSha})`) - return await git.getChanges(baseSha) + core.info(`Changes will be detected between ${baseSha} and ${head}`) + return await git.getChanges(baseSha, head) } - // Changes introduced by current branch against the base branch - core.info(`Changes will be detected against ${baseRef}`) - return await git.getChangesSinceMergeBase(baseRef, ref, initialFetchDepth) + core.info(`Changes will be detected between ${base} and ${head}`) + return await git.getChangesSinceMergeBase(base, head, initialFetchDepth) } // Uses github REST api to get list of files changed in PR async function getChangedFilesFromApi( token: string, - pullRequest: Webhooks.WebhookPayloadPullRequestPullRequest + prNumber: Webhooks.WebhookPayloadPullRequestPullRequest ): Promise { - core.startGroup(`Fetching list of changed files for PR#${pullRequest.number} from Github API`) + core.startGroup(`Fetching list of changed files for PR#${prNumber.number} from Github API`) try { const client = new github.GitHub(token) const per_page = 100 const files: File[] = [] for (let page = 1; ; page++) { - core.info(`Invoking listFiles(pull_number: ${pullRequest.number}, page: ${page}, per_page: ${per_page})`) + core.info(`Invoking listFiles(pull_number: ${prNumber.number}, page: ${page}, per_page: ${per_page})`) const response = await client.pulls.listFiles({ owner: github.context.repo.owner, repo: github.context.repo.repo, - pull_number: pullRequest.number, + pull_number: prNumber.number, per_page, page })