GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — fix: reduce redundant git-call-failed log messages in upload-sarif by mvanhorn · Pull Request #3959 · github/codeql-action

Conversation

@mvanhorn

@mvanhorn 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mvanhorn
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
mvanhorn requested a review from a team as a code owner June 13, 2026 07:15
mbg
mbg reviewed Jun 15, 2026
View reviewed changes

@mbg mbg left a comment

Copy link
Copy Markdown
Member

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mvanhorn

mvanhorn commented Sep 21, 2026

Copy link
Copy Markdown
Author

@mbg thanks, that is a fair read and I think you are right.

runGitCommand has seven callers in src/git-utils.ts: getGitVersionOrThrow, getCommitOid, determineBaseBranchHeadCommitOid, getGitRoot, getFileOidsUnderPath, listFiles and getGeneratedFiles. My diff demotes the shared line to debug for all seven and only adds a replacement info back in getCommitOid, so the other six lose their user-visible failure message with nothing taking over. And the comment you pointed at is exactly the tripwire: // Errors are already logged by runGitCommand() in getGitRoot stops being true the moment that line moves to debug. That is masking rather than deduplicating, and I should have caught it.

On the direction you sketched, detecting once that there is no repository checked out, warning once, and skipping the later git calls: that is what I actually wanted, and it treats the cause instead of the symptom. The not a git repository condition is already special-cased inside runGitCommand, so the signal is there to hang it on.

Two things I would rather have your call on before writing it:

  1. Scope. Do you want it in this PR, or should I close this one and open a fresh PR against upload-sarif: git call failed. more times than necessary for a working directory without a git repository #3383 so the history reads cleanly?
  2. Placement. Should the check live inside runGitCommand, so all seven callers inherit it, or one level up in the callers? Inside is less code, but it means runGitCommand grows a piece of memoized state, and I would rather not introduce that unless you want it there.

Either is fine by me. And if you would rather keep #3383 for someone on the team, say so and I will close this one without any fuss.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mvanhorn

mvanhorn commented Sep 25, 2026

Copy link
Copy Markdown
Author

Thanks @mbg, that makes sense. Masking the failure in runGitCommand pushes the reporting onto every caller, so I'll close this rather than take it down that path.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mvanhorn mvanhorn closed this Sep 25, 2026
@mbg

mbg commented Sep 25, 2026

Copy link
Copy Markdown
Member

Hi @mvanhorn,

Thanks for the follow-up in #3959 (comment). I meant to reply to that, but haven't had the chance yet. I happen to have been working on semi-related parts of the code recently (see #4148). That (in essence) completes the initial validation that there is a repo to work with. I was considering looking into working on suppressing further messages about the lack of a repo as a follow-up to that work.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mvanhorn

mvanhorn commented Sep 29, 2026

Copy link
Copy Markdown
Author

@mbg sounds good, #4148 is the right place to hang it. Thanks for taking the follow-up.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mbg mbg mbg left review comments

Uh oh!

There was an error while loading. Please reload this page.

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

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@mvanhorn @mbg
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.