-
Notifications
You must be signed in to change notification settings - Fork 321
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
Add support for diff-informed queries #2559
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I have a bunch of comments and questions — feel free to only address the ones that make sense.
src/actions-util.ts
Outdated
/** | ||
* Deepen the git history of the given ref by one level. Errors are ignored. | ||
*/ | ||
export const deepenGitHistory = async function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two comments about the additions to this file in general:
- Why completely ignore errors? Does it make sense to at least debug log them?
- Nit, optional: This PR adds a bunch of functionality that interacts with git — now might be a good time to pull this functionality out into its own file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error is indeed logged centrally within runGitCommand()
. By "Errors are ignored", I was trying to convey that errors are not reported back to the caller. I have update the function documentation to clarify the behavior.
Moving the git interactions to its own file sounds like a good idea, though trying to incorporate that into this PR is a bit messy. Can I defer that to a dedicated PR after this one merges?
export const deepenGitHistory = async function () { | ||
try { | ||
await runGitCommand( | ||
getOptionalInput("checkout_path"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be worth noting that this function (and others) only work when called from analyze
or upload-sarif
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a note to the function documentation, here and for other git functions.
"-c", | ||
"core.quotePath=false", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain the difference between this approach and using the -z
flag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The -z
flag affects only specific modes: --raw
, --numstat
, --name-only
and --name-status
. Since we need the actual diff line ranges (as opposed to only the file-level information), we cannot use any of these modes, and as a result specifying -z
here has no effect.
const results = new Array<[string, number, number]>(); | ||
|
||
let changedFile = ""; | ||
for (const line of diffHunkHeaders) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general this seems to keep going when it sees certain kinds of unexpected items in the diff. It seems that there's a risk of missing changed files — should we be more robust here, or fail more eagerly, or is that not important for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you tell me more about the risk you see, and how the code can be made more robust?
@@ -161,6 +162,147 @@ export const determineBaseBranchHeadCommitOid = async function ( | |||
} | |||
}; | |||
|
|||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: same comment about considering moving this bunch of self-contained functionality to a new file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see response to your earlier mention of the same suggestion.
d757098
to
38487cd
Compare
Thanks for the detailed comments and helpful suggestions! PTAL. |
Merge / deployment checklist