feat(vcs): compare commit versions by repository history

[MeteorsLiu]

Design: #182

[fennoai]

Review: VCS-backed version comparison

This PR adds git-history–based version comparison (vcs.CompareFunc), wiring internal/vcs.Repo.CompareFunc into the comparator runtime via RegisterExternal, and consolidates git invocation into a shared gitCommand helper. The design is well tested (context isolation + reverse-symmetry invariants) and the git shell-out is defended against option injection (--end-of-options, check-ref-format). The revisions cache correctly avoids re-resolving refs, and locking around the map is sound.

The main items worth addressing before merge are architectural rather than mechanical:

• Panic-based error handling in the comparator path. resolveRevision/prepareHistory panic on every failure (network error, unknown ref, git failure). CompareFunc is invoked from latestVersion inside slices.MaxFunc (internal/modules/load.go:74) with no recover() on that path — only runFormulaHook recovers, and it does not wrap comparator invocation. A transient git fetch failure or a malformed version string therefore crashes the whole process instead of surfacing a comparable error. The CompareFunc(...) int signature (no error return) is the root cause; consider (int, error) or pre-resolving refs before comparison.
• Lock scope defeats MVS parallelism. CompareFunc holds historyMu across the full-history git fetch and every per-ref git subprocess. MVS deliberately parallelizes with a 10-worker pool, but per-repo comparisons are fully serialized behind this lock. Narrow historyMu to the map + one-time init (sync.Once) and run git/network work outside it.

Inline comments cover these plus a few smaller items. No blocking (P0) issues.

Additional findings

• /workspace/llar/internal/vcs/git.go:21: [P3] gitCommand indexes args[0] without guarding empty args: The error path formats args[0] without checking that args is non-empty; a zero-arg call would panic inside the error handler. No current caller passes empty args, but this is a shared helper — a small len(args) > 0 guard (or a default label) would make it robust.
• internal/vcs/repo.go:99: [P3] parseRepoPath does not validate path segments before URL construction: modPath is module-derived and flows unvalidated into fmt.Sprintf("github.com/%s", modPath) and then into repoURL = https://github.com/<owner>/<repo>.git. parseRepoPath accepts any characters, including .///unicode. This isn't argument injection (the https:// scheme prevents that), but a modPath containing ../ or extra / segments could alter the effective fetch target (SSRF-style) or produce unexpected owner/name splits. A strict per-segment allowlist (e.g. ^[A-Za-z0-9._-]+$) at NewRepo/parseRepoPath would close this trust-boundary gap defensively.

[fennoai]

[P1] CompareFunc panics on transient failures on a non-recovered path

CompareFunc → resolveRevision/prepareHistory panic on every error: network/fetch failure, unresolvable ref, git subprocess failure, and timestamp parse failure. These are expected runtime conditions (offline network, bad tag, deleted repo), not programmer errors.

The critical concern is the call path: latestVersion invokes the comparator directly inside slices.MaxFunc (internal/modules/load.go:74) with no recover(). The only recovery in the package is runFormulaHook, which wraps Filter/OnRequire, not comparator invocation. So a transient git fetch failure or a malformed version string turns a recoverable "cannot compare versions" into a full process crash — and is a DoS vector for untrusted version strings that fail rev-parse.

This is also inconsistent with the rest of vcs (Tags, Latest, Sync, syncHistory all return errors). Root cause is the CompareFunc(...) int signature having no error channel; consider (int, error), or resolve/validate all refs (surfacing errors) before the comparison sort begins.

[fennoai]

[P1] historyMu held across git subprocesses + network fetch serializes MVS

CompareFunc acquires r.historyMu and holds it for the entire body — including prepareHistory (a full-history git fetch --filter=blob:none of all heads/tags) and both resolveRevision calls (each spawning ~4–5 git subprocesses: check-ref-format, rev-parse, show -s, tag --merged, tag --points-at).

MVS deliberately parallelizes network work with a 10-worker pool, but per-repo comparisons are now fully serialized behind this lock, and the expensive fetch runs synchronously on the first comparison rather than overlapping with the rest of the load. The lock only needs to protect the revisions map and the one-time history init. Recommend: use sync.Once (or singleflight) for prepareHistory, and lock only around the revisions map read/write, running git/network work outside the lock. Also consider batching ref resolution (git for-each-ref/a single git log --format) to reduce ~5N subprocess spawns for N versions.

[fennoai]

[P2] Comparison git calls ignore context cancellation

runGit hardcodes context.Background() and prepareHistory passes context.Background() to syncHistory. The surrounding Load/compareModuleVersion flow threads a real ctx everywhere else, so a caller that cancels (timeout, Ctrl-C) cannot interrupt the potentially long git fetch or the per-revision git invocations. Because the CompareFunc interface method takes no ctx, the context is structurally unavailable — another consequence of the interface shape worth revisiting alongside the panic/error-return question.

[fennoai]

[P2] History temp dir cleanup relies on GC finalizer

On the success path the only cleanup of historyDir is runtime.AddCleanup(r, ...), which fires only when the *repo becomes unreachable and GC runs. repo instances are cached in moduleCache for the lifetime of a Load (and possibly beyond), so each full-history bare clone persists on disk until some indeterminate later GC. For a long-lived process loading many modules that use vcs.CompareFunc, these temp dirs accumulate. Consider an explicit Close/teardown hook on Repo for deterministic removal. (The cleanup func correctly captures only dir, not r — good.)

[codecov]

Codecov Report

❌ Patch coverage is 99.20635% with 2 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/vcs/compare.go	97.64%	1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[fennoai]

Review: Git-backed version comparison (x/git)

Solid, well-tested addition. compareRevisions/comparableRevisionOf are pure and unit-tested with a symmetry invariant, --end-of-options and check-ref-format are used correctly to guard against ref/option injection, and --filter=blob:none keeps the fetch lean. The main concerns are the per-comparison network fetch on a hot sort path, an unbounded/uncancelable git fetch, and a modPath→URL host-redirection surface. Details inline.

Also worth noting (no reliable single line):

• CompareFunc panics on every error path (temp dir, fetch, resolve). It is wired as a formula comparator and reaches slices.MaxFunc in internal/modules/load.go:74 (latestVersion) and compareModuleVersion at load.go:94, neither of which recovers nearby — a transient git/network failure will abort resolution via panic rather than a handled error. Since the func(a, b) int signature has no error return this may be intentional, but the doc comment on CompareFunc should state that it panics and which layer is expected to recover.

[fennoai]

Performance: full network git fetch on every pairwise comparison.

CompareFunc calls fetchHistory (init bare repo + remote add + full git fetch of all heads/tags) into a fresh temp dir on every invocation. This comparator is passed to slices.MaxFunc in internal/modules/load.go:74 (latestVersion), so selecting the latest of N tags triggers O(N) full network fetches of the same repo (O(N log N) on general sort paths), each re-downloading identical history.

Consider fetching history once per module path and reusing it — either memoize the prepared bare repo keyed on a.Path (the fetch only uses a.Path), or restructure to "fetch once → resolve all revisions → sort in memory with compareRevisions", collapsing O(N) fetches into one. Recommend addressing before this runs against real modules with many tags.

[fennoai]

Security: modPath is interpolated into the clone URL without validation (host-redirection / SSRF surface).

modPath flows from formula source (internal/modules/source.go) and is not validated here. A value containing @ — e.g. x@evil.com/y — produces https://github.com/x@evil.com/y.git, which git parses with host evil.com, redirecting the fetch to an attacker-controlled server. Args are passed as argv (no shell), so this is bounded, but it is still a real redirection vector.

Suggest validating modPath against an owner/repo shape (reject @, relative segments, control chars) or url.Parse + assert Host == "github.com" before building the URL. Note module.EscapePath exists but isn't applied here. For defense-in-depth against a hostile remote, also consider pinning empty credential.helper and neutralizing GIT_ASKPASS alongside the existing GIT_TERMINAL_PROMPT=0.

[fennoai]

No context/timeout on the git subprocesses.

CompareFunc takes no context.Context and runGit uses execbroker.Command with no deadline, so a git fetch against an unresponsive remote can hang indefinitely. Because this runs inside dependency resolution (slices.MaxFunc), a single stalled remote blocks the whole resolution with no way to cancel. Consider threading a context through and using a command-with-context / timeout.

[fennoai]

Minor: runGit indexes args[0] in the error path without a guard. All current callers pass at least one arg so it's not a live bug, but runGit(dir) with no args would panic with index-out-of-range and mask the real error. A defensive check would make it safer against future callers.

[luoliwoshang]

似乎当前测试并没有表现出 commit 对比的部分

[MeteorsLiu]

Done in 7c0715f