feat(handoff): reduce size of packs, update stale remoteURLs#1972
feat(handoff): reduce size of packs, update stale remoteURLs#1972jonathanlab wants to merge 6 commits intomainfrom
Conversation
…eckpoint Generated-By: PostHog Code Task-Id: da45c20c-df1d-4761-a47b-e0c658828493
Generated-By: PostHog Code Task-Id: da45c20c-df1d-4761-a47b-e0c658828493
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
packages/git/src/sagas/checkpoint.ts:489-502
**Large modified tracked file left in inconsistent worktree/index state**
When a tracked file larger than 1 MiB has been locally modified, `reconcileLargeBlobs` reverts the temp-index entry to the HEAD blob (lines 492–496), so `worktreeTree` contains the HEAD version. After handoff apply, `git read-tree --reset -u worktreeTree` puts the HEAD content on disk. However, the restored `.git/index` (copied from the real index at checkpoint time) still records the modified blob as staged. The result is that `git status` shows the large file as both "to be committed" (staged modification) and "not staged for commit" (disk = HEAD), which is a confusing state for the user.
If losing the in-progress modification to large files is an accepted trade-off, a comment or a `git restore --staged <large-file>` step during apply would help. If the modification should be preserved, `reconcileLargeBlobs` should also be applied to the main index before `copyIndexFile`.
### Issue 2 of 2
packages/git/src/handoff.ts:293-310
**Silent fetch suppression can produce cryptic object-missing errors downstream**
`ensureBaselineForApply` swallows the remote fetch error with only a `warn` log, then returns normally. If the pack was built with `^upstreamHead` exclusions and the receiver's local store doesn't already contain those baseline objects, the immediately subsequent `git read-tree --reset -u worktreeTree` (in `applyFromHandoff`) will fail with a low-level "unable to read sha1" or "object not found" error that gives no indication that the baseline fetch was the root cause.
Consider re-throwing (or wrapping) the fetch error with a clear message, or promoting it to `error`-level so users and operators can correlate the cause without digging through logs.
Reviews (1): Last reviewed commit: "feat(folders): add remoteUrl override wh..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| for (const filePath of largePaths) { | ||
| const headEntry = headEntries.get(filePath); | ||
| if (headEntry) { | ||
| await tempGit.raw([ | ||
| "update-index", | ||
| "--cacheinfo", | ||
| `${headEntry.mode},${headEntry.hash},${filePath}`, | ||
| ]); | ||
| } else { | ||
| await tempGit | ||
| .raw(["update-index", "--force-remove", filePath]) | ||
| .catch(() => {}); | ||
| } | ||
| } |
There was a problem hiding this comment.
When a tracked file larger than 1 MiB has been locally modified, reconcileLargeBlobs reverts the temp-index entry to the HEAD blob (lines 492–496), so worktreeTree contains the HEAD version. After handoff apply, git read-tree --reset -u worktreeTree puts the HEAD content on disk. However, the restored .git/index (copied from the real index at checkpoint time) still records the modified blob as staged. The result is that git status shows the large file as both "to be committed" (staged modification) and "not staged for commit" (disk = HEAD), which is a confusing state for the user.
If losing the in-progress modification to large files is an accepted trade-off, a comment or a git restore --staged <large-file> step during apply would help. If the modification should be preserved, reconcileLargeBlobs should also be applied to the main index before copyIndexFile.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/git/src/sagas/checkpoint.ts
Line: 489-502
Comment:
**Large modified tracked file left in inconsistent worktree/index state**
When a tracked file larger than 1 MiB has been locally modified, `reconcileLargeBlobs` reverts the temp-index entry to the HEAD blob (lines 492–496), so `worktreeTree` contains the HEAD version. After handoff apply, `git read-tree --reset -u worktreeTree` puts the HEAD content on disk. However, the restored `.git/index` (copied from the real index at checkpoint time) still records the modified blob as staged. The result is that `git status` shows the large file as both "to be committed" (staged modification) and "not staged for commit" (disk = HEAD), which is a confusing state for the user.
If losing the in-progress modification to large files is an accepted trade-off, a comment or a `git restore --staged <large-file>` step during apply would help. If the modification should be preserved, `reconcileLargeBlobs` should also be applied to the main index before `copyIndexFile`.
How can I resolve this? If you propose a fix, please make it concise.| private async ensureBaselineForApply( | ||
| git: GitClient, | ||
| checkpoint: GitHandoffCheckpoint, | ||
| localGitState: HandoffLocalGitState | undefined, | ||
| ): Promise<void> { | ||
| const tracking = this.getPreferredTracking(localGitState, checkpoint); | ||
| if (!tracking.upstreamRemote || !tracking.upstreamMergeRef) return; | ||
|
|
||
| await this.ensureRemoteForTracking(git, tracking).catch(() => {}); | ||
| await git | ||
| .raw(["fetch", tracking.upstreamRemote, tracking.upstreamMergeRef]) | ||
| .catch((err) => { | ||
| this.logger?.warn( | ||
| "Handoff baseline fetch failed; continuing with locally available history", | ||
| { err: String(err) }, | ||
| ); | ||
| }); | ||
| } |
There was a problem hiding this comment.
ensureBaselineForApply swallows the remote fetch error with only a warn log, then returns normally. If the pack was built with ^upstreamHead exclusions and the receiver's local store doesn't already contain those baseline objects, the immediately subsequent git read-tree --reset -u worktreeTree (in applyFromHandoff) will fail with a low-level "unable to read sha1" or "object not found" error that gives no indication that the baseline fetch was the root cause.
Consider re-throwing (or wrapping) the fetch error with a clear message, or promoting it to error-level so users and operators can correlate the cause without digging through logs.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/git/src/handoff.ts
Line: 293-310
Comment:
**Silent fetch suppression can produce cryptic object-missing errors downstream**
`ensureBaselineForApply` swallows the remote fetch error with only a `warn` log, then returns normally. If the pack was built with `^upstreamHead` exclusions and the receiver's local store doesn't already contain those baseline objects, the immediately subsequent `git read-tree --reset -u worktreeTree` (in `applyFromHandoff`) will fail with a low-level "unable to read sha1" or "object not found" error that gives no indication that the baseline fetch was the root cause.
Consider re-throwing (or wrapping) the fetch error with a clear message, or promoting it to `error`-level so users and operators can correlate the cause without digging through logs.
How can I resolve this? If you propose a fix, please make it concise.
k11kirky
left a comment
k11kirky
left a comment
There was a problem hiding this comment.
greptile comments seem valid
Generated-By: PostHog Code Task-Id: da45c20c-df1d-4761-a47b-e0c658828493
|
addressed both |
2 participants
Created with PostHog Code