feat: add USB fixture helpers for backup-mode testing#3
Conversation
Harden snapshot restore path handling, simplify git output parsing with stdlib helpers, add CI checks, and expand README usage/requirements to improve maintainability and consumer confidence. Made-with: Cursor
Improve README onboarding for test usage and add a developer guide covering testing, contribution expectations, and maintainer release workflow. Made-with: Cursor
Parse `git remote -v` output without truncating whitespace-containing URLs and add a regression test to lock in behavior for local remotes with spaced paths. Made-with: Cursor
Provide reusable helpers for creating .git-fire volume roots, reading/writing marker config, asserting bare/non-bare git destinations, and file URL conversion.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDocumentation updated with active SSH clone instructions and expanded project structure. Remote suffix parsing logic refined to conditionally strip role markers instead of unconditionally trimming sequential suffixes. USB fixture validation enhanced with stricter filesystem type checks. Tests added to verify special character handling in remote paths. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (3)
usb_fixtures_test.go (1)
23-40: Missing assertion forCreatedAtround-trip.The test verifies
SchemaVersion,LayoutDir, andStrategybut omits validation ofCreatedAt. SinceWriteUSBVolumeConfigsets a default timestamp andReadUSBVolumeConfigparses it, verifying this field would improve coverage.🧪 Proposed test enhancement
func TestReadWriteUSBVolumeConfig(t *testing.T) { root := t.TempDir() WriteUSBVolumeConfig(t, root, USBVolumeConfig{ SchemaVersion: 2, LayoutDir: "custom", Strategy: "git-clone", }) cfg := ReadUSBVolumeConfig(t, root) if cfg.SchemaVersion != 2 { t.Fatalf("schema mismatch: %d", cfg.SchemaVersion) } if cfg.LayoutDir != "custom" { t.Fatalf("layout mismatch: %s", cfg.LayoutDir) } if cfg.Strategy != "git-clone" { t.Fatalf("strategy mismatch: %s", cfg.Strategy) } + if cfg.CreatedAt.IsZero() { + t.Fatal("expected CreatedAt to be set") + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usb_fixtures_test.go` around lines 23 - 40, The test TestReadWriteUSBVolumeConfig is missing a check that CreatedAt round-trips through WriteUSBVolumeConfig and ReadUSBVolumeConfig; after reading cfg := ReadUSBVolumeConfig(t, root) add an assertion that cfg.CreatedAt is non-zero (or equals the value written if WriteUSBVolumeConfig accepts one) and/or that time.Since(cfg.CreatedAt) is reasonable (e.g., < a few seconds) to ensure the timestamp was written and parsed correctly; reference the CreatedAt field on the cfg returned by ReadUSBVolumeConfig and adjust the test assertion accordingly.usb_fixtures.go (1)
96-108: Consider logging parse failures for debugging.Lines 98 and 105 silently ignore parse errors for
schema_versionandcreated_at. While acceptable for test fixtures, logging these failures could help debug test issues.🔧 Optional: Add debug logging for parse failures
switch key { case "schema_version": - n, _ := strconv.Atoi(val) + n, err := strconv.Atoi(val) + if err != nil { + t.Logf("warning: invalid schema_version %q: %v", val, err) + } cfg.SchemaVersion = n case "layout_dir": cfg.LayoutDir = val case "strategy": cfg.Strategy = val case "created_at": - if ts, err := time.Parse(time.RFC3339, val); err == nil { + ts, err := time.Parse(time.RFC3339, val) + if err != nil { + t.Logf("warning: invalid created_at %q: %v", val, err) + } else { cfg.CreatedAt = ts } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usb_fixtures.go` around lines 96 - 108, The switch that parses fixture fields (the block handling "schema_version" and "created_at") currently ignores errors from strconv.Atoi and time.Parse; update the handlers in that switch so parse failures are logged (e.g., use log.Printf or the package's logger) with context including the key, val and the parse error; for "schema_version" ensure you check and log the error returned by strconv.Atoi before assigning to cfg.SchemaVersion, and for "created_at" log the error when time.Parse fails so failures are visible during test debugging.DEVELOPER_GUIDE.md (1)
45-49: Minor:gofmtcommand doesn't format subdirectories.The command
gofmt -w *.goonly formats Go files in the current directory, not subdirectories. Consider usinggo fmt ./...for consistency with the other commands.📝 Suggested fix
```bash -gofmt -w *.go +go fmt ./... go vet ./... go test ./...</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@DEVELOPER_GUIDE.mdaround lines 45 - 49, Replace the current formatting
commandgofmt -w *.gowith the recursive project-wide formatter by usinggo fmt ./...so Go files in subdirectories are formatted; update the
DEVELOPER_GUIDE.md snippet where the three commands are listed to usego fmt ./..., keeping thego vet ./...andgo test ./...lines unchanged.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@usb_fixtures.go:
- Around line 126-134: FileURLForPath produces non-compliant Windows file URLs
because the Path value doesn't start with a leading '/' (e.g., "C:/foo" yields
"file://C:/foo"); update FileURLForPath to normalize the absolute path via
filepath.ToSlash(abs) and ensure the resulting path string has a leading '/'
(prepend one when it doesn't) before assigning to url.URL.Path so
url.URL.String() emits the RFC 8089 form (file:///C:/...).
Nitpick comments:
In@DEVELOPER_GUIDE.md:
- Around line 45-49: Replace the current formatting command
gofmt -w *.gowith
the recursive project-wide formatter by usinggo fmt ./...so Go files in
subdirectories are formatted; update the DEVELOPER_GUIDE.md snippet where the
three commands are listed to usego fmt ./..., keeping thego vet ./...and
go test ./...lines unchanged.In
@usb_fixtures_test.go:
- Around line 23-40: The test TestReadWriteUSBVolumeConfig is missing a check
that CreatedAt round-trips through WriteUSBVolumeConfig and ReadUSBVolumeConfig;
after reading cfg := ReadUSBVolumeConfig(t, root) add an assertion that
cfg.CreatedAt is non-zero (or equals the value written if WriteUSBVolumeConfig
accepts one) and/or that time.Since(cfg.CreatedAt) is reasonable (e.g., < a few
seconds) to ensure the timestamp was written and parsed correctly; reference the
CreatedAt field on the cfg returned by ReadUSBVolumeConfig and adjust the test
assertion accordingly.In
@usb_fixtures.go:
- Around line 96-108: The switch that parses fixture fields (the block handling
"schema_version" and "created_at") currently ignores errors from strconv.Atoi
and time.Parse; update the handlers in that switch so parse failures are logged
(e.g., use log.Printf or the package's logger) with context including the key,
val and the parse error; for "schema_version" ensure you check and log the error
returned by strconv.Atoi before assigning to cfg.SchemaVersion, and for
"created_at" log the error when time.Parse fails so failures are visible during
test debugging.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `8f538181-bbe0-41a2-b8d8-1f85c6ef6674` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 39100e12962b80cf51af38cba82e10311f800c8b and 58fe7d0f4031fbfaa130df5e81c6606f5b620fe9. </details> <details> <summary>📒 Files selected for processing (9)</summary> * `.github/workflows/ci.yml` * `DEVELOPER_GUIDE.md` * `README.md` * `fixtures.go` * `fixtures_test.go` * `go.mod` * `snapshots.go` * `usb_fixtures.go` * `usb_fixtures_test.go` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Write/read config asymmetry with
%qescaping- ReadUSBVolumeConfig now uses strconv.Unquote to reverse %q escaping (with trim fallback), and a regression test verifies round-trip fidelity for backslashes and quotes.
Preview (428203380e)
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
new file mode 100644
--- /dev/null
+++ b/.github/workflows/ci.yml
@@ -1,0 +1,30 @@
+name: CI
+
+on:
+ push:
+ branches:
+ - main
+ pull_request:
+
+jobs:
+ test:
+ runs-on: ubuntu-latest
+ strategy:
+ fail-fast: false
+ matrix:
+ go-version: ["1.22.x", "stable"]
+
+ steps:
+ - name: Checkout
+ uses: actions/checkout@v4
+
+ - name: Setup Go
+ uses: actions/setup-go@v5
+ with:
+ go-version: ${{ matrix.go-version }}
+
+ - name: Run vet
+ run: go vet ./...
+
+ - name: Run tests
+ run: go test ./...
diff --git a/DEVELOPER_GUIDE.md b/DEVELOPER_GUIDE.md
new file mode 100644
--- /dev/null
+++ b/DEVELOPER_GUIDE.md
@@ -1,0 +1,127 @@
+# Developer Guide
+
+This guide covers how to use `git-testkit`, contribute safely, run checks locally, and maintain releases.
+
+## Audience
+
+- **Users** writing tests that need real git repositories.
+- **Contributors** making code/docs changes.
+- **Maintainers/admins** preparing releases and keeping CI healthy.
+
+## Local setup
+
+Requirements:
+
+- Go 1.22+
+- `git` on `PATH`
+
+Clone and verify:
+
+```bash
+git clone git@github.com:git-fire/git-testkit.git
+cd git-testkit
+go test ./...
+```
+
+## Project structure
+
+- `fixtures.go`: base repo creation and command helpers.
+- `scenarios.go`: fluent scenario builder and predefined multi-repo scenarios.
+- `snapshots.go`: snapshot/restore utilities for expensive test setup reuse.
+- `fixtures_test.go`: external package tests (`testutil_test`) that validate public API usage.
+- `scenarios_test.go`: package-internal tests for scenario/snapshot behavior.
+
+## Design principles
+
+- Prefer real git commands over mocks for behavior confidence.
+- Keep helper APIs composable and minimal.
+- Fail fast in setup helpers (`t.Fatalf`) so fixture errors are obvious.
+- Keep tests isolated using `t.TempDir()`.
+
+## Testing and quality checks
+
+Run before opening a PR:
+
+```bash
+gofmt -w *.go
+go vet ./...
+go test ./...
+```
+
+Optional:
+
+```bash
+go test -short ./...
+```
+
+CI mirrors the required checks (`go vet` and `go test`) on pull requests.
+
+## Usage guidance for library consumers
+
+- Prefer scenario builders for integration-style tests with remotes/worktrees.
+- Prefer base fixtures for small unit/integration tests that only need one repo.
+- Keep assertions close to setup; helper failures already include command output.
+- Use snapshots for expensive setups that are reused across subtests.
+
+Common usage split:
+
+- `CreateTestRepo`: fast setup for single-repo state.
+- `NewScenario`: multi-repo topologies and fluent setup.
+- `SnapshotRepo`/`RestoreSnapshot`: performance optimization for repeated expensive setups.
+
+## Adding new helpers
+
+When adding new exported helpers:
+
+- Add or update tests in `fixtures_test.go` and/or `scenarios_test.go`.
+- Add usage notes/examples to `README.md` when the helper is user-facing.
+- Keep function names explicit and test-focused (avoid generic utility names).
+
+## Snapshot safety expectations
+
+- Snapshot restore must never write outside the restore root.
+- Avoid introducing behavior that can restore unsupported file types silently.
+- Keep snapshot behavior deterministic for repeatable tests.
+
+## Pull request guidance
+
+- Keep PRs focused and small when possible.
+- Include a clear "why" in the commit/PR description.
+- Include a short test plan with commands you ran locally.
+- Prefer additive, backward-compatible changes for existing exported helpers.
+
+Suggested PR checklist:
+
+- [ ] public API changes documented in `README.md`
+- [ ] tests added/updated for behavior changes
+- [ ] `gofmt`, `go vet`, and `go test` all pass locally
+- [ ] no unrelated refactors mixed into functional changes
+
+## Maintainer/admin workflow
+
+### Branch and PR flow
+
+1. Create a feature branch from `main`.
+2. Commit focused changes with clear commit messages.
+3. Open a PR with summary and test plan.
+4. Merge only after CI is green.
+
+### Release flow
+
+1. Verify `main` has passed CI.
+2. Confirm `README.md` and this guide reflect current API/behavior.
+3. Pull release notes from merged PRs (user-visible changes first).
+4. Create and push a version tag.
+5. Publish a GitHub release with:
+ - notable changes
+ - compatibility notes (for example, minimum Go version)
+ - migration notes when behavior changed
+
+## Release notes checklist
+
+Before cutting a release:
+
+- Ensure CI is green on `main`.
+- Summarize user-visible changes (new helpers, behavior changes, compatibility notes).
+- Call out any minimum Go version changes.
+- Verify examples in `README.md` still compile conceptually with current API.
diff --git a/README.md b/README.md
--- a/README.md
+++ b/README.md
@@ -2,18 +2,57 @@
`git-testkit` provides helpers for writing Go tests that exercise real Git repositories.
+## Why use this
+
+- Exercise real git behavior instead of mocking command output.
+- Build common repo states quickly (dirty trees, detached HEAD, diverged remotes, worktrees).
+- Reuse expensive setups across tests with in-memory snapshots.
+
## Install
```bash
go get github.com/git-fire/git-testkit+## Requirements
+
+- git must be installed and available on PATH
+- Go 1.22+
+
What it includes
- Repository fixtures (
CreateTestRepo,CreateBareRemote,RunGitCmd) - Scenario builders for common multi-repo states (
NewScenario, conflict/worktree helpers) - Snapshot helpers for capturing and restoring repository state in tests
+## API overview
+
+- CreateTestRepo(t, RepoOptions) creates a real repository with optional files/remotes/branches.
+- CreateBareRemote(t, name) creates a bare repository for remote testing.
+- NewScenario(t) returns a fluent builder for multi-repo test topologies.
+- SnapshotRepo(t, path) and RestoreSnapshot(t, snap) speed up repeated fixture setup.
+- IsDirty, GetCurrentSHA, GetBranches, GetRemotes provide common assertions/helpers.
+
+## Quickstart for using in tests
+
+1. Create fixture repos with CreateTestRepo or NewScenario.
+2. Apply setup operations with fluent helpers (AddFile, Commit, WithRemote, Push).
+3. Run your code under test against the real repository paths.
+4. Assert with helper methods (IsDirty, GetCurrentSHA, GetBranches).
+
+Minimal test flow:
+
+```go
+func TestMyGitBehavior(t *testing.T) {
- repoPath := testutil.CreateTestRepo(t, testutil.RepoOptions{Name: "subject"})
- testutil.RunGitCmd(t, repoPath, "checkout", "-b", "feature")
- // call your package functions here
- if testutil.IsDirty(t, repoPath) {
-
t.Fatal("repo should be clean") - }
+}
+```
Example
@@ -34,3 +73,50 @@
}
}+## Cleanup behavior
+
+- All helper-created repositories use t.TempDir().
+- Repos/worktrees are automatically removed by Go's test framework at test completion.
+- As with any temp directories, force-killed test processes may leave files behind.
+
+## Common patterns
+
+### Build a conflict scenario
+
+```go
+func TestConflictFlow(t *testing.T) {
- _, local, _ := testutil.CreateConflictScenario(t)
- // Exercise your logic against a real diverged local clone.
- testutil.RunGitCmd(t, local.Path(), "status")
+}
+```
+### Snapshot expensive setup
+
+```go
+func TestUsingSnapshot(t *testing.T) {
- _, repo := testutil.CreateLargeRepoScenario(t, 20, 10)
- snap := testutil.SnapshotRepo(t, repo.Path())
- clonePath := testutil.RestoreSnapshot(t, snap)
- // Use clonePath in assertions without rebuilding the fixture each time.
- testutil.RunGitCmd(t, clonePath, "status")
+}
+```
+## Notes
+
+- Snapshots are intended for deterministic test fixtures and only restore regular files/directories.
+- Helpers fail tests immediately (t.Fatalf) when git commands fail, so errors surface close to setup code.
+- When tests build large repo graphs repeatedly, prefer snapshot/restore to reduce total runtime.
+
+## Developer docs
+
+- See DEVELOPER_GUIDE.md for:
-
- testing and quality gates
-
- usage guidance for library consumers
-
- administration/maintenance and release workflow
-
- contribution process and PR expectations
diff --git a/fixtures.go b/fixtures.go
--- a/fixtures.go
+++ b/fixtures.go
@@ -4,6 +4,7 @@
"os"
"os/exec"
"path/filepath"
- "strings"
"testing"
)
@@ -193,69 +194,43 @@
// "origin /path/to/remote (push)"
remotes := make(map[string]string)
- lines := string(output)
- lines := strings.TrimSpace(string(output))
if lines == "" {
return remotes
}
- // Simple parsing - just extract remote names
- // Full parsing not needed for tests
- for _, line := range splitLines(lines) {
- for _, line := range strings.Split(lines, "\n") {
-
line = strings.TrimSpace(line) if line == "" { continue }
-
// Just check if "origin" appears in the line -
// Good enough for test validation -
if len(line) > 0 { -
// Extract first word (remote name) -
parts := splitWhitespace(line) -
if len(parts) >= 2 { -
name := parts[0] -
url := parts[1] -
remotes[name] = url
-
name, remainder, ok := strings.Cut(line, "\t") -
if !ok { -
// Fallback for unusual formatting that does not use tabs. -
idx := strings.IndexAny(line, " \t") -
if idx == -1 { -
continue } -
name = line[:idx] -
remainder = strings.TrimSpace(line[idx+1:]) }
-
}
-
return remotes
-}
-
if strings.HasSuffix(remainder, ")") { -
if idx := strings.LastIndex(remainder, " ("); idx != -1 { -
suffix := remainder[idx+2 : len(remainder)-1] -
if suffix == "fetch" || suffix == "push" { -
remainder = remainder[:idx] -
} -
} -
}
-// Helper: split by newlines
-func splitLines(s string) []string {
- var lines []string
- current := ""
- for _, ch := range s {
-
if ch == '\n' { -
lines = append(lines, current) -
current = "" -
} else { -
current += string(ch)
-
if name != "" && remainder != "" { -
}
remotes[name] = remainder }
- if current != "" {
-
lines = append(lines, current) - }
- return lines
-}
-// Helper: split by whitespace/tabs
-func splitWhitespace(s string) []string {
- var parts []string
- current := ""
- for _, ch := range s {
-
if ch == ' ' || ch == '\t' { -
if current != "" { -
parts = append(parts, current) -
current = "" -
} -
} else { -
current += string(ch) -
} - }
- if current != "" {
-
parts = append(parts, current) - }
- return parts
- return remotes
}
// RunGitCmd runs a git command and fails the test if it errors
@@ -277,13 +252,7 @@
t.Fatalf("Failed to get current SHA: %v", err)
}
- sha := string(output)
- // Trim newline
- if len(sha) > 0 && sha[len(sha)-1] == '\n' {
-
sha = sha[:len(sha)-1] - }
- return sha
- return strings.TrimSpace(string(output))
}
// GetBranches returns all branches in the repo
@@ -298,7 +267,7 @@
t.Fatalf("Failed to get branches: %v", err)
}
- branches := splitLines(string(output))
-
branches := strings.Split(strings.TrimSpace(string(output)), "\n")
// Filter out empty lines
var result []string
diff --git a/fixtures_test.go b/fixtures_test.go
--- a/fixtures_test.go
+++ b/fixtures_test.go
@@ -79,6 +79,51 @@
}
}
+func TestCreateTestRepo_WithRemotePathContainingSpaces(t *testing.T) {
- remotePath := testutil.CreateBareRemote(t, "origin with space")
- repoPath := testutil.CreateTestRepo(t, testutil.RepoOptions{
-
Name: "remote-space-repo", -
Remotes: map[string]string{ -
"origin": remotePath, -
}, - })
- remotes := testutil.GetRemotes(t, repoPath)
- originURL, exists := remotes["origin"]
- if !exists {
-
t.Fatal("Expected 'origin' remote to be configured") - }
- if originURL != remotePath {
-
t.Fatalf("Expected origin URL %q, got %q", remotePath, originURL) - }
+}
+func TestCreateTestRepo_WithRemotePathContainingPushSuffix(t *testing.T) {
- tmpDir := t.TempDir()
- remotePath := filepath.Join(tmpDir, "origin (push)")
- if err := os.MkdirAll(remotePath, 0755); err != nil {
-
t.Fatalf("Failed to create bare repo directory: %v", err) - }
- testutil.RunGitCmd(t, tmpDir, "init", "--bare", remotePath)
- repoPath := testutil.CreateTestRepo(t, testutil.RepoOptions{
-
Name: "remote-push-suffix-repo", -
Remotes: map[string]string{ -
"origin": remotePath, -
}, - })
- remotes := testutil.GetRemotes(t, repoPath)
- originURL, exists := remotes["origin"]
- if !exists {
-
t.Fatal("Expected 'origin' remote to be configured") - }
- if originURL != remotePath {
-
t.Fatalf("Expected origin URL %q, got %q", remotePath, originURL) - }
+}
func TestCreateBareRemote(t *testing.T) {
remotePath := testutil.CreateBareRemote(t, "test-remote")
diff --git a/go.mod b/go.mod
--- a/go.mod
+++ b/go.mod
@@ -1,3 +1,3 @@
module github.com/git-fire/git-testkit
-go 1.24.2
+go 1.22
diff --git a/snapshots.go b/snapshots.go
--- a/snapshots.go
+++ b/snapshots.go
@@ -43,6 +43,9 @@
if err != nil {
return fmt.Errorf("failed to get relative path: %w", err)
}
-
if relPath == "." { -
return nil -
} header.Name = relPath // Write header
@@ -117,7 +120,10 @@
}
// Construct full path
-
targetPath := filepath.Join(restorePath, header.Name)
-
targetPath, err := safeJoin(restorePath, header.Name) -
if err != nil { -
t.Fatalf("Invalid snapshot path %q: %v", header.Name, err) -
} // Handle different file types switch header.Typeflag {
@@ -154,6 +160,30 @@
return restorePath
}
+func safeJoin(base, name string) (string, error) {
- cleanName := filepath.Clean(name)
- if cleanName == "." || cleanName == string(filepath.Separator) {
-
return "", fmt.Errorf("empty or root path is not allowed") - }
- if filepath.IsAbs(cleanName) {
-
return "", fmt.Errorf("absolute paths are not allowed") - }
- if cleanName == ".." || len(cleanName) >= 3 && cleanName[:3] == ".."+string(filepath.Separator) {
-
return "", fmt.Errorf("path traversal is not allowed") - }
- target := filepath.Join(base, cleanName)
- rel, err := filepath.Rel(base, target)
- if err != nil {
-
return "", err - }
- if rel == ".." || len(rel) >= 3 && rel[:3] == ".."+string(filepath.Separator) {
-
return "", fmt.Errorf("resolved path escapes restore directory") - }
- return target, nil
+}
// SnapshotSize returns the size of the snapshot in bytes
func (s *Snapshot) Size() int {
return len(s.tarball)
diff --git a/usb_fixtures.go b/usb_fixtures.go
new file mode 100644
--- /dev/null
+++ b/usb_fixtures.go
@@ -1,0 +1,143 @@
+package testutil
+
+import (
- "fmt"
- "net/url"
- "os"
- "path/filepath"
- "strconv"
- "strings"
- "testing"
- "time"
+)
+type USBVolumeOptions struct {
- LayoutDir string
- Strategy string
- CreateReposDir bool
+}
+type USBVolumeConfig struct {
- SchemaVersion int
- LayoutDir string
- Strategy string
- CreatedAt time.Time
+}
+func MustUSBVolumeRoot(t *testing.T, opts USBVolumeOptions) string {
- t.Helper()
- root := t.TempDir()
- cfg := USBVolumeConfig{
-
SchemaVersion: 1, -
LayoutDir: opts.LayoutDir, -
Strategy: opts.Strategy, -
CreatedAt: time.Now().UTC(), - }
- if cfg.LayoutDir == "" {
-
cfg.LayoutDir = "repos" - }
- if cfg.Strategy == "" {
-
cfg.Strategy = "git-mirror" - }
- WriteUSBVolumeConfig(t, root, cfg)
- if opts.CreateReposDir {
-
if err := os.MkdirAll(filepath.Join(root, cfg.LayoutDir), 0o755); err != nil { -
t.Fatalf("failed creating repos dir: %v", err) -
} - }
- return root
+}
+func WriteUSBVolumeConfig(t *testing.T, root string, cfg USBVolumeConfig) {
- t.Helper()
- if cfg.SchemaVersion <= 0 {
-
cfg.SchemaVersion = 1 - }
- if cfg.LayoutDir == "" {
-
cfg.LayoutDir = "repos" - }
- if cfg.Strategy == "" {
-
cfg.Strategy = "git-mirror" - }
- if cfg.CreatedAt.IsZero() {
-
cfg.CreatedAt = time.Now().UTC() - }
- content := fmt.Sprintf(
-
"schema_version = %d\nlayout_dir = %q\nstrategy = %q\ncreated_at = %q\n", -
cfg.SchemaVersion, -
cfg.LayoutDir, -
cfg.Strategy, -
cfg.CreatedAt.Format(time.RFC3339), - )
- if err := os.WriteFile(filepath.Join(root, ".git-fire"), []byte(content), 0o644); err != nil {
-
t.Fatalf("failed writing .git-fire: %v", err) - }
+}
+func ReadUSBVolumeConfig(t *testing.T, root string) USBVolumeConfig {
- t.Helper()
- data, err := os.ReadFile(filepath.Join(root, ".git-fire"))
- if err != nil {
-
t.Fatalf("failed reading .git-fire: %v", err) - }
- cfg := USBVolumeConfig{}
- lines := strings.Split(string(data), "\n")
- for _, line := range lines {
-
line = strings.TrimSpace(line) -
if line == "" || strings.HasPrefix(line, "#") { -
continue -
} -
key, val, ok := strings.Cut(line, "=") -
if !ok { -
continue -
} -
key = strings.TrimSpace(key) -
val = strings.TrimSpace(val) -
if unquoted, err := strconv.Unquote(val); err == nil { -
val = unquoted -
} else { -
val = strings.Trim(val, "\"") -
} -
switch key { -
case "schema_version": -
n, _ := strconv.Atoi(val) -
cfg.SchemaVersion = n -
case "layout_dir": -
cfg.LayoutDir = val -
case "strategy": -
cfg.Strategy = val -
case "created_at": -
if ts, err := time.Parse(time.RFC3339, val); err == nil { -
cfg.CreatedAt = ts -
} -
} - }
- return cfg
+}
+func AssertGitDirAt(t *testing.T, path string, wantBare bool) {
- t.Helper()
- if wantBare {
-
if _, err := os.Stat(filepath.Join(path, "HEAD")); err != nil { -
t.Fatalf("expected bare repo at %s: %v", path, err) -
} -
return - }
- if _, err := os.Stat(filepath.Join(path, ".git")); err != nil {
-
t.Fatalf("expected non-bare repo at %s: %v", path, err) - }
+}
+func FileURLForPath(t *testing.T, path string) string {
- t.Helper()
- abs, err := filepath.Abs(path)
- if err != nil {
-
t.Fatalf("failed to make abs path: %v", err) - }
- filePath := filepath.ToSlash(abs)
- if !strings.HasPrefix(filePath, "/") {
-
filePath = "/" + filePath - }
- u := &url.URL{Scheme: "file", Path: filePath}
- return u.String()
+}
diff --git a/usb_fixtures_test.go b/usb_fixtures_test.go
new file mode 100644
--- /dev/null
+++ b/usb_fixtures_test.go
@@ -1,0 +1,76 @@
+package testutil
+
+import (
- "os"
- "path/filepath"
- "testing"
+)
+func TestMustUSBVolumeRoot(t *testing.T) {
- root := MustUSBVolumeRoot(t, USBVolumeOptions{
-
LayoutDir: "repos", -
Strategy: "git-mirror", -
CreateReposDir: true, - })
- if _, err := os.Stat(filepath.Join(root, ".git-fire")); err != nil {
-
t.Fatalf("expected .git-fire marker: %v", err) - }
- if _, err := os.Stat(filepath.Join(root, "repos")); err != nil {
-
t.Fatalf("expected repos dir: %v", err) - }
+}
+func TestReadWriteUSBVolumeConfig(t *testing.T) {
- root := t.TempDir()
- WriteUSBVolumeConfig(t, root, USBVolumeConfig{
-
SchemaVersion: 2, -
LayoutDir: "custom", -
Strategy: "git-clone", - })
- cfg := ReadUSBVolumeConfig(t, root)
- if cfg.SchemaVersion != 2 {
-
t.Fatalf("schema mismatch: %d", cfg.SchemaVersion) - }
- if cfg.LayoutDir != "custom" {
-
t.Fatalf("layout mismatch: %s", cfg.LayoutDir) - }
- if cfg.Strategy != "git-clone" {
-
t.Fatalf("strategy mismatch: %s", cfg.Strategy) - }
+}
+func TestReadWriteUSBVolumeConfigEscapedValues(t *testing.T) {
- root := t.TempDir()
- WriteUSBVolumeConfig(t, root, USBVolumeConfig{
-
SchemaVersion: 3, -
LayoutDir: `repos\windows\"quoted"`, -
Strategy: `git\mirror`, - })
- cfg := ReadUSBVolumeConfig(t, root)
- if cfg.LayoutDir !=
repos\windows\"quoted"{ -
t.Fatalf("layout mismatch: %q", cfg.LayoutDir) - }
- if cfg.Strategy !=
git\mirror{ -
t.Fatalf("strategy mismatch: %q", cfg.Strategy) - }
+}
+func TestFileURLForPath(t *testing.T) {
- root := t.TempDir()
- got := FileURLForPath(t, root)
- if len(got) < 7 || got[:7] != "file://" {
-
t.Fatalf("expected file:// URL, got %s", got) - }
+}
+func TestAssertGitDirAt(t *testing.T) {
- nonBare := t.TempDir()
- runGit(t, nonBare, "init")
- AssertGitDirAt(t, nonBare, false)
- parent := t.TempDir()
- bare := filepath.Join(parent, "remote.git")
- runGit(t, parent, "init", "--bare", bare)
- AssertGitDirAt(t, bare, true)
+}
</details>
<sub>You can send follow-ups to the cloud agent <a href="https://cursor.com/agents/bc-d4201f0d-47b2-4e6c-aab5-d8e166063ad6">here</a>.</sub>
</details>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@usb_fixtures.go`:
- Around line 102-104: In the "schema_version" case the code calls
strconv.Atoi(val) and ignores the error, which causes malformed values to
produce 0 and overwrite cfg.SchemaVersion; change the logic in the case
"schema_version" branch to check the error return from strconv.Atoi and only
assign cfg.SchemaVersion = n when err == nil, otherwise leave cfg.SchemaVersion
unchanged (i.e., ignore malformed input) so previous valid values are not reset.
- Around line 120-127: The current checks only assert existence of HEAD and .git
but not their types; update the wantBare branch to stat filepath.Join(path,
"HEAD") and assert that it exists and is a regular file (os.FileMode where
FileInfo.Mode().IsDir() is false), failing via t.Fatalf if it's missing or a
directory, and update the non-bare branch to stat filepath.Join(path, ".git")
and assert that it exists and is a directory (FileInfo.Mode().IsDir() == true),
failing with t.Fatalf if it's missing or not a directory; keep existing error
messages and use the same variables (wantBare, path, t.Fatalf) so the checks are
stricter about entry types.
- Around line 43-45: The code calls os.MkdirAll(filepath.Join(root,
cfg.LayoutDir), ...) without validating cfg.LayoutDir, allowing absolute paths
or traversal like "../.." to escape root; before creating dirs (in the
opts.CreateReposDir branch) validate and sanitize cfg.LayoutDir: ensure it is
not an absolute path (use filepath.IsAbs), clean it (filepath.Clean),
split/check path elements do not start with ".." (or ensure the first element is
not ".."), or compute absTarget := filepath.Join(root, cfg.LayoutDir) then
resolve absTarget and root with filepath.Abs and verify absTarget has root as
its prefix; if validation fails call t.Fatalf with a clear error; only then call
os.MkdirAll on the validated target.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 723cfe51-3cbf-42e2-b6fc-c9c50365f995
📒 Files selected for processing (2)
usb_fixtures.gousb_fixtures_test.go
✅ Files skipped from review due to trivial changes (1)
- usb_fixtures_test.go
Resolve conflicts: combine main snapshot/docs/tests with USB helpers; harden GetRemotes by stripping only one (fetch)/(push) suffix; tighten AssertGitDirAt with file/dir checks. Made-with: Cursor
|
bugbot run |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
usb_fixtures.go (1)
170-187: Add regression tests for the new entry-type guards.Line 175 and Line 185 introduce stricter behavior, but current tests cover only happy paths. Please add negative cases where
HEADis a directory and.gitis a regular file, so these checks don’t regress.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usb_fixtures.go` around lines 170 - 187, Add two negative unit tests that exercise the new entry-type guards: one where HEAD is created as a directory (so filepath.Join(path, "HEAD") exists and IsDir() == true) and assert the code path that currently calls t.Fatalf is executed, and another where .git is created as a regular file (so filepath.Join(path, ".git") exists but IsDir() == false) and assert the corresponding fatal error occurs; target the code that checks headPath/info.IsDir() and gitPath/info.IsDir() (the checks around headPath := filepath.Join(path, "HEAD") and gitPath := filepath.Join(path, ".git")) so these failure cases are covered and won’t regress.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@usb_fixtures.go`:
- Around line 170-187: Add two negative unit tests that exercise the new
entry-type guards: one where HEAD is created as a directory (so
filepath.Join(path, "HEAD") exists and IsDir() == true) and assert the code path
that currently calls t.Fatalf is executed, and another where .git is created as
a regular file (so filepath.Join(path, ".git") exists but IsDir() == false) and
assert the corresponding fatal error occurs; target the code that checks
headPath/info.IsDir() and gitPath/info.IsDir() (the checks around headPath :=
filepath.Join(path, "HEAD") and gitPath := filepath.Join(path, ".git")) so these
failure cases are covered and won’t regress.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9eb325e6-12aa-4b31-8288-4b44e2f05d5d
📒 Files selected for processing (3)
DEVELOPER_GUIDE.mdfixtures.gousb_fixtures.go
✅ Files skipped from review due to trivial changes (2)
- DEVELOPER_GUIDE.md
- fixtures.go
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit af329f9. Configure here.
2 participants
Summary
MustUSBVolumeRoot, marker read/write helpers, git dir assertions, and file URL conversion)Test plan
go test ./...Note
Low Risk
Low risk: small, targeted changes to test helper parsing and filesystem assertions, covered by new edge-case tests.
Overview
Tightens
GetRemotesparsing so it strips only one trailing(fetch)/(push)role suffix, avoiding corruption when a remote URL/path itself ends with(push).Hardens USB fixture
AssertGitDirAtto validate file-vs-directory expectations for bareHEADand non-bare.git, and adds/updates tests plus a briefDEVELOPER_GUIDE.mdnote about the new USB fixture helpers.Reviewed by Cursor Bugbot for commit af329f9. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Release Notes
Documentation
Bug Fixes
Tests