Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve Git subgraph fetching for diff-informed queries #2638

Merged
merged 5 commits into from
Dec 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 25 additions & 3 deletions lib/actions-util.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion lib/actions-util.js.map

Large diffs are not rendered by default.

11 changes: 8 additions & 3 deletions lib/analyze.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion lib/analyze.js.map

Large diffs are not rendered by default.

10 changes: 10 additions & 0 deletions lib/logging.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion lib/logging.js.map

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

29 changes: 27 additions & 2 deletions src/actions-util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ export const determineBaseBranchHeadCommitOid = async function (
};

/**
* Deepen the git history of the given ref by one level. Errors are logged.
* Deepen the git history of HEAD by one level. Errors are logged.
*
* This function uses the `checkout_path` to determine the repository path and
* works only when called from `analyze` or `upload-sarif`.
Expand All @@ -172,7 +172,14 @@ export const deepenGitHistory = async function () {
try {
await runGitCommand(
getOptionalInput("checkout_path"),
["fetch", "--no-tags", "--deepen=1"],
[
"fetch",
"origin",
"HEAD",
"--no-tags",
"--no-recurse-submodules",
"--deepen=1",
],
"Cannot deepen the shallow repository.",
);
} catch {
Expand All @@ -198,6 +205,24 @@ export const gitFetch = async function (branch: string, extraFlags: string[]) {
}
};

/**
* Repack the git repository, using with the given flags. Errors are logged.
*
* This function uses the `checkout_path` to determine the repository path and
* works only when called from `analyze` or `upload-sarif`.
*/
export const gitRepack = async function (flags: string[]) {
try {
await runGitCommand(
getOptionalInput("checkout_path"),
["repack", ...flags],
"Cannot repack the repository.",
);
} catch {
// Errors are already logged by runGitCommand()
}
};

/**
* Compute the all merge bases between the given refs. Returns an empty array
* if no merge base is found, or if there is an error.
Expand Down
30 changes: 19 additions & 11 deletions src/analyze.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import { addDiagnostic, makeDiagnostic } from "./diagnostics";
import { EnvVar } from "./environment";
import { FeatureEnablement, Feature } from "./feature-flags";
import { isScannedLanguage, Language } from "./languages";
import { Logger, withGroup } from "./logging";
import { Logger, withGroupAsync } from "./logging";
import { DatabaseCreationTimings, EventReport } from "./status-report";
import { ToolsFeature } from "./tools-features";
import { endTracingForCluster } from "./tracer-config";
Expand Down Expand Up @@ -256,14 +256,17 @@ export async function setupDiffInformedQueryRun(
if (!(await features.getValue(Feature.DiffInformedQueries, codeql))) {
return undefined;
}
return await withGroup("Generating diff range extension pack", async () => {
const diffRanges = await getPullRequestEditedDiffRanges(
baseRef,
headRef,
logger,
);
return writeDiffRangeDataExtensionPack(logger, diffRanges);
});
return await withGroupAsync(
"Generating diff range extension pack",
async () => {
const diffRanges = await getPullRequestEditedDiffRanges(
baseRef,
headRef,
logger,
);
return writeDiffRangeDataExtensionPack(logger, diffRanges);
},
);
}

interface DiffThunkRange {
Expand Down Expand Up @@ -295,7 +298,7 @@ async function getPullRequestEditedDiffRanges(

// To compute the merge bases between the base branch and the PR topic branch,
// we need to fetch the commit graph from the branch heads to those merge
// babes. The following 4-step procedure does so while limiting the amount of
// babes. The following 6-step procedure does so while limiting the amount of
// history fetched.

// Step 1: Deepen from the PR merge commit to the base branch head and the PR
Expand All @@ -314,7 +317,12 @@ async function getPullRequestEditedDiffRanges(
// Step 4: Fetch the base branch history, stopping when we reach commits that
// are reachable from the PR topic branch head.
await actionsUtil.gitFetch(baseRef, [`--shallow-exclude=${headRef}`]);
// Step 5: Deepen the history so that we have the merge bases between the base
// Step 5: Repack the history to remove the shallow grafts that were added by
// the previous fetches. This step works around a bug that causes subsequent
// deepening fetches to fail with "fatal: error in object: unshallow <SHA>".
// See https://stackoverflow.com/q/63878612
await actionsUtil.gitRepack(["-d"]);
// Step 6: Deepen the history so that we have the merge bases between the base
// branch and the PR topic branch.
await actionsUtil.deepenGitHistory();

Expand Down
12 changes: 12 additions & 0 deletions src/logging.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,18 @@ export function withGroup<T>(groupName: string, f: () => T): T {
}
}

export async function withGroupAsync<T>(
groupName: string,
f: () => Promise<T>,
): Promise<T> {
core.startGroup(groupName);
try {
return await f();
} finally {
core.endGroup();
}
}

/** Format a duration for use in logs. */
export function formatDuration(durationMs: number) {
if (durationMs < 1000) {
Expand Down
Loading