fix: improve git sync #17
Conversation
WalkthroughAdds a CommandRunner abstraction and mock, centralizes git URL normalization, switches manager to use NormalizeURL, and refactors source updates to run concurrently with a semaphore-limited, fail-fast cancellable workflow. Also adds mockery config, a Makefile mocks target, and minor .gitignore/go.mod edits. Changes
Sequence DiagramsequenceDiagram
participant Client as Caller
participant Controller as updateSources()
participant Semaphore as Semaphore (max 4)
participant Worker as Worker Goroutine
participant GitSvc as Git Client
participant Runner as CommandRunner
participant ErrCh as Error Channel
Client->>Controller: call UpdateSources(ctx)
activate Controller
Controller->>Semaphore: create (limit=4)
Controller->>Controller: create cancellable ctx, errCh
loop for each source
Controller->>Semaphore: acquire slot
Controller->>Worker: spawn goroutine
activate Worker
Worker->>Worker: set status "Pending"
Worker->>Worker: set status "Syncing"
Worker->>GitSvc: New per-source git client (uses Runner)
activate GitSvc
GitSvc->>Runner: Run / RunWithTTY git commands
alt command success
Runner-->>GitSvc: stdout, nil
GitSvc-->>Worker: nil
Worker->>Worker: set status "Synced"
else command failure
Runner-->>GitSvc: "", error
GitSvc-->>Worker: wrapped error
Worker->>ErrCh: send error (non-blocking)
Worker->>Controller: call cancel()
Worker->>Worker: set status "Failed"
end
deactivate GitSvc
Worker->>Semaphore: release slot
deactivate Worker
end
Controller->>Controller: wait for workers
Controller->>ErrCh: collect first error (if any)
alt error present
Controller-->>Client: return first error
else
Controller-->>Client: return nil
end
deactivate Controller
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.suminternal/git/git_test.gois excluded by!**/*_test.go
📒 Files selected for processing (10)
.gitignore.mockery.yamlMakefilecmd/devbox/update.gogo.modinternal/git/exec.gointernal/git/git.gointernal/git/mock_CommandRunner.gointernal/git/normalizer.gointernal/manager/manager.go
🧰 Additional context used
📓 Path-based instructions (3)
internal/**/*.go
⚙️ CodeRabbit configuration file
internal/**/*.go: Core packages providing project functionality:
- project/: Project configuration, Docker Compose extensions (x-devbox-*)
- manager/: Project/service autodetection from current directory
- git/: Git operations (clone, sparse checkout, sync)
- cert/: SSL certificate generation
- hosts/: /etc/hosts management with project-scoped markers
- table/: CLI table output formatting
Review for:
- Clean interfaces and proper encapsulation
- Error wrapping with context
- No interface{} - use 'any' instead (enforced by linter)
- YAGNI principle - no speculative features or premature abstractions
- Security considerations for host file and certificate operations
Files:
internal/manager/manager.gointernal/git/normalizer.gointernal/git/exec.gointernal/git/mock_CommandRunner.gointernal/git/git.go
internal/manager/**/*.go
⚙️ CodeRabbit configuration file
internal/manager/**/*.go: Project autodetection with three-step process:
- Check if directory is a local mount of any project
- Match Git remote URL + path against project source definitions
- Check if directory is the project's manifest repository
Review for edge cases in path matching and Git remote detection.
Files:
internal/manager/manager.go
cmd/devbox/**/*.go
⚙️ CodeRabbit configuration file
cmd/devbox/**/*.go: This directory contains CLI commands using Cobra framework.
Each command is in a separate file with an init() function that registers via root.AddCommand().Review for:
- Proper error handling with context wrapping (fmt.Errorf with %w)
- Consistent command structure following existing patterns
- Appropriate use of runWrapper for context handling
- Clear, concise command descriptions
- No over-engineering - only implement what's needed
Files:
cmd/devbox/update.go
🧬 Code graph analysis (3)
internal/manager/manager.go (1)
internal/git/normalizer.go (1)
NormalizeURL(21-60)
cmd/devbox/update.go (3)
internal/project/config.go (1)
SourceConfig(4-9)internal/app/const.go (1)
SourcesDir(13-13)internal/git/git.go (1)
New(16-21)
internal/git/git.go (1)
internal/git/exec.go (1)
CommandRunner(10-13)
🪛 checkmake (0.2.2)
Makefile
[warning] 1-1: Missing required phony target "clean"
(minphony)
⏰ 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: Lint & Test
- GitHub Check: E2E Tests
🔇 Additional comments (9)
.gitignore (1)
12-12: LGTM.Adding
coverage.outto.gitignoreis idiomatic for Go projects—coverage artifacts shouldn't be tracked in version control. The change aligns with the existing pattern of excluding build artifacts.Makefile (1)
1-6: LGTM!The
mockstarget is correctly added to support mock generation via mockery, and the.PHONYdeclaration is properly updated..mockery.yaml (1)
1-13: LGTM!The mockery configuration is well-structured for generating in-package mocks with the expecter pattern enabled, which aligns with modern testing practices.
internal/manager/manager.go (1)
84-265: Excellent refactoring to centralize URL normalization.Replacing the local
normalizeRemoteURLfunction withgit.NormalizeURLacross all call sites (lines 84, 107, 155, 230, 265) follows the DRY principle and improves maintainability. The centralized implementation in the git package provides consistent URL handling across the codebase.internal/git/git.go (2)
13-20: LGTM - Clean abstraction for testability.The introduction of the
CommandRunnerfield and its initialization withdefaultRunnerprovides good separation of concerns and enables testing through dependency injection.
169-190: Helpful git configuration hints for common authentication issues.The
gitConfigHintfunction provides contextual guidance for SSH vs HTTPS configuration issues, which improves the developer experience when troubleshooting authentication failures.cmd/devbox/update.go (2)
124-183: Well-implemented concurrent updates with proper fail-fast behavior.The semaphore-based concurrency control (max 4 concurrent syncs) combined with cancellable context provides good resource management and fail-fast error handling. Key strengths:
- Semaphore acquisition properly checks
ctx.Done()to avoid blocking when cancelledcancelSync()on first error prevents wasted work on remaining sources- Error collection loop waits for all goroutines to prevent leaks
- Per-source git client creation correctly isolates operations
The implementation correctly handles the edge case where goroutines are waiting for the semaphore when cancellation occurs.
144-167: Status progression provides clear feedback.The status text flow from "Pending" → "Syncing" → "Synced"/"Failed" with appropriate progress status (Done/Error) gives users clear visibility into the update process.
internal/git/mock_CommandRunner.go (1)
1-180: Generated mock implementation looks correct.This auto-generated mock properly implements the CommandRunner interface with both
RunandRunWithTTYmethods. The variadic argument handling, type assertions, and expecter pattern all follow standard mockery conventions. The constructor correctly integrates withtesting.Tcleanup for automatic expectation verification.Note: As stated in line 1, this file should not be manually edited—regenerate via mockery if the CommandRunner interface changes.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
internal/git/normalizer.go (1)
19-60: Comprehensive URL normalization with minor redundancy.The implementation correctly handles various git URL formats including SSH, HTTPS, Azure DevOps variants, and git:// protocol. The normalization logic is sound and the fallback provides reasonable behavior for edge cases.
Note: As mentioned in a previous review, the
TrimSuffix(matches[3], ".git")calls are redundant since the regex patterns already handle the optional.gitsuffix with(?:\.git)?$. This is cosmetic and doesn't affect correctness.internal/git/exec.go (1)
24-29: Correct TTY handling with combined output capture.The
RunWithTTYimplementation properly connectsStdinfor interactive operations and usesCombinedOutput()to capture both stdout and stderr, addressing the diagnostic concerns from previous review feedback.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.suminternal/git/git_test.gois excluded by!**/*_test.go
📒 Files selected for processing (10)
.gitignore.mockery.yamlMakefilecmd/devbox/update.gogo.modinternal/git/exec.gointernal/git/git.gointernal/git/mock_CommandRunner.gointernal/git/normalizer.gointernal/manager/manager.go
🧰 Additional context used
📓 Path-based instructions (3)
internal/**/*.go
⚙️ CodeRabbit configuration file
internal/**/*.go: Core packages providing project functionality:
- project/: Project configuration, Docker Compose extensions (x-devbox-*)
- manager/: Project/service autodetection from current directory
- git/: Git operations (clone, sparse checkout, sync)
- cert/: SSL certificate generation
- hosts/: /etc/hosts management with project-scoped markers
- table/: CLI table output formatting
Review for:
- Clean interfaces and proper encapsulation
- Error wrapping with context
- No interface{} - use 'any' instead (enforced by linter)
- YAGNI principle - no speculative features or premature abstractions
- Security considerations for host file and certificate operations
Files:
internal/manager/manager.gointernal/git/normalizer.gointernal/git/git.gointernal/git/exec.gointernal/git/mock_CommandRunner.go
internal/manager/**/*.go
⚙️ CodeRabbit configuration file
internal/manager/**/*.go: Project autodetection with three-step process:
- Check if directory is a local mount of any project
- Match Git remote URL + path against project source definitions
- Check if directory is the project's manifest repository
Review for edge cases in path matching and Git remote detection.
Files:
internal/manager/manager.go
cmd/devbox/**/*.go
⚙️ CodeRabbit configuration file
cmd/devbox/**/*.go: This directory contains CLI commands using Cobra framework.
Each command is in a separate file with an init() function that registers via root.AddCommand().Review for:
- Proper error handling with context wrapping (fmt.Errorf with %w)
- Consistent command structure following existing patterns
- Appropriate use of runWrapper for context handling
- Clear, concise command descriptions
- No over-engineering - only implement what's needed
Files:
cmd/devbox/update.go
🧬 Code graph analysis (4)
internal/manager/manager.go (1)
internal/git/normalizer.go (1)
NormalizeURL(21-60)
internal/git/normalizer.go (1)
internal/project/config.go (1)
URL(4-9)
cmd/devbox/update.go (3)
internal/project/config.go (1)
SourceConfig(4-9)internal/app/const.go (1)
SourcesDir(13-13)internal/git/git.go (1)
New(16-21)
internal/git/git.go (1)
internal/git/exec.go (1)
CommandRunner(10-13)
🪛 checkmake (0.2.2)
Makefile
[warning] 1-1: Missing required phony target "clean"
(minphony)
🔇 Additional comments (15)
.gitignore (1)
12-12: LGTM - Standard Go coverage artifact.Adding
coverage.outto.gitignoreis standard practice for Go projects to prevent coverage artifacts from being committed.go.mod (1)
140-140: LGTM - Expected transitive dependency.The
objxpackage is a dependency oftestify/mockand is appropriately marked as indirect. This aligns with the new mock generation infrastructure introduced in this PR.Makefile (1)
1-6: LGTM - Clean mock generation integration.The
mockstarget properly integrates with the.mockery.yamlconfiguration to generate test mocks. The.PHONYdeclaration is correct..mockery.yaml (1)
1-13: LGTM - Well-configured mock generation setup.The mockery configuration properly targets the
CommandRunnerinterface with sensible defaults. Theinpackage: Truesetting will place mocks alongside the implementation, which is appropriate for internal packages.internal/manager/manager.go (1)
107-107: Consistent application of centralized normalization.All remote URL comparisons now correctly use
git.NormalizeURL(), ensuring consistent behavior across autodetection and source matching logic.Also applies to: 155-155, 230-230, 265-265
internal/git/exec.go (2)
9-13: Clean abstraction for command execution.The
CommandRunnerinterface provides a testable abstraction for git command execution, enabling mock-based testing. The separation betweenRunandRunWithTTYappropriately handles interactive vs. non-interactive scenarios.
17-22: Proper non-interactive execution setup.Setting
GIT_TERMINAL_PROMPT=0prevents git from prompting for credentials in non-interactive contexts, which aligns with the PR objective of fixing TTY-related issues. UsingCombinedOutput()correctly captures both stdout and stderr for diagnostics.internal/git/git.go (4)
13-13: Clean integration of CommandRunner abstraction.Adding the
runnerfield and initializing it with&defaultRunner{}properly integrates the new abstraction while maintaining backward compatibility. This enables testability without changing the public API.Also applies to: 19-19
29-32: Enhanced error diagnostics with helpful hints.Using
RunWithTTYfor clone operations and addinggitConfigHint(url)to error messages provides users with actionable guidance when authentication fails. This directly addresses the PR objective of improving git operations.
56-59: Important safety check documented.The comment clarifies the critical safety check: without a
.gitdirectory,git resetwould traverse up and potentially lock a parent repository. This defensive approach prevents unintended side effects.
79-87: Proper sparse checkout configuration.The sparse checkout initialization and configuration correctly uses the runner interface. The
appendpattern for variadic arguments is handled properly.cmd/devbox/update.go (3)
109-111: LGTM on timeout and context setup.30-minute timeout is reasonable for git operations. Context handling with
defer cancel()ensures proper cleanup.
149-170: Per-source git client instantiation is correct.Creating a new
git.New(repoDir)per source aligns with the runner-based architecture. The error handling and progress event updates are clean.One minor observation: the error path sets
Status: progress.Errorwhile success setsStatus: progress.Done, which is correct. TheStatusTextvalues ("Pending", "Syncing", "Synced", "Failed") provide good user feedback.
173-184: Error aggregation is correct.Collecting all errors and returning the first non-nil is the right approach for fail-fast. The buffered channel prevents goroutine leaks.
internal/git/mock_CommandRunner.go (1)
1-180: Auto-generated mock file - no review needed.This file is generated by mockery v2.52.2 as indicated in the header comment. The generated code follows standard testify/mock patterns and should not be manually modified. Ensure this file is regenerated when the
CommandRunnerinterface changes (viamake mocksor equivalent).
This PR improves the way of detecting source properly and fixes the issue when TTY is needed fot git operations
Summary by CodeRabbit
Bug Fixes
New Features
✏️ Tip: You can customize this high-level summary in your review settings.