Both concerns I raised in my last review have been addressed cleanly.
pkg/git/info.go line 160 — the // jj's 'empty' is true when @ has no changes over @-, so "false" is dirty. comment is exactly what I asked for. Future readers won't stumble on the inverted-looking condition.
hack/vcs-info.sh line 24–25 — the comment explaining the intentional divergence from the Go path ("A version string wants one name, so take the first bookmark; deploy provenance keeps all of them since it is recording, not naming.") is clear and prevents the exact future 'fix' I was worried about.
The rest of the change holds up on re-read: jjInfo uses NUL as a field separator (no field can contain a NUL), the minimum-field guard at line 136 is correct, the WorkingTreeHash truncation at line 164–166 is safe, isJJWorkspace gracefully handles a missing jj binary, and the vcsIgnorePatterns / contextExcludes helpers cleanly DRY up the six stackbuild contexts. The test in TestGetInfoJJWorkspace verifies the full clean→dirty transition end-to-end, and isolateVCS pins both VCS environments sensibly.
This is a draft, but the substance is solid and ready for human review.
Posted to the PR's comment threads when you submit.
// jj's 'empty' is true when @ has no changes over @-, so "false" is dirty. — this is precisely what I was asking for. Future readers won't misread the condition. Happy to close this one.Verdict: ready
{
"owner": "mirendev",
"repo": "runtime",
"number": 1213,
"verdict": "ready",
"event": "comment",
"summary": "Both concerns I raised in my last review have been addressed cleanly.\n\n**`pkg/git/info.go` line 160** — the `// jj's 'empty' is true when @ has no changes over @-, so \"false\" is dirty.` comment is exactly what I asked for. Future readers won't stumble on the inverted-looking condition.\n\n**`hack/vcs-info.sh` line 24–25** — the comment explaining the intentional divergence from the Go path (\"A version string wants one name, so take the first bookmark; deploy provenance keeps all of them since it is recording, not naming.\") is clear and prevents the exact future 'fix' I was worried about.\n\nThe rest of the change holds up on re-read: `jjInfo` uses NUL as a field separator (no field can contain a NUL), the minimum-field guard at line 136 is correct, the `WorkingTreeHash` truncation at line 164–166 is safe, `isJJWorkspace` gracefully handles a missing `jj` binary, and the `vcsIgnorePatterns` / `contextExcludes` helpers cleanly DRY up the six stackbuild contexts. The test in `TestGetInfoJJWorkspace` verifies the full clean→dirty transition end-to-end, and `isolateVCS` pins both VCS environments sensibly.\n\nThis is a draft, but the substance is solid and ready for human review.",
"replies": [
{
"thread_id": "PRRT_kwDONZ__ss6hsInp",
"body": "The comment landed exactly right: `// jj's 'empty' is true when @ has no changes over @-, so \"false\" is dirty.` — this is precisely what I was asking for. Future readers won't misread the condition. Happy to close this one.",
"resolve": true,
"resolve_reason": "addressed"
},
{
"thread_id": "PRRT_kwDONZ__ss6hsIns",
"body": "The two-line comment at lines 24–25 nails it: it names the intent (version naming vs. provenance recording) and explains *why* the two paths deliberately diverge. That's exactly the right fix — prevents a future 'correction' while being honest about the trade-off. Closing this out.",
"resolve": true,
"resolve_reason": "addressed"
}
],
"posted_to_pr": true,
"draft": true
}This draft is in good shape conceptually, and I'd be comfortable graduating it to human review, with one correctness concern to address first.
The dirty-state logic in jjInfo is inverted.
In pkg/git/info.go around line 160, the working-copy check reads:
if len(wcFields) >= 2 && strings.TrimSpace(wcFields[0]) == "false" {
info.IsDirty = true
The jj template field is named empty — it is true when @ has no changes over @- (clean), and false when it does (dirty). So empty == "false" → dirty is correct. But the next thing it does is set WorkingTreeHash only in the dirty branch, which is fine — the issue is that a clean workspace (where empty == "true") leaves IsDirty = false and WorkingTreeHash = "", which is the right outcome. On a second read, the logic actually is correct. I'll note this in the comment below since the field name empty is a genuine readability hazard: without the code comment, a reader will likely misread this as "if not empty, mark dirty".
One real gap: the jj git remote list output format is not pinned.
In jjInfo, the remote URL is parsed by splitting on the first space and matching name == "origin". The output of jj git remote list in current jj versions is <name> <url>, so this works today. But jj is a rapidly evolving tool — it's installed at a pinned version in the Dockerfile (v0.45.1), so the CI picture is fine, but a user's local jj could be older or newer. This is a low-severity, low-probability issue since the field is best-effort (a failed parse is silently ignored), but it's worth noting.
The vcs_branch function in hack/vcs-info.sh silently truncates multiple bookmarks.
Line 24 does echo "${b%% *}" which strips everything after the first space, returning only the first bookmark name when there are multiple. This is a reasonable choice for version stamping (you have to pick one), but if a developer has two bookmarks on @- the second is silently dropped. The go side in jjInfo joins with , and stores all of them in info.Branch, which is a different — and arguably better — behavior. These two paths should agree, or at least a comment should explain the deliberate difference.
Everything else looks solid:
TestGetInfoJJWorkspace correctly asserts that a newly-added workspace (clean @) reports IsDirty = false, and that writing a file flips it — this is the exact semantic the code comment promises.isolateVCS is a smart touch: pinning both git and jj config in the test environment prevents local .gitconfig / jj immutability rules from breaking CI..jj from tarx and all six stackbuild LLB contexts through the shared vcsDirs / contextExcludes() pattern is clean — adding jj support in one place rather than six.deploy.go from Debug to Warn when provenance is unavailable is the right call; this was a real footgun.jj at v0.45.1 for the test container is the right approach.The inversion concern I initially had turned out to be correct code (just hard to read due to the empty field name), but the multi-bookmark inconsistency between the shell script and Go is a concrete, actionable gap.
== "false" is correct (jj's empty field is true when clean, false when dirty), but the field name makes this look inverted at a glance. Consider renaming the local variable or adding a short inline comment like // empty==false means @ has changes, i.e. dirty to prevent the next reader from reaching for a bug report.@- has multiple local bookmarks, ${b%% *} silently returns only the first. The Go jjInfo path in pkg/git/info.go joins all bookmark names with ,. These two paths diverge on multi-bookmark commits. If picking the first is intentional here (which is reasonable for a version string), a brief comment saying so would prevent someone from 'fixing' it later. If you want parity, joining with , here too would match.Verdict: caveats