Skip to content

getLegacyGitAccessor(): Don't ignore errors#425

Merged
edolstra merged 1 commit intomainfrom
eelcodolstra/nix-377
Apr 16, 2026
Merged

getLegacyGitAccessor(): Don't ignore errors#425
edolstra merged 1 commit intomainfrom
eelcodolstra/nix-377

Conversation

@edolstra
Copy link
Copy Markdown
Collaborator

@edolstra edolstra commented Apr 15, 2026

Motivation

Fix a regression introduced in fbf176d that caused getLegacyGitAccessor() to ignore errors from Git. This could lead to empty Git exports in the Nix store.

Context

Summary by CodeRabbit

Bug Fixes

  • Enhanced output clarity for git submodule operations. When fetching git repositories with submodules enabled, the system now executes all necessary git operations without producing unnecessary console output. This results in significantly cleaner build logs and makes it easier to identify actual issues or important information during the fetch process.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 15, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: b5b43d65-8605-428b-999a-e16980b40381

📥 Commits

Reviewing files that changed from the base of the PR and between b5c0faf and 7a0aade.

📒 Files selected for processing (1)
  • src/libfetchers/git.cc

📝 Walkthrough

Walkthrough

The change replaces multiple runProgram() calls with runProgram2() in the legacy submodule-fetch path of the Git input scheme, routing stdout from git operations (init, remote add, fetch, checkout) into a StringSink to prevent output pollution while maintaining identical command execution and sequencing.

Changes

Cohort / File(s) Summary
Git Legacy Submodule Output Handling
src/libfetchers/git.cc
Replaced runProgram() invocations with runProgram2() calls in the getLegacyGitAccessor method for submodule fetches, redirecting stdout to StringSink for git init, remote add, fetch, and checkout commands to prevent stdout pollution.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 A rabbit's tale of cleaner streams,
Where git commands flow in silent beams,
No more pollution clogs the way,
StringSinks catch what words would say,
Output hidden, workflows clean,
The neatest submodule fix you've seen!

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change in the changeset—replacing runProgram() calls with runProgram2() to capture stdout, thereby preventing error suppression in the legacy Git accessor function.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch eelcodolstra/nix-377

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown

@github-actions github-actions bot temporarily deployed to pull request April 15, 2026 20:53 Inactive
@cole-h
Copy link
Copy Markdown
Member

cole-h commented Apr 16, 2026

Is there an easy way to add a regression test for this?

@edolstra edolstra added this pull request to the merge queue Apr 16, 2026
Merged via the queue into main with commit 6e8f3ff Apr 16, 2026
31 checks passed
@edolstra edolstra deleted the eelcodolstra/nix-377 branch April 16, 2026 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants