Skip to content
New issue

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

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

Already on GitHub? # to your account

getDiffRanges: better fallback for absent patch #2699

Merged
merged 2 commits into from
Jan 16, 2025
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
25 changes: 15 additions & 10 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.

12 changes: 11 additions & 1 deletion lib/analyze.test.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.test.js.map

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

13 changes: 12 additions & 1 deletion src/analyze.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,13 @@ test("getDiffRanges: file unchanged", async (t) => {

test("getDiffRanges: file diff too large", async (t) => {
const diffRanges = runGetDiffRanges(1000000, undefined);
t.deepEqual(diffRanges, undefined);
t.deepEqual(diffRanges, [
{
path: "/checkout/path/test.txt",
startLine: 0,
endLine: 0,
},
]);
});

test("getDiffRanges: diff thunk with single addition range", async (t) => {
Expand Down Expand Up @@ -308,3 +314,8 @@ test("getDiffRanges: no diff context lines", async (t) => {
},
]);
});

test("getDiffRanges: malformed thunk header", async (t) => {
const diffRanges = runGetDiffRanges(2, ["@@ 30 +50,2 @@", "+1", "+2"]);
t.deepEqual(diffRanges, undefined);
});
29 changes: 16 additions & 13 deletions src/analyze.ts
Original file line number Diff line number Diff line change
Expand Up @@ -387,6 +387,13 @@ function getDiffRanges(
fileDiff: FileDiff,
logger: Logger,
): DiffThunkRange[] | undefined {
// Diff-informed queries expect the file path to be absolute. CodeQL always
// uses forward slashes as the path separator, so on Windows we need to
// replace any backslashes with forward slashes.
const filename = path
.join(actionsUtil.getRequiredInput("checkout_path"), fileDiff.filename)
.replaceAll(path.sep, "/");

if (fileDiff.patch === undefined) {
if (fileDiff.changes === 0) {
// There are situations where a changed file legitimately has no diff.
Expand All @@ -397,21 +404,17 @@ function getDiffRanges(
return [];
}
// If a file is reported to have nonzero changes but no patch, that may be
// due to the file diff being too large. In this case, we should return
// undefined to indicate that we cannot process the diff.
logger.warning(
`No patch found for file ${fileDiff.filename} with ${fileDiff.changes} changes.`,
);
return undefined;
// due to the file diff being too large. In this case, we should fall back
// to a special diff range that covers the entire file.
return [
{
path: filename,
startLine: 0,
endLine: 0,
},
];
}

// Diff-informed queries expect the file path to be absolute. CodeQL always
// uses forward slashes as the path separator, so on Windows we need to
// replace any backslashes with forward slashes.
const filename = path
.join(actionsUtil.getRequiredInput("checkout_path"), fileDiff.filename)
.replaceAll(path.sep, "/");

// The 1-based file line number of the current line
let currentLine = 0;
// The 1-based file line number that starts the current range of added lines
Expand Down
Loading