Skip to content

Share transient libgit2 config lookup helper - #2066

Open
tyrielv wants to merge 1 commit into
microsoft:masterfrom
tyrielv:tyrielv/shared-libgit2-config-lookup
Open

Share transient libgit2 config lookup helper#2066
tyrielv wants to merge 1 commit into
microsoft:masterfrom
tyrielv:tyrielv/shared-libgit2-config-lookup

Conversation

@tyrielv

@tyrielv tyrielv commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Note

This PR is now a single commit (5471e46), squashed for a clean review/merge history.

Summary

  • add LibGit2Repo.GetConfigBoolOrDefault(...) (instance + static overloads) for one-off boolean config reads, replacing scattered short-lived LibGit2Repo/LibGit2RepoInvoker usage at 4 call sites
  • avoid LibGit2RepoInvoker's object-store preload for reads that only need a single config value and immediately dispose the repo
  • cover happy-path, unset-key, and exception-fallback behavior with unit tests

Motivation

LibGit2RepoInvoker.InitializeSharedRepo() intentionally forces an object-store probe so long-lived/shared callers can amortize object-store load costs. That is wasted work for one-off config reads that immediately dispose the repo. This change centralizes the short-lived config-read pattern behind two methods.

Design

The helper methods live directly on LibGit2Repo rather than a separate extension class — this matches the repo.GetConfigBoolOrDefault(name, default) convention already documented in AGENTS.md, and avoids unnecessary indirection for a class the team owns in the same assembly:

  • public bool GetConfigBoolOrDefault(string key, bool defaultValue) — instance method, reads from an already-open repo using this.Tracer.
  • public static bool GetConfigBoolOrDefault(ITracer tracer, string repoPath, string key, bool defaultValue) — opens a transient LibGit2Repo, delegates to the instance method, and disposes it.

Both fall back to defaultValue and log a RelatedWarning on any failure (matching the "default on any failure" contract each call site previously implemented independently).

Also added a protected LibGit2Repo(ITracer tracer) constructor purely to support test doubles that need to inject a mock tracer without opening a real repo.

Refactored call sites

  • GVFS/GVFS/CommandLine/CloneVerb.csgvfs.trust-pack-indexes (only read when clone succeeded)
  • GVFS/GVFS.Hooks/Program.csgvfs.show-hydration-status (runs on every git hook invocation)
  • GVFS/GVFS.Mount/InProcessMount.csgvfs.background-cache-auth
  • GVFS/GVFS/CommandLine/PrefetchVerb.csgvfs.prefetch-offload
  • GVFS/GVFS.Hooks/GVFS.Hooks.csproj — updated to drop the now-removed extension-class source file

Survey notes / intentionally left alone

Surveyed master for short-lived config-only LibGit2Repo/LibGit2RepoInvoker usage. These were left alone because they are shared/long-lived repo access, not the transient anti-pattern this PR addresses:

  • GVFS/GVFS.Common/Maintenance/PrefetchStep.cs
  • GVFS/GVFS.Common/GitStatusCache.cs
  • GVFS/GVFS.Common/Git/GitRepo.cs

Stacked PR

#2065 ("Fix NullReferenceException when cloning into a non-empty directory") is rebased on top of this branch and depends on the LibGit2Repo.GetConfigBoolOrDefault helper added here. This PR should merge first.

Review

Reviewed with an internal 6-lens review-swarm pass (correctness, security, design, tests, async-parallelism, risk-rollout). All actionable findings were addressed:

  • Widened the shared helper's exception handling back to a plain catch (Exception), restoring the "default on any failure" guarantee InProcessMount/PrefetchVerb relied on before this refactor.
  • Fixed a double-RelatedWarning log on the repo-open-failure path.
  • Replaced a hardcoded, non-portable Z:\... path in a unit test with a GUID-suffixed temp path.
  • Added missing test coverage for the unset-key (null-coalescing) branch and the InvalidDataException catch arm.
  • Simplified the parameterless constructor to delegate to the tracer-accepting one.

Validation

  • dotnet build GVFS\GVFS.UnitTests\GVFS.UnitTests.csproj -c Debug
  • out\GVFS.UnitTests\bin\Debug\net10.0-windows10.0.17763.0\win-x64\GVFS.UnitTests.exe
    • Result: 891 passed, 0 failed, 11 skipped (pre-existing, unrelated)

Add LibGit2Repo.GetConfigBoolOrDefault(...) (instance + static overloads) for
one-off boolean config reads, replacing scattered short-lived
LibGit2Repo/LibGit2RepoInvoker usage at 4 call sites:

- GVFS/CommandLine/CloneVerb.cs (gvfs.trust-pack-indexes)
- GVFS.Hooks/Program.cs (gvfs.show-hydration-status)
- GVFS.Mount/InProcessMount.cs (gvfs.background-cache-auth)
- GVFS/CommandLine/PrefetchVerb.cs (gvfs.prefetch-offload)

LibGit2RepoInvoker.InitializeSharedRepo() intentionally forces an
object-store probe so long-lived/shared callers can amortize object-store
load costs. That is wasted work for one-off config reads that immediately
dispose the repo.

The helper methods live directly on LibGit2Repo rather than a separate
extension class, matching the repo.GetConfigBoolOrDefault(name, default)
convention already documented in AGENTS.md, and avoiding unnecessary
indirection for a class the team owns in the same assembly. Both methods
fall back to defaultValue and log a RelatedWarning on any failure, matching
the "default on any failure" contract each call site previously implemented
independently.

Added a protected LibGit2Repo(ITracer tracer) constructor to support test
doubles that inject a mock tracer without opening a real repo.

Surveyed master for other short-lived config-only LibGit2Repo/
LibGit2RepoInvoker usage; PrefetchStep.cs, GitStatusCache.cs, and
GitRepo.cs were left alone because they use shared/long-lived repo access,
not the transient anti-pattern this change addresses.

Reviewed with an internal 6-lens review-swarm pass (correctness, security,
design, tests, async-parallelism, risk-rollout); addressed all actionable
findings:
- Widened the shared helper's exception handling to a plain catch
  (Exception), restoring the "default on any failure" guarantee
  InProcessMount/PrefetchVerb relied on before this refactor.
- Fixed a double-RelatedWarning log on the repo-open-failure path.
- Replaced a hardcoded, non-portable "Z:\..." path in a unit test with a
  GUID-suffixed temp path.
- Added test coverage for the unset-key (null-coalescing) branch and the
  InvalidDataException catch arm.
- Simplified the parameterless constructor to delegate to the
  tracer-accepting one.

Full unit test suite: 891 passed, 0 failed, 11 skipped (pre-existing,
unrelated).

Assisted-by: Claude Opus 4.8
Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
@tyrielv
tyrielv force-pushed the tyrielv/shared-libgit2-config-lookup branch from 970ee18 to 5471e46 Compare August 7, 2026 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant