Skip to content

Conversation

@anmarchenko
Copy link
Member

What does this PR do?

  • Replaced proactive global git config modification with per-command -c safe.directory=<path> flag
  • Added getSafeDirectoryConfig() function that caches the repository root path (via sync.Once)
  • Updated execGit() and execGitStringWithInput() to automatically include the safe.directory config
  • Removed the code in getLocalGitData() that was adding entries to global git config

Motivation

The previous approach modified the global git config by running:

git config --global --add safe.directory <path>

This caused several problems:

  1. Config pollution: Using --add creates duplicate entries on repeated runs
  2. Security: Modifying global config affects all repositories, not just the current one
  3. Persistence: Changes remain after the tool finishes running

Instead of modifying global config, we now pass safe.directory as a command-line config override:

git -c safe.directory=/path/to/repo <command>

Benefits:

  • No modification to any config files (global, local, or system)
  • Only affects the single command execution
  • Cannot cause config pollution
  • More secure (isolated to single invocation)
  • Repository root is cached via sync.Once for performance

Port of DataDog/ddtest#29

Reviewer's Checklist

  • Changed code has unit tests for its functionality at or near 100% coverage.
  • System-Tests covering this feature have been added and enabled with the va.b.c-dev version tag.
  • There is a benchmark for any new code, or changes to existing code.
  • If this interacts with the agent in a new way, a system test has been added.
  • New code is free of linting errors. You can check this by running ./scripts/lint.sh locally.
  • Add an appropriate team label so this PR gets put in the right place for the release notes.
  • Non-trivial go.mod changes, e.g. adding new modules, are reviewed by @DataDog/dd-trace-go-guild.

Unsure? Have a question? Request a review!

@anmarchenko anmarchenko requested a review from a team as a code owner January 13, 2026 11:13
@anmarchenko anmarchenko changed the title fix(civisibility): Use -c flag for git safe.directory instead of modifying global config [SDTEST-3227] fix(civisibility): Use -c flag for git safe.directory instead of modifying global config Jan 13, 2026
@anmarchenko anmarchenko changed the title [SDTEST-3227] fix(civisibility): Use -c flag for git safe.directory instead of modifying global config fix(civisibility): Use -c flag for git safe.directory instead of modifying global config Jan 13, 2026
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 496930f949

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@codecov
Copy link

codecov bot commented Jan 13, 2026

Codecov Report

❌ Patch coverage is 60.00000% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 54.87%. Comparing base (27a40d0) to head (5d88e29).
⚠️ Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
internal/civisibility/utils/git.go 60.00% 5 Missing and 3 partials ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
internal/civisibility/utils/git.go 58.23% <60.00%> (+0.09%) ⬆️

... and 9 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@pr-commenter
Copy link

pr-commenter bot commented Jan 13, 2026

Benchmarks

⚠️ Warning: Unable to generate benchmark comparison - missing baseline or candidate data. Check the pipeline logs for details.

@tonyredondo
Copy link
Member

/merge

@gh-worker-devflow-routing-ef8351
Copy link

gh-worker-devflow-routing-ef8351 bot commented Jan 13, 2026

View all feedbacks in Devflow UI.

2026-01-13 11:53:49 UTC ℹ️ Start processing command /merge


2026-01-13 11:53:53 UTC ℹ️ MergeQueue: pull request added to the queue

The expected merge time in main is approximately 33m (p90).


2026-01-13 11:56:26 UTC ℹ️ MergeQueue: Readding this merge request to the queue because another merge request processed with yours failed. No action is needed from your side.


2026-01-13 12:03:00 UTC ⚠️ MergeQueue: This merge request build was cancelled

dario.castane@datadoghq.com cancelled this merge request build

@darccio
Copy link
Member

darccio commented Jan 13, 2026

/remove

@gh-worker-devflow-routing-ef8351
Copy link

gh-worker-devflow-routing-ef8351 bot commented Jan 13, 2026

View all feedbacks in Devflow UI.

2026-01-13 12:02:52 UTC ℹ️ Start processing command /remove


2026-01-13 12:02:56 UTC ℹ️ Devflow: /remove

@darccio
Copy link
Member

darccio commented Jan 13, 2026

/merge

@gh-worker-devflow-routing-ef8351
Copy link

gh-worker-devflow-routing-ef8351 bot commented Jan 13, 2026

View all feedbacks in Devflow UI.

2026-01-13 12:04:55 UTC ℹ️ Start processing command /merge


2026-01-13 12:05:03 UTC ℹ️ MergeQueue: waiting for PR to be ready

This pull request is not mergeable according to GitHub. Common reasons include pending required checks, missing approvals, or merge conflicts — but it could also be blocked by other repository rules or settings.
It will be added to the queue as soon as checks pass and/or get approvals. View in MergeQueue UI.
Note: if you pushed new commits since the last approval, you may need additional approval.
You can remove it from the waiting list with /remove command.


2026-01-13 12:30:38 UTC ℹ️ MergeQueue: merge request added to the queue

The expected merge time in main is approximately 33m (p90).


2026-01-13 12:57:54 UTCMergeQueue: The checks failed on this merge request

Tests failed on this commit a6cf2d0:

What to do next?

  • Investigate the failures and when ready, re-add your pull request to the queue!
  • If your PR checks are green, try to rebase/merge. It might be because the CI run is a bit old.
  • Any question, go check the FAQ.

@darccio
Copy link
Member

darccio commented Jan 14, 2026

/merge

@gh-worker-devflow-routing-ef8351
Copy link

gh-worker-devflow-routing-ef8351 bot commented Jan 14, 2026

View all feedbacks in Devflow UI.

2026-01-14 11:15:03 UTC ℹ️ Start processing command /merge


2026-01-14 11:15:07 UTC ℹ️ MergeQueue: pull request added to the queue

The expected merge time in main is approximately 33m (p90).


2026-01-14 11:37:51 UTC ℹ️ MergeQueue: Readding this merge request to the queue because another merge request processed with yours failed. No action is needed from your side.


2026-01-14 12:01:52 UTCMergeQueue: The checks failed on this merge request

Tests failed on this commit f225a88:

What to do next?

  • Investigate the failures and when ready, re-add your pull request to the queue!
  • If your PR checks are green, try to rebase/merge. It might be because the CI run is a bit old.
  • Any question, go check the FAQ.

@darccio
Copy link
Member

darccio commented Jan 14, 2026

/merge

@gh-worker-devflow-routing-ef8351
Copy link

gh-worker-devflow-routing-ef8351 bot commented Jan 14, 2026

View all feedbacks in Devflow UI.

2026-01-14 12:11:31 UTC ℹ️ Start processing command /merge


2026-01-14 12:11:36 UTC ℹ️ MergeQueue: pull request added to the queue

The expected merge time in main is approximately 33m (p90).


2026-01-14 12:37:59 UTC ℹ️ MergeQueue: This merge request was merged

@dd-mergequeue dd-mergequeue bot merged commit e83252c into main Jan 14, 2026
186 checks passed
@dd-mergequeue dd-mergequeue bot deleted the anmarchenko/fix_safe_directory_issue branch January 14, 2026 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants