FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix: reduce redundant git-call-failed log messages in upload-sarif by mvanhorn · Pull Request #3959 · github/codeql-action · GitHub

/ codeql-action Public

fix: reduce redundant git-call-failed log messages in upload-sarif - #3959

Open
mvanhorn wants to merge 1 commit into
github:mainfrom
mvanhorn:fix/3383-git-call-failed-log-dedup
ConversationCommits (1)ChecksFiles changed
Open

fix: reduce redundant git-call-failed log messages in upload-sarif#3959
mvanhorn wants to merge 1 commit into
github:mainfrom
mvanhorn:fix/3383-git-call-failed-log-dedup

Conversation

mvanhorn commented Jun 13, 2026

Copy link
Copy Markdown

When upload-sarif runs in a directory that is not a git repository, runGitCommand was calling core.info on every git call failure. Since callers such as getCommitOid and determineBaseBranchHeadCommitOid silently swallow the exception and fall back to environment variables, the same "not a git repository" diagnostic was emitted multiple times per run, adding noise without adding value.

This PR changes the log level in runGitCommand from core.info to core.debug so the full diagnostic is preserved for maintainers with debug logging enabled, but does not appear in normal output. A single core.info fallback notice is added in getCommitOid so callers still see exactly one actionable message when git is unavailable.

Fixes #3383.

Risk assessment

  • Low risk: Changes are test-covered, only affect log output levels, and have no impact on action behavior or correctness.

Which use cases does this change impact?

  • Third-party analyses - The changes affect the upload-sarif action.

How did/will you validate this change?

  • Unit tests - I am depending on unit test coverage (i.e. tests in .test.ts files). Existing determineBaseBranchHeadCommitOid tests have been updated to assert core.debug is called instead of core.info. All 30 tests in src/git-utils.test.ts pass.

If something goes wrong after this change is released, what are the mitigation and rollback strategies?

  • Rollback - Change can only be disabled by rolling back the release or releasing a new version with a fix.
  • Development/testing only - This change cannot cause any failures in production.

How will you know if something goes wrong after this change is released?

  • Other - Log level changes are observable; if users report missing diagnostics, debug logging can be enabled.

Are there any special considerations for merging or releasing this change?

  • No special considerations - This change can be merged at any time.

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Consider adding a changelog entry for this change.
  • Confirm the readme and docs have been updated if necessary.

fix: reduce redundant git-call-failed log messages in upload-sarif
f68f8d1 
Change the log level of the "git call failed." message in `runGitCommand`
from `core.info` to `core.debug` so repeated identical failures in
non-git working directories no longer flood the normal output. Add a
single `core.info` fallback notice in `getCommitOid` so callers still
see one actionable message when git is unavailable. Update tests to
assert the new debug-level logging.

Fixes github#3383
mvanhorn requested a review from a team as a code owner June 13, 2026 07:15
mbg reviewed Jun 15, 2026
View reviewed changes

mbg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Hi @mvanhorn 👋🏻

Thanks for opening this PR! I think this is probably one of those innocent-seeming changes that's unfortunately a bit of a rabbit hole.

Typically, we expect that the CodeQL Action is run with reference to some Git repository. In the general case, we therefore want to know when that is not the case and git commands fail unexpectedly. The change here mostly just masks these unexpected failures, since runGitCommand is used by several callers and the change shifts the responsibility of communicating the failure from runGitCommand to the caller. See e.g. the comment in getGitRoot which notes that // Errors are already logged by runGitCommand() which would no longer be true with this change.

I agree with the comment in the issue that:

Ideally, the action should be able to "quickly" determine that there isn't a repository checked out and not waste additional time running git commands.

This seems feasible and we could then emit one warning about that and skip subsequent git calls. That's a more involved change than this one though.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mbg mbg left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

upload-sarif: git call failed. more times than necessary for a working directory without a git repository

2 participants

Back | FazBrowse Home | New Git URL