Repository navigation
Batch Git config reads for branch operations - #8978
Conversation
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
No unresolved review issues were identified.
Review effort: Lite
Findings: None
What changed in this PR
Optimizes local pull request refreshes by reading repository configuration once per refresh.
Changes:
- Adds batch metadata lookup for multiple branches.
- Reuses one configuration snapshot across branch chunks.
- Expands tests for batching, refresh behavior, and failures.
| File | Description |
|---|---|
src/test/github/pullRequestGitHelper.test.ts |
Expands duplicate metadata coverage. |
src/test/github/folderRepositoryManager.test.ts |
Tests configuration snapshot behavior. |
src/github/pullRequestGitHelper.ts |
Implements batch metadata lookup. |
src/github/folderRepositoryManager.ts |
Reuses metadata during local PR discovery. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Alex Ross (alexr00)
left a comment
There was a problem hiding this comment.
Looks reasonable, thank you! One question, and there are conflicts.
034c570 to
eae1d7b
Compare
Alex Ross (alexr00)
left a comment
There was a problem hiding this comment.
Tamir Duberstein (@tamird), just for the future: it's hard to re-review PRs that are force pushed. Instead of just reviewing your new changes, I have to re-review everything.
|
Alex Ross (@alexr00) ack, will try to avoid that in the future. FWIW this is why I use reviewable.io for OSS projects I maintain. 20 years in GH code review is still extremely basic. |
Local PR enumeration queried all Git config for every branch. Match all requested branches in one helper-owned config read and one pass, keeping the highest finite PR number and the first match on ties. Single-branch lookups use the same helper; each enumeration reads fresh config.
First-activation branch association and branch deletion still reread repository config for each branch. Read metadata once for the selected branches, and inspect deletion leftovers after each mutation phase. Only refresh the cleanup snapshot when fallback deletions are needed. Keep association cleanup before that refresh so known metadata is removed even if reading the remaining config fails. Remove the single-branch metadata API and its manager wrapper. Callers use the batch API even when they need only one branch, keeping each operation responsible for the set of branches it requests.
The config key branch.feature.preview.remote belongs to feature.preview. Matching the prefix branch.feature. when deleting feature can recreate feature unnecessarily and remove the surviving branch's remote. Use the last dot, before the variable name, to identify the complete branch section in both cleanup passes. Scan each config snapshot once for the whole batch.
eae1d7b to
1a09429
Compare
|
Alex Ross (@alexr00), this compares the two commits from your review about the cleanup calls with the current three. Run it from a clone of this repository: git range-diff --ignore-cr-at-eol \
034c5709~2..034c5709 1a094297~3..1a094297 \
--end-of-options --ignore-cr-at-eolThe two flags ignore the old CRLF conversion both inside each patch and when comparing the patches. Commit 1 is unchanged. Commit 2 shows the two cleanup calls restored and the test for that ordering. If you already reviewed the September 30 update, use Fetch the revisions if your clone doesn’t have themgit fetch --no-tags https://github.com/microsoft/vscode-pull-request-github.git \
034c5709092b2d225e43399f1bbece203c7664f2 \
eae1d7ba18b8c48fc233f91eeeceebc5fe1279fd \
1a0942973c10afca34af3e3512e9bda250834581— Codex, on Tamir’s behalf |
Alex Ross (alexr00)
left a comment
There was a problem hiding this comment.
Looks good, thank you!
|
hey folks, is it expected that |
|
No, PRs from forks need to be kicked, and sometimes I forget to do so. Thanks for the reminder! |
|
/AzurePipelines run |
|
Azure Pipelines: 2 pipeline(s) were filtered out due to trigger conditions. |
Local pull request discovery reads the complete Git configuration separately for every local branch. Reading one snapshot and indexing the requested branches reduces this from N config reads to one: for 102 branches, 102 reads become 1, eliminating 101 reads (about 99%). The config entries are scanned once instead of once per branch.
Use the same batch lookup for first-activation association, reducing metadata reads from up to 10 to 1. Branch deletion reads config once after the deletion batch and refreshes it only when fallback deletions were needed, replacing the per-branch reads while preserving cleanup of leftover settings. Match the full branch section so deleting
featurepreserves settings forfeature.preview.Remove the single-branch metadata API and migrate its callers to the batch API. Keep the Map for branch lookup and presence checks, preserve the highest PR number and the first entry on ties, and retain the regression covering reuse across branch chunks and fresh data on the next refresh.