fix(host-service): v2 workspace git correctness (upstream, 3-dot, numstat)#3543
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughAdds upstream-tracking resolution and base-ref determination for git comparisons, switches diff parsing to NUL-delimited output, updates workspace creation to prefer reachable upstream refs, and introduces tests and documentation for the V2 workspace diff behaviors. Changes
Sequence DiagramssequenceDiagram
participant Client
participant GitRouter as Git Router
participant BaseResolver as resolveBaseComparison
participant GitCfg as Git Config
participant DiffParser as Diff Parser
Client->>GitRouter: Request diff/comparison
GitRouter->>BaseResolver: resolveBaseComparison(explicitBranch?)
BaseResolver->>GitCfg: Read branch.<name>.remote
GitCfg-->>BaseResolver: remote (or missing)
BaseResolver->>GitCfg: Read branch.<name>.merge
GitCfg-->>BaseResolver: merge (or missing)
BaseResolver-->>GitRouter: { branchName, baseRef } or null
GitRouter->>GitRouter: Compute three-dot diff range (baseRef...HEAD)
GitRouter->>GitRouter: Run: git diff --name-status -z / git diff --numstat -z
GitRouter->>DiffParser: Provide NUL-delimited output
DiffParser-->>GitRouter: Structured change data
GitRouter-->>Client: Changes with line counts
sequenceDiagram
participant CreationAPI as Creation API
participant WSCreator as workspaceCreationRouter
participant StartResolver as resolveStartPoint
participant UpstreamResolver as resolveUpstream
participant GitVerify as git rev-parse
CreationAPI->>WSCreator: Create workspace from branch
WSCreator->>StartResolver: Resolve initial startPoint
StartResolver-->>WSCreator: startPoint { kind: "local", branchName }
alt startPoint is local default branch
WSCreator->>UpstreamResolver: resolveUpstream(branchName)
UpstreamResolver-->>WSCreator: { remote, remoteBranch } or null
alt upstream found
WSCreator->>GitVerify: git rev-parse --verify --quiet <remote>/<remoteBranch>^{commit}
GitVerify-->>WSCreator: exists?
alt exists
WSCreator->>WSCreator: Mutate startPoint -> remote-tracking (remote, remoteBranch, fullRef)
end
end
end
WSCreator->>WSCreator: Fetch/create worktree using startPoint.kind
WSCreator-->>CreationAPI: Workspace created
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 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)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/host-service/src/trpc/router/workspace-creation/workspace-creation.ts (1)
630-667:⚠️ Potential issue | 🟠 MajorFetch the remote-tracking ref before checking its existence.
The code verifies
refs/remotes/<remote>/<branch>exists at line 630 before fetching upstream. On first run (when the remote-tracking ref hasn't been fetched yet), this check fails and the configured upstream is never set as the start point, even though fetching it later would make it available. Fetch the explicit refspec into the remote-tracking branch first, then verify it exists.🐛 Proposed direction
- const remoteExists = await git - .raw([ - "rev-parse", - "--verify", - "--quiet", - `${remoteRef}^{commit}`, - ]) + await git + .raw([ + "fetch", + upstream.remote, + `+refs/heads/${upstream.remoteBranch}:${remoteRef}`, + "--quiet", + "--no-tags", + ]) + .catch(() => {}); + const remoteExists = await git + .raw([ + "rev-parse", + "--verify", + "--quiet", + `${remoteRef}^{commit}`, + ]) .then(() => true) .catch(() => false);Also consider using the same explicit refspec in the later remote-tracking fetch path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/trpc/router/workspace-creation/workspace-creation.ts` around lines 630 - 667, The code checks for the existence of remoteRef (refs/remotes/<remote>/<branch>) before fetching, which fails on a fresh clone; modify the logic around remoteRef/upstream so you first fetch the explicit refspec for that upstream (use git.fetch with upstream.remote and an explicit refspec mapping the remote branch into refs/remotes/<remote>/<branch>) before running the rev-parse existence check, then set startPoint to the "remote-tracking" shape when remoteRef is found; also reuse the same explicit refspec pattern in the later fetch block that handles startPoint.kind === "remote-tracking" (referencing startPoint, remoteRef, upstream, and git.fetch).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/host-service/src/trpc/router/git/utils/git-helpers.ts`:
- Around line 182-187: Update the git diff invocation and parser to use
NUL-delimited name-status: add the "-z" flag to the git.raw call that currently
runs git.raw(["diff", "--name-status", ...diffArgs]) and update the
parseNameStatus function to parse NUL-separated records (split on '\0'),
handling both simple status entries and rename/copy entries which contain
multiple path fields (e.g., status byte followed by one or two paths) rather
than tab/newline delimiters; ensure parseNameStatus mirrors parseNumstat’s
NUL-handling semantics so paths containing tabs/newlines are preserved.
- Around line 111-115: The baseRef construction in the git helper produces
invalid "./branch" when a branch's upstream remote is "."; update the logic in
the function that builds baseRef (the code that uses resolveUpstream and the
variables upstream.remote and upstream.remoteBranch) to treat upstream.remote
=== "." as meaning the local branch name itself (i.e., use upstream.remoteBranch
alone) instead of `${upstream.remote}/${upstream.remoteBranch}`, otherwise use
`${upstream.remote}/${upstream.remoteBranch}`, and keep the fallback
`origin/${branchName}` when no upstream exists.
In `@packages/host-service/test/numstat-parse.test.ts`:
- Around line 1-68: The test file for parseNumstat is placed under
packages/host-service/test instead of co-locating with its source; move the test
file next to the utility file that defines parseNumstat (git-helpers.ts) and
update the import to use the correct relative path to that module (replace
"../src/trpc/router/git/utils/git-helpers" with the local relative path to
git-helpers.ts), ensuring any project test globs still pick up the new location;
keep the test content unchanged.
---
Outside diff comments:
In
`@packages/host-service/src/trpc/router/workspace-creation/workspace-creation.ts`:
- Around line 630-667: The code checks for the existence of remoteRef
(refs/remotes/<remote>/<branch>) before fetching, which fails on a fresh clone;
modify the logic around remoteRef/upstream so you first fetch the explicit
refspec for that upstream (use git.fetch with upstream.remote and an explicit
refspec mapping the remote branch into refs/remotes/<remote>/<branch>) before
running the rev-parse existence check, then set startPoint to the
"remote-tracking" shape when remoteRef is found; also reuse the same explicit
refspec pattern in the later fetch block that handles startPoint.kind ===
"remote-tracking" (referencing startPoint, remoteRef, upstream, and git.fetch).
🪄 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: 352d901f-8db8-4118-be9a-3e2723c501ea
📒 Files selected for processing (6)
packages/host-service/src/runtime/git/index.tspackages/host-service/src/runtime/git/refs.tspackages/host-service/src/trpc/router/git/git.tspackages/host-service/src/trpc/router/git/utils/git-helpers.tspackages/host-service/src/trpc/router/workspace-creation/workspace-creation.tspackages/host-service/test/numstat-parse.test.ts
| const upstream = await resolveUpstream(git, branchName); | ||
| const baseRef = upstream | ||
| ? `${upstream.remote}/${upstream.remoteBranch}` | ||
| : `origin/${branchName}`; | ||
| return { branchName, baseRef }; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
When a Git branch tracks another local branch, does branch..remoteuse the value "." andbranch..mergeuserefs/heads/?
💡 Result:
Yes, when a Git local branch tracks another local branch, the configuration uses branch..remote = . (dot, representing the current local repository) and branch..merge = refs/heads/ (the full reference name of the tracked local branch).
Citations:
- 1: https://mirrors.edge.kernel.org/pub/software/scm/git/docs/git-branch.html
- 2: https://stackoverflow.com/questions/43317872/understanding-git-configs-remote-and-branch-sections
- 3: https://stackoverflow.com/questions/20453399/can-one-create-a-tracking-branch-with-git-branch-rather-than-git-checkout
🏁 Script executed:
# First, let's find and examine the git-helpers.ts file and the resolveUpstream function
fd -t f "git-helpers.ts"Repository: superset-sh/superset
Length of output: 126
🏁 Script executed:
# Look at the file content around the mentioned lines
head -150 packages/host-service/src/trpc/router/git/utils/git-helpers.ts | tail -50Repository: superset-sh/superset
Length of output: 1451
🏁 Script executed:
# Search for the resolveUpstream function definition
rg -n "resolveUpstream" packages/host-service/src/trpc/router/git/utils/ -A 15Repository: superset-sh/superset
Length of output: 3032
🏁 Script executed:
# Check what format remoteBranch is in and how baseRef is used downstream
rg -n "baseRef" packages/host-service/src/trpc/router/git/utils/ -B 3 -A 5Repository: superset-sh/superset
Length of output: 1653
🏁 Script executed:
# Find the resolveUpstream function in the runtime/git/refs directory
fd -t f "refs.ts" packages/runtime/Repository: superset-sh/superset
Length of output: 168
🏁 Script executed:
# Search for resolveUpstream function definition
rg -n "resolveUpstream" packages/runtime/ -A 20Repository: superset-sh/superset
Length of output: 121
🏁 Script executed:
# Let's find the actual location of resolveUpstream
rg -n "export.*resolveUpstream" --type tsRepository: superset-sh/superset
Length of output: 152
🏁 Script executed:
# Let's also check the directory structure to understand the path better
find packages -type d -name "runtime" 2>/dev/null | head -20Repository: superset-sh/superset
Length of output: 141
🏁 Script executed:
# Look at the git-helpers.ts imports more carefully
head -20 packages/host-service/src/trpc/router/git/utils/git-helpers.tsRepository: superset-sh/superset
Length of output: 593
🏁 Script executed:
# Now let's examine the resolveUpstream function implementation
sed -n '151,200p' packages/host-service/src/runtime/git/refs.tsRepository: superset-sh/superset
Length of output: 606
🏁 Script executed:
# Let's get more context about what resolveUpstream returns
sed -n '140,210p' packages/host-service/src/runtime/git/refs.tsRepository: superset-sh/superset
Length of output: 849
🏁 Script executed:
# Now let's check how baseRef is used downstream to confirm the proposed fix is correct
rg -n "baseRef" packages/host-service/src/trpc/router/git/utils/ -B 2 -A 8Repository: superset-sh/superset
Length of output: 1803
🏁 Script executed:
# Check if baseRef is used in any git commands (rev-list, diff, log, etc.)
rg -n "baseRef" packages/host-service/src/ -B 2 -A 8Repository: superset-sh/superset
Length of output: 10701
Handle Git's local-upstream remote value.
When a local branch tracks another local branch, branch.<name>.remote is "." and the current code produces ./branchname for baseRef, which is invalid for git diff/log/show commands. Map "." to use the branch name directly instead.
Proposed fix
const upstream = await resolveUpstream(git, branchName);
const baseRef = upstream
- ? `${upstream.remote}/${upstream.remoteBranch}`
+ ? upstream.remote === "."
+ ? upstream.remoteBranch
+ : `${upstream.remote}/${upstream.remoteBranch}`
: `origin/${branchName}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/host-service/src/trpc/router/git/utils/git-helpers.ts` around lines
111 - 115, The baseRef construction in the git helper produces invalid
"./branch" when a branch's upstream remote is "."; update the logic in the
function that builds baseRef (the code that uses resolveUpstream and the
variables upstream.remote and upstream.remoteBranch) to treat upstream.remote
=== "." as meaning the local branch name itself (i.e., use upstream.remoteBranch
alone) instead of `${upstream.remote}/${upstream.remoteBranch}`, otherwise use
`${upstream.remote}/${upstream.remoteBranch}`, and keep the fallback
`origin/${branchName}` when no upstream exists.
| import { describe, expect, test } from "bun:test"; | ||
| import { parseNumstat } from "../src/trpc/router/git/utils/git-helpers"; | ||
|
|
||
| describe("parseNumstat", () => { | ||
| test("regular file entry", () => { | ||
| const raw = "5\t2\tsrc/foo.ts\0"; | ||
| const result = parseNumstat(raw); | ||
| expect(result.get("src/foo.ts")).toEqual({ additions: 5, deletions: 2 }); | ||
| }); | ||
|
|
||
| test("multiple regular entries", () => { | ||
| const raw = "5\t2\tsrc/foo.ts\x003\t0\tsrc/bar.ts\x00"; | ||
| const result = parseNumstat(raw); | ||
| expect(result.get("src/foo.ts")).toEqual({ additions: 5, deletions: 2 }); | ||
| expect(result.get("src/bar.ts")).toEqual({ additions: 3, deletions: 0 }); | ||
| }); | ||
|
|
||
| test("exact rename with edits indexes both paths", () => { | ||
| const raw = "4\t3\t\x00src/old.ts\x00src/new.ts\x00"; | ||
| const result = parseNumstat(raw); | ||
| expect(result.get("src/new.ts")).toEqual({ additions: 4, deletions: 3 }); | ||
| expect(result.get("src/old.ts")).toEqual({ additions: 4, deletions: 3 }); | ||
| }); | ||
|
|
||
| test("pure rename with zero line changes", () => { | ||
| const raw = "0\t0\t\x00src/old.ts\x00src/new.ts\x00"; | ||
| const result = parseNumstat(raw); | ||
| expect(result.get("src/new.ts")).toEqual({ additions: 0, deletions: 0 }); | ||
| expect(result.get("src/old.ts")).toEqual({ additions: 0, deletions: 0 }); | ||
| }); | ||
|
|
||
| test("binary file with dash markers", () => { | ||
| const raw = "-\t-\tassets/image.png\0"; | ||
| const result = parseNumstat(raw); | ||
| expect(result.get("assets/image.png")).toEqual({ | ||
| additions: 0, | ||
| deletions: 0, | ||
| }); | ||
| }); | ||
|
|
||
| test("mixed regular, rename, and binary", () => { | ||
| const raw = | ||
| "5\t2\tsrc/foo.ts\x00" + | ||
| "4\t3\t\x00src/old.ts\x00src/new.ts\x00" + | ||
| "-\t-\tassets/image.png\x00"; | ||
| const result = parseNumstat(raw); | ||
| expect(result.get("src/foo.ts")).toEqual({ additions: 5, deletions: 2 }); | ||
| expect(result.get("src/new.ts")).toEqual({ additions: 4, deletions: 3 }); | ||
| expect(result.get("src/old.ts")).toEqual({ additions: 4, deletions: 3 }); | ||
| expect(result.get("assets/image.png")).toEqual({ | ||
| additions: 0, | ||
| deletions: 0, | ||
| }); | ||
| }); | ||
|
|
||
| test("empty input returns empty map", () => { | ||
| expect(parseNumstat("")).toEqual(new Map()); | ||
| }); | ||
|
|
||
| test("path containing tab is preserved as-is", () => { | ||
| const raw = "1\t1\tweird\tpath.ts\0"; | ||
| const result = parseNumstat(raw); | ||
| expect(result.get("weird\tpath.ts")).toEqual({ | ||
| additions: 1, | ||
| deletions: 1, | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Move this test next to git-helpers.ts.
The coverage is good, but this new test lives under packages/host-service/test instead of the utility’s directory. As per coding guidelines, **/*.test.{ts,tsx}: Co-locate tests with their source files; tests should be in the same directory as the component/utility they test.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/host-service/test/numstat-parse.test.ts` around lines 1 - 68, The
test file for parseNumstat is placed under packages/host-service/test instead of
co-locating with its source; move the test file next to the utility file that
defines parseNumstat (git-helpers.ts) and update the import to use the correct
relative path to that module (replace "../src/trpc/router/git/utils/git-helpers"
with the local relative path to git-helpers.ts), ensuring any project test globs
still pick up the new location; keep the test content unchanged.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Greptile SummaryThis PR fixes four related correctness bugs in the v2 workspace git layer: new workspaces were forking off stale local
Confidence Score: 4/5Safe to merge after fixing the --name-status / --numstat -z quoting mismatch for non-ASCII filenames; all other changes are correct and well-tested. Four concrete user-facing bugs are correctly fixed with good test coverage for the new parser. The one P1 finding (non-ASCII path quoting regression in getChangedFilesForDiff) is a targeted, self-contained fix. All other logic is sound, the workspace-creation upstream-swap is correct, and the 3-dot diff change is the right semantic. Score is 4 rather than 5 because the regression is a silent correctness failure (returns 0 instead of an error) that could affect real codebases with non-ASCII filenames. packages/host-service/src/trpc/router/git/utils/git-helpers.ts — specifically the getChangedFilesForDiff function where --name-status lacks the -z flag that --numstat now uses.
|
| Filename | Overview |
|---|---|
| packages/host-service/src/trpc/router/git/utils/git-helpers.ts | Central fix file: rewrites parseNumstat for NUL-delimited rename handling (correct), adds resolveBaseComparison helper, and switches getChangedFilesForDiff to --numstat -z; introduces a path-quoting mismatch with --name-status for non-ASCII filenames that causes additions/deletions to silently drop to 0. |
| packages/host-service/src/runtime/git/refs.ts | Adds resolveUpstream that reads branch.<name>.remote / .merge via parallel git-config calls; implementation is clean, handles missing config by returning null, and exported correctly through the index. |
| packages/host-service/src/trpc/router/git/git.ts | All four git procedures (listBranches, getStatus, listCommits, getDiff) consistently migrated to resolveBaseComparison; 2-dot diff corrected to 3-dot in getStatus; staged/unstaged numstat correctly uses -z since lookups come from git.status() (unquoted paths). |
| packages/host-service/src/trpc/router/workspace-creation/workspace-creation.ts | Adds upstream-swap logic for local default-branch start points: fetches from upstream remote if the remote-tracking ref is cached locally, otherwise falls back to the stale local ref; logic is sound and handles offline/uncached scenarios correctly. |
| packages/host-service/test/numstat-parse.test.ts | New test file with 8 cases covering regular, rename, binary, mixed, empty, and tab-in-path inputs; tests correctly model the --numstat -z wire format; missing a tab-in-path rename case but otherwise solid. |
| packages/host-service/src/runtime/git/index.ts | Trivial re-export of new resolveUpstream from refs; no issues. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[workspace creation / git router call] --> B{startPoint kind?}
B -- local --> C{shortName == defaultBranch?}
C -- yes --> D[resolveUpstream git, branchName]
D -- upstream found --> E{remoteRef exists locally?}
E -- yes --> F[swap startPoint to remote-tracking]
F --> G[fetch from upstream remote]
G -- ok --> H[worktree from upstream tip ✓]
G -- fail/offline --> H
E -- no --> I[keep local ref]
C -- no --> I
B -- remote-tracking --> G
I --> J[worktree from local ref]
A2[getStatus / listBranches / listCommits / getDiff] --> K[resolveBaseComparison git, explicitBranch?]
K --> L[getDefaultBranchName git]
L --> M[resolveUpstream git, branchName]
M -- upstream configured --> N[baseRef = remote/remoteBranch]
M -- no upstream --> O[baseRef = origin/branchName fallback]
N --> P[3-dot diff: baseRef...HEAD]
O --> P
P --> Q[getChangedFilesForDiff]
Q --> R[--name-status no -z may quote non-ASCII]
Q --> S[--numstat -z raw UTF-8]
R --> T[parseNameStatus path keys]
S --> U[parseNumstat indexes old and new paths]
T --> V{numstat.get path}
U --> V
V -- ASCII path --> W[additions/deletions correct]
V -- non-ASCII path --> X[lookup miss returns 0]
Prompt To Fix All With AI
This is a comment left during a code review.
Path: packages/host-service/src/trpc/router/git/utils/git-helpers.ts
Line: 181-187
Comment:
**Non-ASCII path quoting mismatch between `--name-status` and `--numstat -z`**
`--name-status` (without `-z`) respects `core.quotePath` (default `true`) and outputs non-ASCII pathnames as octal escape sequences — e.g. `ñ.ts` becomes `"\303\261.ts"`. `--numstat -z` suppresses all quoting and outputs raw UTF-8. For any file with a non-ASCII name (common in CJK or non-English codebases), the lookup `numstat.get(f.path)` will fail because the map key is the raw byte string while the name-status key is the escape-quoted form — `additions` and `deletions` silently fall back to `0`.
This is a regression: before the PR both commands operated without `-z`, so their quoting matched for regular files. Now only `--numstat` uses `-z`.
The consistent fix is to add `-z` to the `--name-status` invocation too and update `parseNameStatus` to split on `\0` instead of `\n`:
```typescript
const [nameStatusRaw, numstatRaw] = await Promise.all([
git.raw(["diff", "--name-status", "-z", ...diffArgs]),
git.raw(["diff", "--numstat", "-z", ...diffArgs]),
]);
```
`parseNameStatus` would then need to split each `\0`-separated record on `\t` (same structure, different line delimiter).
The same mismatch exists for the `stagedNumstat` / `unstagedNumstat` paths in `git.ts` (lines 114 and 135), but those read `file.path` from `git.status()` which already returns unquoted UTF-8, so they are unaffected — the problem is isolated to `getChangedFilesForDiff`.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/host-service/src/trpc/router/git/utils/git-helpers.ts
Line: 105-116
Comment:
**`resolveBaseComparison` calls `resolveUpstream` on the explicitly-supplied branch name**
When `explicitBranch` is set (e.g. `input.baseBranch = "main"`), the function looks up the upstream tracking config for that branch. If the caller passes an arbitrary feature branch whose local tracking points at a different remote (e.g. a user's fork), the `baseRef` will silently use that fork remote instead of `origin/<branch>`, which may not be what the caller intended.
More practically: if the branch name is passed as a remote-qualified string like `"origin/main"`, `resolveUpstream(git, "origin/main")` returns `null` and the fallback becomes `origin/origin/main` — same broken behavior as the old code. Consider documenting that `explicitBranch` must be a bare local branch name, or add a guard:
```typescript
const upstream =
explicitBranch ? null : await resolveUpstream(git, branchName);
```
This preserves upstream-aware behavior for the auto-resolved default branch while keeping the `origin/<name>` fallback for callers that explicitly name a branch.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/host-service/test/numstat-parse.test.ts
Line: 60-67
Comment:
**Tab-in-path test only covers regular files, not renames**
The test verifies that a tab embedded in a regular file's path is preserved. It would be worth adding a parallel case for a renamed file with a tab in both source and destination paths, since the rename branch (`pathMaybe === ""`) consumes the next two entries. Without a test, a regression in that path won't be caught. Example:
```typescript
test("rename where both paths contain tabs", () => {
const raw = "2\t1\t\x00weird\told.ts\x00weird\tnew.ts\x00";
const result = parseNumstat(raw);
expect(result.get("weird\told.ts")).toEqual({ additions: 2, deletions: 1 });
expect(result.get("weird\tnew.ts")).toEqual({ additions: 2, deletions: 1 });
});
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(host-service): correct additions/del..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| try { | ||
| const [nameStatusRaw, numstatRaw] = await Promise.all([ | ||
| git.raw(["diff", "--name-status", ...diffArgs]), | ||
| git.raw(["diff", "--numstat", ...diffArgs]), | ||
| git.raw(["diff", "--numstat", "-z", ...diffArgs]), | ||
| ]); | ||
| const nameStatus = parseNameStatus(nameStatusRaw); | ||
| const numstat = parseNumstat(numstatRaw); |
There was a problem hiding this comment.
--name-status and --numstat -z
--name-status (without -z) respects core.quotePath (default true) and outputs non-ASCII pathnames as octal escape sequences — e.g. ñ.ts becomes "\303\261.ts". --numstat -z suppresses all quoting and outputs raw UTF-8. For any file with a non-ASCII name (common in CJK or non-English codebases), the lookup numstat.get(f.path) will fail because the map key is the raw byte string while the name-status key is the escape-quoted form — additions and deletions silently fall back to 0.
This is a regression: before the PR both commands operated without -z, so their quoting matched for regular files. Now only --numstat uses -z.
The consistent fix is to add -z to the --name-status invocation too and update parseNameStatus to split on \0 instead of \n:
const [nameStatusRaw, numstatRaw] = await Promise.all([
git.raw(["diff", "--name-status", "-z", ...diffArgs]),
git.raw(["diff", "--numstat", "-z", ...diffArgs]),
]);parseNameStatus would then need to split each \0-separated record on \t (same structure, different line delimiter).
The same mismatch exists for the stagedNumstat / unstagedNumstat paths in git.ts (lines 114 and 135), but those read file.path from git.status() which already returns unquoted UTF-8, so they are unaffected — the problem is isolated to getChangedFilesForDiff.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/src/trpc/router/git/utils/git-helpers.ts
Line: 181-187
Comment:
**Non-ASCII path quoting mismatch between `--name-status` and `--numstat -z`**
`--name-status` (without `-z`) respects `core.quotePath` (default `true`) and outputs non-ASCII pathnames as octal escape sequences — e.g. `ñ.ts` becomes `"\303\261.ts"`. `--numstat -z` suppresses all quoting and outputs raw UTF-8. For any file with a non-ASCII name (common in CJK or non-English codebases), the lookup `numstat.get(f.path)` will fail because the map key is the raw byte string while the name-status key is the escape-quoted form — `additions` and `deletions` silently fall back to `0`.
This is a regression: before the PR both commands operated without `-z`, so their quoting matched for regular files. Now only `--numstat` uses `-z`.
The consistent fix is to add `-z` to the `--name-status` invocation too and update `parseNameStatus` to split on `\0` instead of `\n`:
```typescript
const [nameStatusRaw, numstatRaw] = await Promise.all([
git.raw(["diff", "--name-status", "-z", ...diffArgs]),
git.raw(["diff", "--numstat", "-z", ...diffArgs]),
]);
```
`parseNameStatus` would then need to split each `\0`-separated record on `\t` (same structure, different line delimiter).
The same mismatch exists for the `stagedNumstat` / `unstagedNumstat` paths in `git.ts` (lines 114 and 135), but those read `file.path` from `git.status()` which already returns unquoted UTF-8, so they are unaffected — the problem is isolated to `getChangedFilesForDiff`.
How can I resolve this? If you propose a fix, please make it concise.| export async function resolveBaseComparison( | ||
| git: SimpleGit, | ||
| explicitBranch?: string, | ||
| ): Promise<{ branchName: string; baseRef: string } | null> { | ||
| const branchName = explicitBranch ?? (await getDefaultBranchName(git)); | ||
| if (!branchName) return null; | ||
| const upstream = await resolveUpstream(git, branchName); | ||
| const baseRef = upstream | ||
| ? `${upstream.remote}/${upstream.remoteBranch}` | ||
| : `origin/${branchName}`; | ||
| return { branchName, baseRef }; | ||
| } |
There was a problem hiding this comment.
resolveBaseComparison calls resolveUpstream on the explicitly-supplied branch name
When explicitBranch is set (e.g. input.baseBranch = "main"), the function looks up the upstream tracking config for that branch. If the caller passes an arbitrary feature branch whose local tracking points at a different remote (e.g. a user's fork), the baseRef will silently use that fork remote instead of origin/<branch>, which may not be what the caller intended.
More practically: if the branch name is passed as a remote-qualified string like "origin/main", resolveUpstream(git, "origin/main") returns null and the fallback becomes origin/origin/main — same broken behavior as the old code. Consider documenting that explicitBranch must be a bare local branch name, or add a guard:
const upstream =
explicitBranch ? null : await resolveUpstream(git, branchName);This preserves upstream-aware behavior for the auto-resolved default branch while keeping the origin/<name> fallback for callers that explicitly name a branch. Was the intent for resolveBaseComparison to honor upstream tracking even when the caller explicitly passes a branch name? The current behavior may surprise callers that pass input.baseBranch expecting a plain origin/<name> comparison.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/src/trpc/router/git/utils/git-helpers.ts
Line: 105-116
Comment:
**`resolveBaseComparison` calls `resolveUpstream` on the explicitly-supplied branch name**
When `explicitBranch` is set (e.g. `input.baseBranch = "main"`), the function looks up the upstream tracking config for that branch. If the caller passes an arbitrary feature branch whose local tracking points at a different remote (e.g. a user's fork), the `baseRef` will silently use that fork remote instead of `origin/<branch>`, which may not be what the caller intended.
More practically: if the branch name is passed as a remote-qualified string like `"origin/main"`, `resolveUpstream(git, "origin/main")` returns `null` and the fallback becomes `origin/origin/main` — same broken behavior as the old code. Consider documenting that `explicitBranch` must be a bare local branch name, or add a guard:
```typescript
const upstream =
explicitBranch ? null : await resolveUpstream(git, branchName);
```
This preserves upstream-aware behavior for the auto-resolved default branch while keeping the `origin/<name>` fallback for callers that explicitly name a branch. Was the intent for `resolveBaseComparison` to honor upstream tracking even when the caller explicitly passes a branch name? The current behavior may surprise callers that pass `input.baseBranch` expecting a plain `origin/<name>` comparison.
How can I resolve this? If you propose a fix, please make it concise.| test("path containing tab is preserved as-is", () => { | ||
| const raw = "1\t1\tweird\tpath.ts\0"; | ||
| const result = parseNumstat(raw); | ||
| expect(result.get("weird\tpath.ts")).toEqual({ | ||
| additions: 1, | ||
| deletions: 1, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The test verifies that a tab embedded in a regular file's path is preserved. It would be worth adding a parallel case for a renamed file with a tab in both source and destination paths, since the rename branch (pathMaybe === "") consumes the next two entries. Without a test, a regression in that path won't be caught. Example:
test("rename where both paths contain tabs", () => {
const raw = "2\t1\t\x00weird\told.ts\x00weird\tnew.ts\x00";
const result = parseNumstat(raw);
expect(result.get("weird\told.ts")).toEqual({ additions: 2, deletions: 1 });
expect(result.get("weird\tnew.ts")).toEqual({ additions: 2, deletions: 1 });
});Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/test/numstat-parse.test.ts
Line: 60-67
Comment:
**Tab-in-path test only covers regular files, not renames**
The test verifies that a tab embedded in a regular file's path is preserved. It would be worth adding a parallel case for a renamed file with a tab in both source and destination paths, since the rename branch (`pathMaybe === ""`) consumes the next two entries. Without a test, a regression in that path won't be caught. Example:
```typescript
test("rename where both paths contain tabs", () => {
const raw = "2\t1\t\x00weird\told.ts\x00weird\tnew.ts\x00";
const result = parseNumstat(raw);
expect(result.get("weird\told.ts")).toEqual({ additions: 2, deletions: 1 });
expect(result.get("weird\tnew.ts")).toEqual({ additions: 2, deletions: 1 });
});
```
How can I resolve this? If you propose a fix, please make it concise.
| * Resolve a local branch's upstream tracking info (`branch.<name>.remote` | ||
| * / `branch.<name>.merge`). Returns `null` if no upstream is configured. | ||
| */ | ||
| export async function resolveUpstream( |
There was a problem hiding this comment.
i think we actually have this already? can we check or its actually in my open PR that you're reviewing
There was a problem hiding this comment.
resolveUpstream is new in this PR — grep shows the only definition is in runtime/git/refs.ts. Happy to move/consolidate if #3548 or another open PR introduced its own version.
| import { describe, expect, test } from "bun:test"; | ||
| import { parseNumstat } from "../src/trpc/router/git/utils/git-helpers"; | ||
|
|
||
| describe("parseNumstat", () => { |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/docs/V2_WORKSPACE_DIFF_VIEWS.md`:
- Around line 19-23: The fenced code block containing the ASCII branch diagram
("main: A — B — C — D — E" / "your branch: X — Y — Z") is missing a language
identifier; update the opening fence from ``` to ```text so the block becomes a
text-coded fence (leave the closing fence unchanged) to satisfy the markdown
linter.
🪄 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: 006e0cc3-a60d-4037-b7ba-c30254766949
📒 Files selected for processing (2)
apps/desktop/docs/V2_WORKSPACE_DIFF_VIEWS.mdpackages/host-service/src/trpc/router/git/git.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/host-service/src/trpc/router/git/git.ts
New v2 workspaces branched off the user's local default branch, which is rarely fast-forwarded, so the worktree started several commits behind upstream. Now resolves the default branch's configured upstream (`branch.<name>.remote` / `.merge`) and branches off that — honoring fork workflows where `origin` is the fork and `upstream` is the canonical repo. Also swap the "changes vs base" diff from 2-dot (`origin/main HEAD`) to 3-dot (`origin/main...HEAD`) so the count reflects changes on the branch since divergence — not symmetric diff that grows as main advances.
…uter getStatus, listBranches, listCommits, and getDiff all compared against a hardcoded origin/<default>, so fork-workflow users (origin = fork, upstream = canonical) saw counts reflecting the fork's main — often drastically wrong. Now each call site uses resolveBaseComparison, which reads the local default branch's configured upstream first and only falls back to origin/<default>.
parseNumstat previously indexed by the raw numstat path string, which for renames is `<old> => <new>` (or brace-form with a common prefix). Callers looked up by the destination path from --name-status, which never matched, so every renamed file showed +0 -0 in the sidebar and Changes tab. Switch all numstat call sites to --numstat -z, rewrite parseNumstat to split on NUL and recognize the three-cell rename record, index both source and destination paths, and add coverage for the new format.
The per-file diff view was comparing content at the current base tip vs HEAD. For a file both your branch and main happened to touch unrelatedly, that showed main's unrelated edits as if your branch had undone them. Now we find the fork point via git merge-base and show the file's content there, matching the file list's three-dot filtering and what GitHub's Files-changed view renders. Also adds apps/desktop/docs/V2_WORKSPACE_DIFF_VIEWS.md — a plain-English explainer of every place v2 shows "what changed" and which comparison runs under it.
…integration tests Three correctness fixes surfaced in PR review plus real-repo coverage: - --name-status now passes -z as well, matching --numstat -z. Without this, non-ASCII filenames were quoted as octal escapes in name-status but raw in numstat -z — numstat.get(f.path) never matched, so every non-ASCII file silently reported +0 -0. - resolveBaseComparison handles branch.<name>.remote = "." (a local branch that tracks another local branch). Previously produced invalid ./<branch> refs; now uses the bare branch name. - parseNameStatus rewritten to consume the -z NUL-delimited record format (status, then 1-or-2 paths per record). Tests: - Moved parseNumstat tests next to git-helpers.ts per co-location rule, added parseNumstat rename-with-tabs + non-ASCII cases, added parseNameStatus coverage (regular, rename, copy, non-ASCII, tab, mixed, empty). - Added git-helpers.integration.test.ts — spins up real on-disk repos to exercise resolveUpstream, resolveBaseComparison, getDefaultBranchName, and getChangedFilesForDiff end-to-end. Covers fork workflow, local-tracking, non-ASCII filenames, renames, binary files, and the 3-dot semantic guarantee. Docs: - Moved V2_WORKSPACE_DIFF_VIEWS.md to root docs/ (general reference). - Added text language tag to the ASCII diagram fence.
saddlepaddle
force-pushed
the
fix/v2-workspace-stale-base-and-diff
branch
from
d00f067 to
e3a716e
Compare
|
Addressed review feedback on top: Real correctness bugs caught by reviewers (fixed):
Test coverage (Kiet's ask):
Full run: 161/161 host-service tests pass (112 pre-existing + 49 new). Housekeeping:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/host-service/src/trpc/router/workspace-creation/workspace-creation.ts`:
- Around line 766-798: When resolving the default branch upstream in
workspace-creation.ts (inside the block using resolveDefaultBranchName,
resolveUpstream, asRemoteRef and computing remoteExists via git.raw), add
handling for upstream.remote === ".": instead of calling asRemoteRef for ".",
verify the local branch target directly (git rev-parse --verify --quiet
refs/heads/${upstream.remoteBranch}^{commit}); if that commit exists, set
startPoint to a local ref (kind: "local", shortName: upstream.remoteBranch,
fullRef: `refs/heads/${upstream.remoteBranch}` or equivalent) so we honor a
local-tracking upstream; otherwise fall back to the existing remote-tracking
logic.
🪄 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: 097ef01c-f0f2-48ae-b6db-ecd973901305
📒 Files selected for processing (8)
docs/V2_WORKSPACE_DIFF_VIEWS.mdpackages/host-service/src/runtime/git/index.tspackages/host-service/src/runtime/git/refs.tspackages/host-service/src/trpc/router/git/git.tspackages/host-service/src/trpc/router/git/utils/git-helpers.integration.test.tspackages/host-service/src/trpc/router/git/utils/git-helpers.test.tspackages/host-service/src/trpc/router/git/utils/git-helpers.tspackages/host-service/src/trpc/router/workspace-creation/workspace-creation.ts
✅ Files skipped from review due to trivial changes (2)
- packages/host-service/src/runtime/git/index.ts
- docs/V2_WORKSPACE_DIFF_VIEWS.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/host-service/src/runtime/git/refs.ts
- packages/host-service/src/trpc/router/git/git.ts
| // Local default branches are rarely fast-forwarded; swap to the | ||
| // branch's configured upstream so we fork from the real tip, not a | ||
| // stale local ref. Non-default branches stay local-first by design. | ||
| if (startPoint.kind === "local") { | ||
| const defaultBranchName = await resolveDefaultBranchName(git); | ||
| if (startPoint.shortName === defaultBranchName) { | ||
| const upstream = await resolveUpstream(git, defaultBranchName); | ||
| if (upstream) { | ||
| const remoteRef = asRemoteRef( | ||
| upstream.remote, | ||
| upstream.remoteBranch, | ||
| ); | ||
| const remoteExists = await git | ||
| .raw([ | ||
| "rev-parse", | ||
| "--verify", | ||
| "--quiet", | ||
| `${remoteRef}^{commit}`, | ||
| ]) | ||
| .then(() => true) | ||
| .catch(() => false); | ||
| if (remoteExists) { | ||
| startPoint = { | ||
| kind: "remote-tracking", | ||
| fullRef: remoteRef, | ||
| shortName: upstream.remoteBranch, | ||
| remote: upstream.remote, | ||
| remoteShortName: `${upstream.remote}/${upstream.remoteBranch}`, | ||
| }; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Mirror the remote === "." handling from resolveBaseComparison.
If the local default branch is configured to track another local branch (branch.main.remote = "."), asRemoteRef(".", upstream.remoteBranch) yields refs/remotes/./<name>, which is never a valid ref. rev-parse fails, remoteExists is false, and the code silently falls back to the stale local ref — defeating the purpose of this whole block, and inconsistent with the . handling that resolveBaseComparison just added in git-helpers.ts.
In practice this is an unusual setup, but worth handling symmetrically so the "fork from the real tip" intent is honored there too.
🔧 Proposed fix
if (startPoint.shortName === defaultBranchName) {
const upstream = await resolveUpstream(git, defaultBranchName);
- if (upstream) {
+ if (upstream && upstream.remote !== ".") {
const remoteRef = asRemoteRef(
upstream.remote,
upstream.remoteBranch,
);If you do want to honor local-tracking upstreams here as well, the alternative is to resolve the local branch target directly (e.g., verify refs/heads/${upstream.remoteBranch}^{commit} and build a kind: "local" ref) instead of forcing it through the remote-tracking branch.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Local default branches are rarely fast-forwarded; swap to the | |
| // branch's configured upstream so we fork from the real tip, not a | |
| // stale local ref. Non-default branches stay local-first by design. | |
| if (startPoint.kind === "local") { | |
| const defaultBranchName = await resolveDefaultBranchName(git); | |
| if (startPoint.shortName === defaultBranchName) { | |
| const upstream = await resolveUpstream(git, defaultBranchName); | |
| if (upstream) { | |
| const remoteRef = asRemoteRef( | |
| upstream.remote, | |
| upstream.remoteBranch, | |
| ); | |
| const remoteExists = await git | |
| .raw([ | |
| "rev-parse", | |
| "--verify", | |
| "--quiet", | |
| `${remoteRef}^{commit}`, | |
| ]) | |
| .then(() => true) | |
| .catch(() => false); | |
| if (remoteExists) { | |
| startPoint = { | |
| kind: "remote-tracking", | |
| fullRef: remoteRef, | |
| shortName: upstream.remoteBranch, | |
| remote: upstream.remote, | |
| remoteShortName: `${upstream.remote}/${upstream.remoteBranch}`, | |
| }; | |
| } | |
| } | |
| } | |
| } | |
| // Local default branches are rarely fast-forwarded; swap to the | |
| // branch's configured upstream so we fork from the real tip, not a | |
| // stale local ref. Non-default branches stay local-first by design. | |
| if (startPoint.kind === "local") { | |
| const defaultBranchName = await resolveDefaultBranchName(git); | |
| if (startPoint.shortName === defaultBranchName) { | |
| const upstream = await resolveUpstream(git, defaultBranchName); | |
| if (upstream && upstream.remote !== ".") { | |
| const remoteRef = asRemoteRef( | |
| upstream.remote, | |
| upstream.remoteBranch, | |
| ); | |
| const remoteExists = await git | |
| .raw([ | |
| "rev-parse", | |
| "--verify", | |
| "--quiet", | |
| `${remoteRef}^{commit}`, | |
| ]) | |
| .then(() => true) | |
| .catch(() => false); | |
| if (remoteExists) { | |
| startPoint = { | |
| kind: "remote-tracking", | |
| fullRef: remoteRef, | |
| shortName: upstream.remoteBranch, | |
| remote: upstream.remote, | |
| remoteShortName: `${upstream.remote}/${upstream.remoteBranch}`, | |
| }; | |
| } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/host-service/src/trpc/router/workspace-creation/workspace-creation.ts`
around lines 766 - 798, When resolving the default branch upstream in
workspace-creation.ts (inside the block using resolveDefaultBranchName,
resolveUpstream, asRemoteRef and computing remoteExists via git.raw), add
handling for upstream.remote === ".": instead of calling asRemoteRef for ".",
verify the local branch target directly (git rev-parse --verify --quiet
refs/heads/${upstream.remoteBranch}^{commit}); if that commit exists, set
startPoint to a local ref (kind: "local", shortName: upstream.remoteBranch,
fullRef: `refs/heads/${upstream.remoteBranch}` or equivalent) so we honor a
local-tracking upstream; otherwise fall back to the existing remote-tracking
logic.
Reorganizes test coverage to mirror docs/V2_WORKSPACE_DIFF_VIEWS.md. Each surface in the doc (A: workspace creation, B/C: sidebar + Changes tab counts, D: per-file diff view, F: ahead/behind counts, Renames) now has a matching describe block in src/trpc/router/git/v2-diff-surfaces.integration.test.ts that spins up real repos and verifies the described behavior end-to-end. A reviewer can trace each doc section to a passing test without having to hunt through helper-level test cases. The function-level integration tests in git-helpers.integration.test.ts remain as lower-level coverage. Added a pointer in the doc so readers land on the test file directly.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/host-service/src/trpc/router/git/v2-diff-surfaces.integration.test.ts">
<violation number="1" location="packages/host-service/src/trpc/router/git/v2-diff-surfaces.integration.test.ts:89">
P2: Deriving the worktree name with `repo.split("/").pop()` breaks on Windows paths. Use a cross-platform basename instead.
(Based on your team's feedback about using cross-platform path utilities instead of split.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| expect(upstream).toEqual({ remote: "origin", remoteBranch: "main" }); | ||
| const startRef = `${upstream?.remote}/${upstream?.remoteBranch}`; | ||
|
|
||
| const worktreePath = join(repo, "..", `${repo.split("/").pop()}-wt`); |
There was a problem hiding this comment.
P2: Deriving the worktree name with repo.split("/").pop() breaks on Windows paths. Use a cross-platform basename instead.
(Based on your team's feedback about using cross-platform path utilities instead of split.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/host-service/src/trpc/router/git/v2-diff-surfaces.integration.test.ts, line 89:
<comment>Deriving the worktree name with `repo.split("/").pop()` breaks on Windows paths. Use a cross-platform basename instead.
(Based on your team's feedback about using cross-platform path utilities instead of split.) </comment>
<file context>
@@ -0,0 +1,413 @@
+ expect(upstream).toEqual({ remote: "origin", remoteBranch: "main" });
+ const startRef = `${upstream?.remote}/${upstream?.remoteBranch}`;
+
+ const worktreePath = join(repo, "..", `${repo.split("/").pop()}-wt`);
+ await git.raw([
+ "worktree",
</file context>
|
|
||
| An explainer for every place the UI shows "what changed" for a workspace, what comparison runs under the hood, and why. | ||
|
|
||
| Written for people who don't know git. If you do know git: this doc maps each UI surface to the specific git command and explains why we picked that comparison over the alternatives. |
There was a problem hiding this comment.
How condescending, i know git and i don't understand anything either >:(
There was a problem hiding this comment.
Fair — reworded to drop the framing, now just "maps each UI surface to the git command it runs".
| * cells — and are indexed under both source and destination paths so | ||
| * callers keyed by either get a hit. | ||
| */ | ||
| export function parseNumstat( |
There was a problem hiding this comment.
we should also use simple git instead of raw git yeah? if it has an equivalent
There was a problem hiding this comment.
simple-git's diffSummary() tokenizes on whitespace and doesn't expose a -z / NUL-delimited equivalent, so it breaks on filenames with spaces/quotes/newlines (and pairs name-status with numstat ambiguously for renames). Falling back to git.raw([...-z]) is the reason this parser exists — switching to the simple-git helpers would regress the non-ASCII path coverage the tests added in this PR cover.
| "rev-parse", | ||
| "--verify", | ||
| "--quiet", | ||
| `${remoteRef}^{commit}`, |
There was a problem hiding this comment.
I think we moved away from ^{commit} in general because its buggy, --no-track would be better, check #3548
There was a problem hiding this comment.
Different use site from #3548 — there we pass ${ref}^{commit} as a worktree start-point (where --no-track is the right swap). Here we use it inside rev-parse --verify --quiet purely to test whether a remote-tracking ref resolves before preferring it over the local default. For a branch ref the ^{commit} suffix is a no-op, so dropped it.
| // Local default branches are rarely fast-forwarded; swap to the | ||
| // branch's configured upstream so we fork from the real tip, not a | ||
| // stale local ref. Non-default branches stay local-first by design. | ||
| if (startPoint.kind === "local") { |
There was a problem hiding this comment.
nit: consider refactor into a named function for readability onto what this block is doing instead of describing in comments
There was a problem hiding this comment.
Good call — extracted as preferUpstreamForLocalDefault(git, startPoint) so the caller reads as a single step and the early-return conditions are named.
| // Staged — use status.files index character for correct status | ||
| const stagedNumstat = parseNumstat( | ||
| await git.raw(["diff", "--numstat", "--cached"]).catch(() => ""), | ||
| await git.raw(["diff", "--numstat", "-z", "--cached"]).catch(() => ""), |
There was a problem hiding this comment.
-z makes git output NUL-terminated records with raw (unquoted) paths. Without it, paths containing spaces/quotes/non-ASCII bytes come back as "..." with C-style escapes, which parseNumstat would have to re-decode — and rename pairs become ambiguous to line-split. -z also gives us the 3-cell shape for renames that parseNumstat keys on. Same reason it's used in the nameStatus/numstat reads in git-helpers.
- DashboardSidebarWorkspaceItem: hostIsOnline / onCopyBranchName / branch を 子と hook に渡すように修正。子コンポーネントが upstream 仕様で required になっていた 3 つの prop を親が destructuring / 伝播し忘れていたため。 - host-service/listBranches: feat(desktop): VS Code-style branch sort order (184c061) で追加された sortOrder / pinDefault 入力、および remote-only branch 追加 + pin default 処理を復元。upstream superset-sh#3543 取り込み時に git.ts が上書きされて fork 実装が失われていた。API は現在の resolveBaseComparison / buildBranch に合わせて調整。
2 participants
Summary
Fixes a cluster of v2-workspace git correctness bugs uncovered while chasing a report that "new workspaces start multiple commits behind origin/main":
refs/heads/main(local), which is rarely fast-forwarded. Now branches from the default branch's real configured upstream — fork-workflow safe (honorsupstream/mainwhen that's the canonical remote).getStatusused a 2-dot diff, so every upstream merge inflated the sidebar count on untouched workspaces. Now 3-dot (merge-base), stable as main moves — matches GitHub's Files-changed semantics.getStatus,listBranches,listCommits,getDiffall literally computedorigin/<default>regardless of the local default branch's actual upstream. Centralized into oneresolveBaseComparisonhelper that honors the real upstream first, falls back toorigin/<default>.parseNumstatindexed by the raw numstat path string which for renames is<old> => <new>. Callers looked up by destination path from--name-status→ never matched. Switched to--numstat -z, rewrote parser for the three-cell rename record, indexed both source and destination paths.git merge-base <base> HEADto get the fork-point commit and compares content there to HEAD.Documentation
apps/desktop/docs/V2_WORKSPACE_DIFF_VIEWS.md— plain-English explainer of every UI surface that shows "what changed," which comparison runs under the hood, and why. Written for people who don't know git; also useful for anyone who does and wants the mapping between UI and code paths.Scope notes
apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts— stays v1's problem.resolveRef's"origin"default atruntime/git/refs.ts:87left as-is (low severity, not tied to any user-visible count).Test plan
main5 commits behind origin. Create new v2 workspace.git -C <new-worktree> rev-parse HEADmust equalorigin/main.upstreamremote;git branch --set-upstream-to=upstream/main main. Create a new v2 workspace. New worktree must branch offupstream/main, notorigin/main. Sidebar counts must reflectupstream/main...HEAD.main. Sidebar count = 0. Merge a PR to upstream, rungit fetchin the repo, reload the app. Count stays 0.git mv src/foo.ts src/bar.ts+ change ~5 lines in the rename. Sidebar shows non-zero+N -N(not+0 -0).+0 -0correctly.bun testinpackages/host-service— 120/120 pass (112 existing + 8 new numstat cases).Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests