Skip to content

Conversation

@edolstra
Copy link
Collaborator

@edolstra edolstra commented Nov 4, 2025

Motivation

This fixes Nix appearing to hang while fetching a Git repo (with no indication that it's doing that).

We were calling git with --quiet in order not to mess up Nix's progress bar. However, runProgram() already suspends the progress bar (since git may be interactive) so that's no longer an issue. So we can just run with --progress instead.

Also, when pausing the progress bar, print current activities. Otherwise the user won't see messages like "fetching Git repository" from the Git fetcher.

Context

Summary by CodeRabbit

  • Improvements
    • Git operations now display real-time progress feedback instead of operating silently
    • Activity logging enhanced to show ongoing operations during pause-resume cycles
    • Simplified error messages for Git-related failures

We were calling git with `--quiet` in order not to mess up Nix's
progress bar. However, `runProgram()` already suspends the progress
bar (since git may be interactive) so that's no longer an issue. So we
can just run with `--progress` instead.
Otherwise the user won't see messages like "fetching Git repository"
from the Git fetcher.
@coderabbitai
Copy link

coderabbitai bot commented Nov 4, 2025

Walkthrough

Two files modified: git-utils.cc refactors Git command invocation to use --progress with status code-based error handling instead of capturing output with --quiet, while progress-bar.cc introduces per-activity logging state and a logActivity() helper to consolidate activity message output during pause and startActivity flows.

Changes

Cohort / File(s) Summary
Git fetcher output simplification
src/libfetchers/git-utils.cc
Switch Git invocation from --quiet with output capture to --progress relying on status code. Remove output string capturing from runProgram usage. Simplify error handling to throw with URL-only message on non-zero status.
Progress bar activity logging consolidation
src/libmain/progress-bar.cc
Add logged bool to ActInfo for per-activity logged state tracking. Introduce logActivity() helper to log activity messages once at appropriate times. Modify pause() to clear line and log all current activities. Update startActivity() to call logActivity() for initial message emission.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • git-utils.cc: Verify that switching from output capture to status code alone does not lose diagnostic information; confirm error message format is acceptable for users
  • progress-bar.cc: Trace through the pause-resume flow to ensure logActivity() is called correctly in all paths; verify that the new logged state prevents duplicate output and doesn't interfere with existing logging patterns

Poem

🐰 Quiet git commands now proudly speak,
with progress bars so sleek!
Activities logged just once—no more repeats,
while pausing reveals what the code completes.

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title 'Git fetcher: Restore progress indication' directly and clearly describes the main change: modifying git command invocation to restore visible progress during git operations.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch git-progress

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0fe46d2 and e532a6a.

📒 Files selected for processing (2)
  • src/libfetchers/git-utils.cc (1 hunks)
  • src/libmain/progress-bar.cc (4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/libmain/progress-bar.cc (2)
src/libstore/daemon.cc (9)
  • state (111-122)
  • act (143-160)
  • act (143-149)
  • act (162-169)
  • act (162-162)
  • act (171-178)
  • act (171-171)
  • lvl (86-94)
  • lvl (86-86)
src/libutil/logging.cc (16)
  • writeToStderr (153-163)
  • writeToStderr (153-153)
  • act (127-137)
  • act (127-133)
  • act (139-148)
  • act (139-139)
  • act (303-320)
  • act (303-309)
  • act (322-328)
  • act (322-322)
  • act (330-338)
  • act (330-330)
  • lvl (81-117)
  • lvl (81-81)
  • lvl (267-274)
  • lvl (267-267)
src/libfetchers/git-utils.cc (2)
src/libutil/include/nix/util/util.hh (2)
  • append (290-293)
  • append (290-290)
src/libfetchers/include/nix/fetchers/git-utils.hh (2)
  • url (87-87)
  • url (101-101)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: build_x86_64-linux / build
  • GitHub Check: build_aarch64-darwin / build
🔇 Additional comments (5)
src/libfetchers/git-utils.cc (1)

595-603: LGTM! Progress indication successfully restored.

The switch from --quiet to --progress combined with removing output capture allows Git's progress to display directly to the user. The isInteractive = true flag ensures the progress bar is suspended during the git command, preventing interference.

One minor observation: The error message on line 603 no longer includes Git's output, making it less informative than before. However, since Git's stderr is now written directly to the terminal (not captured), users will still see the actual error details from Git before this exception is thrown.

src/libmain/progress-bar.cc (4)

54-54: LGTM! Proper logging state tracking.

The logged flag prevents duplicate activity messages from being emitted, which is essential for the new logging behavior where activities are logged both on start and during pause.


206-212: LGTM! Well-designed activity logging helper.

The logActivity() helper properly consolidates the logic for emitting activity messages once. The exclusion of actBuildWaiting is appropriate since waiting activities don't represent active work worth surfacing to users. The logged flag ensures each activity message is only emitted once.


146-153: Excellent! This solves the core issue.

Logging activities during pause ensures users see messages like "fetching Git repository..." instead of Nix appearing to hang. The implementation correctly:

  • Only logs when state->active is true (TTY mode)
  • Clears the progress bar line before logging activities
  • Uses the logActivity() helper which respects the logged flag to prevent duplicates

This addresses the main PR objective of preventing Nix from appearing to hang during Git fetches.


230-230: LGTM! Proper initial activity logging.

Calling logActivity() immediately after creating the ActInfo ensures the activity message is emitted early (if appropriate), maintaining the expected behavior where users see activity start messages. The logActivity() helper's checks ensure this only happens when appropriate.


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

@github-actions
Copy link

github-actions bot commented Nov 4, 2025

@github-actions github-actions bot temporarily deployed to pull request November 4, 2025 19:30 Inactive
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