GitBlobstore: remote-managed fetch/merge/push sync#10458
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a “remote-managed” synchronization mode to GitBlobstore so local reads/writes automatically fetch from a configured remote, merge remote-tracking state into the local ref, and push updates back using --force-with-lease. This is supported by new git plumbing APIs and a new gitrebase helper that performs key/path-granularity merges with atomic handling for chunked trees.
Changes:
- Introduce remote-managed fetch/merge/push workflows in
GitBlobstore(including lease-based push retries). - Add new git plumbing methods (
FetchRef,PushRefWithLease,MergeBase,ListTreeRecursive,CommitTreeWithParents) and associated tests. - Add
gitrebasemerge helper + tests, and switch chunked-part naming to 4 digits (0001,0002, …).
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| go/store/blobstore/internal/gitrebase/temp_index.go | Adds temp index helper for merge operations. |
| go/store/blobstore/internal/gitrebase/merge.go | Implements merge logic for remote-tracking -> local ref with conflict hooks and chunked-tree atomicity. |
| go/store/blobstore/internal/gitrebase/merge_test.go | Adds test coverage for fast-forward, merge commits, and conflict policies. |
| go/store/blobstore/internal/git/api.go | Extends GitAPI with fetch/push-with-lease and merge/list/commit helpers. |
| go/store/blobstore/internal/git/impl.go | Implements new GitAPI methods via git CLI plumbing commands. |
| go/store/blobstore/internal/git/errors.go | Adds MergeConflictError type and helpers. |
| go/store/blobstore/internal/git/impl_test.go | Refactors test setup and adds tests for new git API methods. |
| go/store/blobstore/git_blobstore.go | Adds remote-managed mode + integrates fetch/merge/push sync; switches chunk part width to 4 digits. |
| go/store/blobstore/git_blobstore_test.go | Adds integration tests for remote-managed behavior and retries. |
| go/store/blobstore/git_blobstore_helpers_test.go | Updates test fake to satisfy extended GitAPI interface. |
| go/store/blobstore/git_blobstore_chunked_put_test.go | Updates expectations to 4-digit chunk part names. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@coffeegoddd DOLT
|
|
@coffeegoddd DOLT
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if err := gbs.syncForRead(ctx); err != nil { | ||
| return false, err | ||
| } |
There was a problem hiding this comment.
Exists now always calls syncForRead(), which will run a git fetch and mutate local refs even for callers that only expect local reads. Consider gating syncForRead() behind an explicit remote-managed option (or a no-op when no remote is configured) to avoid unexpected network/IO and side effects on read-only use cases.
go/store/blobstore/git_blobstore.go
Outdated
| remoteName := opts.RemoteName | ||
| if remoteName == "" { | ||
| remoteName = "origin" | ||
| } | ||
| remoteTrackingRef := DoltRemoteTrackingDataRef(remoteName) |
There was a problem hiding this comment.
RemoteName defaults to "origin" here, which effectively makes remote-managed behavior the default for all GitBlobstores. If local-only operation should still be supported, consider leaving remoteName empty by default and only enabling remote fetch/push when the caller explicitly opts in (or provide a separate flag/enum to control sync behavior).
| return err | ||
| } | ||
| if !okRemote { | ||
| return &git.RefNotFoundError{Ref: gbs.remoteTrackingRef} |
There was a problem hiding this comment.
Returning RefNotFoundError when the remote-tracking ref doesn't exist makes an uninitialized remote (no refs/dolt/data yet) fail hard, instead of behaving like an empty store (e.g., manifest missing). If empty remotes are valid, consider treating !okRemote as an empty state (no-op sync) and letting the existing NotFound / missing-ref logic handle it.
| return &git.RefNotFoundError{Ref: gbs.remoteTrackingRef} | |
| // Treat an uninitialized remote (no remote-tracking ref yet) as an empty store. | |
| // This makes syncForRead a no-op in that case, allowing existing NotFound logic | |
| // elsewhere to handle the absence of data. | |
| return nil |
|
@coffeegoddd DOLT
|
reltuk
left a comment
There was a problem hiding this comment.
LGTM, one comment about separating local vs. remote ref name and making a unique local ref for this blobstore instance.
|
@coffeegoddd DOLT
|
No description provided.