feat: show remote sync status in status commands#89
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Implements Fetch (git fetch) and RemoteStatus (ahead/behind counts via git rev-list --left-right) on the gitops.Client, with TDD coverage for up-to-date, ahead, and behind scenarios. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The shared WriteErrors helper uses "package errors:" instead of "Errors:" — update the assertion to match. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
richhaase
left a comment
There was a problem hiding this comment.
Findings
- Unbounded git fetch blocks status commands on network issues (6/6 reviewers)
RemoteStatus() performs a synchronous git fetch with no timeout via context.Background() on every invocation of status, packages, and dotfiles commands. This turns previously fast local-only commands into potentially hanging network operations when remotes are unreachable, slow, or require authentication.
Evidence:
- internal/gitops/gitops.go:149-151: RemoteStatus() does unconditional git fetch before every status/packages/dotfiles render; can hang indefinitely on unreachable remotes
- internal/commands/remote_sync.go:15-16: getRemoteSyncStatus uses context.Background() with no timeout; all callers (status.go:68, packages.go:39, dotfiles.go:45) pass no deadline
- context.Background() used instead of cmd.Context() prevents Ctrl+C cancellation, potentially leaving orphan git processes
- Needs timeout-bounded path, cached tracking refs, or explicit opt-in refresh to avoid regressing offline/auth-expired workflows
- Dead code in empty-state detection for dotfiles and packages formatters (2/6 reviewers)
The titleOnly string used for empty-output checks is manually reconstructed without accounting for the trailing newline added at line 118, so the comparison never matches and the 'No managed dotfiles' / 'No managed packages' fallback is dead code.
Evidence:
- internal/output/dotfiles_status_formatter.go:121-126: titleOnly comparison never matches because output includes trailing newline from line 118
- Same bug exists in packages_status_formatter.go:95-100; brittle hardcoded string comparisons duplicate output logic of WriteTitle and WriteRemoteSync
- git rev-list fails silently when no upstream tracking branch is configured (1/6 reviewers)
The git rev-list HEAD...@{upstream} command in RemoteStatus will fail if the current branch has no upstream configured. The error is silently swallowed, causing sync status to be missing without explanation.
Evidence:
- internal/gitops/gitops.go:159: git rev-list HEAD...@{upstream} fails if no upstream tracking branch configured; error silenced by getRemoteSyncStatus
Expand for verbatim findings.
Raw findings (verbatim)
- (1/6 reviewers)
`internal/commands/status.go:68`, `internal/commands/dotfiles.go:45`, `internal/commands/packages.go:39`: `context.Background()` with no timeout is passed through to `git fetch` (via `RemoteStatus`). If the network is unreachable or the remote hangs, these commands will block indefinitely. Needs a `context.WithTimeout`.
- (1/6 reviewers)
`internal/commands/remote_sync.go:16`: `getRemoteSyncStatus` uses `context.Background()` (passed from all callers) with no timeout. `RemoteStatus` calls `git fetch` over the network -- if the remote is unreachable, the command hangs indefinitely. Every caller (`status.go:68`, `packages.go:39`, `dotfiles.go:45`) passes `context.Background()` with no deadline.
- (1/6 reviewers)
internal/gitops/gitops.go:148: Implicit network call in `RemoteStatus` (via `Fetch`) causes performance regressions in `status`, `dotfiles`, and `packages` commands, which are expected to be fast local operations.
internal/commands/remote_sync.go:15: Missing timeout for `git fetch` operation; commands like `plonk status` will hang indefinitely if the remote repository is unresponsive or the network is slow.
internal/output/dotfiles_status_formatter.go:126: Brittle empty-state detection using hardcoded string comparisons that duplicate the output logic of `WriteTitle` and `WriteRemoteSync`, leading to potential UI bugs if those helpers are updated.
- (1/6 reviewers)
- internal/gitops/gitops.go:151: Performance issue - `RemoteStatus` triggers a synchronous network operation (`git fetch`) on every call. This will cause noticeable latency in the `status`, `dotfiles`, and `packages` commands, especially on slow or unstable network connections.
- internal/commands/dotfiles.go:45: Maintainability issue - `context.Background()` is used instead of `cmd.Context()`. This prevents command-line cancellations (e.g., via Ctrl+C) from being propagated to the underlying Git operations, potentially leaving orphan processes. (Also in `packages.go:38` and `status.go:67`).
- internal/gitops/gitops.go:159: Logic error - `git rev-list ... HEAD...@{upstream}` will fail if the current branch does not have an upstream tracking branch configured. While `getRemoteSyncStatus` silences the error, it means the sync status will be missing for many common Git workflows without explanation.
- (1/6 reviewers)
The new remote-sync feature introduces an unbounded network fetch into read-only status commands. That can make common status invocations hang or block on Git authentication, so the patch is not safe to consider correct as-is.
Review comment:
- [P1] Avoid blocking status commands on interactive `git fetch` — /Users/rdh/src/plonk/internal/gitops/gitops.go:149-150
`RemoteStatus()` now does an unconditional `git fetch` before every `plonk status`/`packages`/`dotfiles` render. In repos whose remote is private, offline, or only reachable over VPN, `git fetch` can prompt for credentials or sit in network timeouts, so these read-only commands can hang indefinitely instead of returning the local status they used to show immediately. This needs a non-interactive/timeout-bounded path, or an explicit opt-in refresh, to avoid breaking common offline/auth-expired workflows.
- (1/6 reviewers)
The new remote-sync feature makes all status-style commands perform a live fetch, which can turn simple local status checks into slow or hanging network operations. That regression is significant enough that the patch should not be considered fully correct.
Review comment:
- [P2] Avoid blocking status commands on an unbounded `git fetch` — /Users/rdh/src/plonk/internal/gitops/gitops.go:149-151
`plonk status`, `plonk packages`, and `plonk dotfiles` now all reach this path to render the new remote-sync line. Because `RemoteStatus` always runs a real `git fetch` with `context.Background()`, these previously local-only commands can stall for many seconds on slow/unreachable remotes (for example, an SSH remote behind a disconnected VPN) before silently dropping the remote status. This is a user-visible regression for any repo with a configured remote; reading cached tracking refs or making the refresh explicit would avoid turning every status check into a network operation.
- (1/6 reviewers)
`internal/output/dotfiles_status_formatter.go:121-126`: The `titleOnly` string used for the "no managed dotfiles" empty-output check is manually reconstructed instead of derived from the same helpers that wrote the output. An unconditional `output.WriteString("\n")` at line 118 means the actual output when empty is `titleOnly + "\n"`, so the comparison `outputStr == titleOnly` will never match and the "No managed dotfiles." fallback is dead code. Same bug exists in `packages_status_formatter.go:95-100`.
Posted by acr 0.15.0
- Add 10s timeout to git fetch in getRemoteSyncStatus so status commands don't hang on unreachable remotes - Use cmd.Context() instead of context.Background() so Ctrl+C properly cancels the fetch - Fix empty-state detection in all three formatters to use item counts instead of brittle string comparison Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
richhaase
left a comment
There was a problem hiding this comment.
Findings
- Blocking git fetch causes latency regression in status commands (4/6 reviewers)
The synchronous git fetch with a 10-second timeout in getRemoteSyncStatus makes previously-local commands (status, packages, dotfiles) stall on slow, offline, or unreachable networks. Consider cached refs, a shorter/non-blocking probe, or opt-in fetch.
Evidence:
- remote_sync.go:29-32 — synchronous git fetch makes status commands wait up to 10s on slow/offline networks, a noticeable responsiveness regression
- remote_sync.go:18 — synchronous fetch with 10s timeout will cause noticeable latency especially on slow networks
- Branches with no upstream still pay an unnecessary network round-trip; remote result is ultimately discarded
- Fetch runs before checking for upstream tracking branch (1/6 reviewers)
RemoteStatus performs a git fetch before verifying if the current branch has an upstream configured. It should check for an upstream first (e.g., git rev-parse --abbrev-ref @{u}) to avoid unnecessary network calls.
Evidence:
- gitops.go:142 — git fetch runs before verifying upstream tracking branch; check upstream first to skip needless network calls
- Errors from git operations are silently swallowed (1/6 reviewers)
Errors from fetch failures or missing upstream are swallowed with no feedback to the user on why sync status is missing.
Evidence:
- remote_sync.go:32 — errors from git operations (fetch failures, missing upstream) are swallowed, providing no feedback to the user
- Underline width incorrect for multi-byte UTF-8 titles (1/6 reviewers)
len(title) counts bytes, not characters, so underlines will be incorrectly sized for titles containing multi-byte UTF-8 characters.
Evidence:
- format_helpers.go:10 — len(title) for underline count will be wrong for multi-byte UTF-8 characters
Expand for verbatim findings.
Raw findings (verbatim)
- (1/6 reviewers)
internal/commands/remote_sync.go:18: Performance issue - Synchronous `git fetch` with a 10-second timeout in `getRemoteSyncStatus` will cause noticeable latency in `status`, `packages`, and `dotfiles` commands, especially on slow networks.
internal/gitops/gitops.go:142: Efficiency issue - `RemoteStatus` performs a `git fetch` before verifying if the current branch has an upstream tracking branch configured; it should check for an upstream (e.g., via `git rev-parse --abbrev-ref @{u}`) first to avoid unnecessary network calls.
internal/gitops/gitops.go:158: Best practice violation - Use `strconv.Atoi` instead of `fmt.Sscanf` for parsing simple integer strings from command output.
- (1/6 reviewers)
internal/commands/remote_sync.go:17: Performance issue - blocking git fetch on every status check will cause significant delays (up to 10s) on slow networks.
internal/commands/remote_sync.go:32: Logic error - errors from git operations (like fetch failures or missing upstream) are swallowed, providing no feedback to the user on why sync status is missing.
internal/output/format_helpers.go:10: Maintainability/Best practice - `len(title)` used for underline count will result in an incorrectly sized underline if the title contains multi-byte UTF-8 characters.
- (1/6 reviewers)
The patch adds a synchronous remote fetch to status-style commands, which can make them hang for up to 10 seconds in common offline or unreachable-remote scenarios. Aside from that regression, the rest of the changes look reasonable.
Review comment:
- [P2] Skip blocking fetches in local status commands — /Users/rdh/src/plonk/internal/commands/remote_sync.go:29-32
When the config repo's remote is slow or unreachable (for example, offline use, a down VPN, or an SSH remote waiting on auth/network timeout), `getRemoteSyncStatus()` now makes `plonk status`, `plonk packages`, and `plonk dotfiles` wait on a synchronous `git fetch` for up to `fetchTimeout` before showing otherwise-local results. These commands used to complete without network access, so this is a noticeable responsiveness regression in a common scenario; consider using cached refs, a much shorter/non-blocking probe, or making the fetch opt-in.
- (1/6 reviewers)
The new remote-sync feature introduces a user-visible performance regression by making read-only status commands depend on a blocking network fetch. Even though failures are swallowed, the extra fetch can still delay these commands substantially in common offline or no-upstream scenarios.
Review comment:
- [P2] Avoid unconditional fetch in status commands — /Users/rdh/src/plonk/internal/commands/remote_sync.go:29-32
When `$PLONK_DIR` has a git remote, `plonk status`, `plonk packages`, and `plonk dotfiles` now always do a synchronous `git fetch` before printing any local state. That means branches with no upstream still pay an unnecessary network round-trip, and users on a slow/offline/auth-blocked remote can see every status command stall until the 10s timeout even though the remote result is ultimately discarded. This is a noticeable regression for commands that were previously local and fast.
Posted by acr 0.15.0
Skip the network fetch entirely when the branch has no upstream tracking branch configured. Reduces timeout from 10s to 5s for the remaining cases where fetch does run. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Return nil instead of an error when no upstream tracking branch is configured — this is expected in plonk, not an error condition. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Empty config now shows "No managed items." instead of "Summary: 0 managed" since the empty-state detection was fixed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
git fetch+git rev-list --count --left-right HEAD...@{upstream}to detect ahead/behind stateRemote: up to date/behind by N commits (run plonk pull)/ahead by N commits (run plonk push)/divergedat the top ofplonk status,plonk packages, andplonk dotfilesWriteTitle,WriteRemoteSync,WriteErrors) to reduce duplication across the three status formattersTest plan
SyncStatus.String()covering all four states (up to date, ahead, behind, diverged) with singular/pluralFetch()andRemoteStatus()using temp repos with bare remotesplonk status,plonk packages,plonk dotfilesshow the Remote line🤖 Generated with Claude Code