Repository navigation
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
github-actions
Bot
added
vouch:unvouched
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 462f597cbf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const referenceArgs = references.length | ||
| ? [ | ||
| "--dissociate", | ||
| ...references.flatMap((ref) => ["--reference", path.resolve(input.cwd, ref)]), |
There was a problem hiding this comment.
When the source has multiple initialized submodules, this emits several --reference flags. On Git 2.43, git submodule update -h documents a single --reference <repository>, and tracing this exact invocation shows its wrapper overwrites earlier values and forwards only the final reference to submodule--helper. Consequently every destination clone except, at best, the submodule matching that last reference still transfers its full object set; the tests pass because they verify checkout and dissociation rather than object reuse. Update each submodule with its corresponding reference or use a mechanism that preserves the mappings.
AGENTS.md reference: AGENTS.md:L15-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 57a6e53. Each direct child now receives only its matching source reference, then nested children are initialized with their corresponding source paths. Custom active/update and clone concurrency settings retain the ordinary recursive Git command.
Added a cached two-sibling plus nested-submodule fixture over file:// transport that asserts no object want requests. That assertion fails against 462f597 and passes with this correction; all 63 focused driver tests pass. The PR body now includes fresh measurements of the revised implementation and explicitly separates single-submodule timing from multi-submodule reuse coverage.
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This production-path change replaces ordinary recursive submodule checkout with a new reference-reuse and dissociation algorithm for every worktree containing submodules. Although it has extensive integration coverage and preserves fallback behavior, the implementation is complex and changes existing runtime behavior beyond a small isolated tweak. Notes:
You can add or adjust custom eligibility rules. Learn more. |
|
@codex review The repeated-reference finding is addressed in 57a6e53, including a transport-level regression test that fails on the previous implementation. This addresses the same object-reuse coverage gap raised in Macroscope’s approvability comment. The PR description has updated measurements and regression coverage for the revised implementation. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 57a6e533dd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /^submodule\.(active|fetchjobs|.*\.(active|update))\n/.test(entry) && | ||
| !entry.endsWith(".active\ntrue"), |
There was a problem hiding this comment.
submodule.active=true selection
When repository config contains submodule.active=true, this suffix check treats it like the boolean submodule.<name>.active=true and enters the optimized path, which explicitly updates every path from .gitmodules. However, Git defines the global submodule.active value as a pathspec, so true selects a submodule whose path matches true; an ordinary recursive update leaves other submodules inactive. Repositories using this valid selection therefore clone submodules they intentionally excluded, potentially restoring the startup cost this change is intended to avoid.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 57a6e53. Configure here.
|
@codex review 1001b4c addresses both new findings: the global active pathspec and continuing ordinary Git initialization after a failure. Both regression cases were checked against the original baseline and the reviewed commit; all 65 focused driver tests, targeted lint/formatting, and the server typecheck pass. The full production diff remains 98 changed lines. |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Updated the edge-case measurement matrix with explanations of the slower cases. Recalculated the published numbers from the raw samples; the existing means, percentages, and absolute differences were correct. No code changed (
These explanations follow the command paths and packet traces; timings do not isolate each subprocess's contribution. The 44–164 ms overhead is likely minor during first-message setup because it occurs once per newly created worktree, not on each subsequent message. User perception was not measured, and larger module counts or slower failures can increase the cost. The separate large-repository sample saved 15.82 seconds; the PR retains both the benefits and the negative rows so reviewers can assess the tradeoff. All 120 measured calls across 12 scenarios matched the baseline destination state in the documented checks. The PR includes all five samples per implementation and identifies the synthetic local-transport measurements as Linux/btrfs results. |
juliusmarminge
added
the
macroscope-review
ChatGPT Codex Connector
|
Note Written by Hi! We are cleaning up open PRs, and this one appears to have been created with an older model ( |
fixing since i do a lot of work on a repo with a decent amount of submodule content, so everytime i start a new thread (I have it start a new worktree based on origin for new threads), and send my first message, it would take ~18 seconds to setup the worktree, this does some work to cut that down.
======= below this line is ai slop =========
What Changed
Fresh worktrees clone submodule history again even when the source checkout already has the objects. Initialize each destination submodule using its matching source checkout as a temporary Git
--reference, then--dissociateso the destination owns its objects. Repeat for nested submodules. Missing objects still come from the configured remote, and unsuitable references fall back to an ordinary clone.Git's submodule wrapper retains only the last
--reference, so each clone now receives exactly one matching reference. Custom submodule active/update policies and clone concurrency settings retain the ordinary recursive command. If preparation still fails, retry the ordinary recursive update so Git can process remaining siblings before the existing best-effort warning.The production diff is 98 changed lines in the shared server Git driver; the only other changed file is its focused test suite. No new settings, persistent cache, dependencies, or client/protocol changes.
Closes #9853.
Why
An observed first-message startup spent 0.522 s fetching origin, 0.296 s adding the worktree, and 17.417 s initializing submodules. This change targets the dominant submodule step while preserving automatic population and independent worktrees.
Worktree setup measurements
72.1% less time · 15.82 seconds saved on average
Actual
createWorktree()calls, including submodule initialization. Linux/btrfs · Git 2.55 · approximately 1 GiB of packed submodule history. Two runs per version, interleaved, using fresh destinations and the same pinned commit. Excludes origin fetch and provider startup; caches were not flushed.Measurement method and limits
These measurements were taken on
57a6e533d, replacing the earlier implementation's measurements. The subsequent correction handles globalsubmodule.active=trueselection and failure fallback; it does not change the successful path exercised by this benchmark. Baseline:8faf031c2dac4f147c43fe32e326b76678c20c94. Node 26.8.1, disk-backed destinations, the same remote,pack.threads=2, andnice 10. Execution order was before, after, after, before. The baseline driver was loaded from the original source with only benchmark import paths adjusted.The measured driver call includes worktree creation, submodule preparation, base-ref configuration and cache invalidation. Each run checked the exact submodule HEAD and absence of persistent alternates. The original ~18-second trace and these benchmarks are separate samples on a shared machine with network variability. Two observations per version establish a local improvement, not a universal speedup; network byte savings were not measured directly.
The large-repository timing above measures one large submodule. The separate edge-case matrix below measures multiple/nested submodules, custom policies, and fallback overhead on Linux/btrfs. Local-path remotes may already benefit from Git's local clone optimizations; the new fixtures use file:// transport to avoid hardlink cloning.
Edge-case measurements on the final commit
The matrix below compares the original driver (
8faf031c) with the unchanged PR head (1001b4cc0). 120 measured calls: 12 scenarios, five calls per implementation per scenario. Destination state matched the original driver in every measured call.Positive percentages mean less elapsed time; negative percentages mean more. These are synthetic local
file://transport fixtures on Linux/btrfs, separate from the approximately 1 GiB repository measurement above. Ordinary modules contain approximately 16 MiB of history each (four revisions of a 4 MiB random payload); the tiny modules contain a README only. Nested modules use spaces in their paths, and submodule names differ from their paths.The slower cases have small absolute costs in these fixtures, even where the percentage increase is large. The command paths and packet traces explain the likely sources of overhead; the timings measure the whole driver call, not the milliseconds attributable to each subprocess:
wantrequests, so reuse worked. Each module still requires a separate update invocation, reference setup, and dissociation to preserve independence. With only README-sized histories to transfer, this setup costs more than the transfer work it saves.Custom active/update/concurrency settings use Git's ordinary recursive path after the configuration checks. Their measured mean differences range from 12.2 ms less to 17.4 ms more; the fetchJobs=4 difference is 0.6 ms more, with substantial sample variation. Those small differences do not establish either an optimization or a precise overhead cost for custom policies.
For interactive first-message setup, the 44–164 ms penalties in the tiny/shallow/uninitialized fixtures are likely minor: they are paid once when creating a worktree, not on every subsequent message in that worktree. This is a practical assessment, not a measured perceptibility result. The separate large-repository sample saved 15.82 seconds; that is context for the tradeoff, not a promise that the same user receives both results. More modules or slower failures can increase the penalty, so the negative rows remain visible rather than being treated as universally negligible.
Method, behavioral checks, and every timing sample
Every sample runs the actual
createWorktree()driver call, using a fresh disk-backed destination. Source fixtures are initialized before timing, and each scenario uses the same source and selected commit for both implementations. Execution order is before, after, after, before, before, after, after, before, before, after. Caches were not flushed; runs were serial on a shared machine, atnice 10, withpack.threads=2. Fixture setup, validation, and cleanup are outside the measured interval. Origin fetch and provider startup are excluded. Packet tracing is enabled identically for both implementations. Transport is local, so these fixtures measure no Internet latency or bandwidth savings.For each destination, compare module initialization, HEAD, tracked-file lists, working-tree status, payload hash, and README presence against the baseline. Source status is checked after every call; the dirty-source scenario additionally checks the original edited file and excludes staged/untracked source files from the destination. Its selected commit is missing from the source reference, and the remote advances beyond that selected commit. Every initialized destination is checked for absence of persistent alternates. Cached single/sibling/nested/tiny cases make no object
wantrequests with the optimization. The existing source-deletion/fsck tests remain separate coverage.Active selection, update=none, and fetchJobs scenarios retain their selected/skipped modules. In the unreachable-module case, healthy siblings are cloned as with native Git, but Git may abort before checking out their files; successful worktree creation is not a claim of complete submodule population after a failure. All 120 calls matched the corresponding baseline state in these checks.
Times below are milliseconds, listed in execution order within each implementation. Means and percentages above use unrounded samples. Five observations describe these local runs; they do not establish a universal speedup or statistical significance for small differences.
Regression coverage
GitVcsDriverCore.test.tson Node 24.20.0. The cached fixture uses two siblings and a nested submodule overfile://transport; packet tracing asserts no objectwantrequests when the matching source repositories already contain all objects. This assertion fails on the original PR implementation (462f597) and passes with the correction.fsckandreset --hardafter source object stores are deleted.submodule.active=truepathspec) andupdate=nonekeep their ordinary Git behavior. An unreachable first submodule still allows Git to clone healthy siblings. Both added edge-case tests fail on57a6e533d, pass against the original baseline, and pass with the correction. The existing best-effort checkout-failure test remains passing.git diff --checkpass. Typecheck reports existing unrelated Effect suggestions, with no errors. No correctness regression was observed in this coverage. The edge-case measurements above record both speedups and timing regressions.Checklist
Model and harness: GPT-6 / Codex.
Note
Medium Risk
Changes Git submodule initialization during worktree creation with new branching and fallbacks; incorrect reference handling could yield wrong commits or slow paths, though failures still degrade to the prior recursive update behavior.
Overview
Worktree creation no longer relies on a plain recursive
git submodule updatewhen the caller’s checkout already has submodule repos locally. Aftergit worktree add, the driver resolves the source repo top-level frominput.cwdand runs newprepareSubmodules, which walks each submodule path, clones with--referenceon the matching source checkout when a.gitexists there,--dissociateso the new worktree owns its objects, then recurses for nested submodules.Repos with non-default
submodule.active/update/ parallel fetch settings, missing references, or failed reference clones still use the originalsubmodule update --init --recursivepath (with the same best-effort warning if checkout fails).Tests replace one happy-path case with a parametrized matrix (cached, populated, shallow, uninitialized, inactive, unreachable,
update=none, etc.) covering nested submodules, pinned gitlinks, independence after deleting source object stores, untouched source dirty state, and packet tracing for the fully cached clone case.Reviewed by Cursor Bugbot for commit 1001b4c. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Reuse local source submodule objects when creating worktrees in
GitVcsDriverAdds a recursive
prepareSubmoduleshelper that clones each configured direct child submodule from the matching source submodule repository with dissociated objects, then recurses into nested children the same way.createWorktreenow resolves the top-level source repository from the caller's working directory and invokes this helper before falling back to the ordinary recursivegit submodule update.git submodule updatefor the whole destination.prepareSubmodulesinspects.gitmodulesand repo config to decide whether reference-based cloning is safe; if that inspection is wrong it could skip the optimization or produce a partial clone — verify behavior in GitVcsDriverCore.ts when customsubmodule.activeorsubmodule.<name>.updatepolicies are present.Macroscope summarized 1001b4c.