Migrate Tasks.Git.LocateRepository to multithreaded task model#1734
Migrate Tasks.Git.LocateRepository to multithreaded task model#1734jankratochvilcz wants to merge 8 commits into
Conversation
Migrates the shared abstract base RepositoryTask and the concrete
LocateRepository task to the MSBuild multithreaded (MT) task model.
Base RepositoryTask:
- Implements IMultiThreadableTask with a TaskEnvironment property that
defaults to TaskEnvironment.Fallback so existing call paths and unit
tests keep single-process (CWD) semantics with no setup.
- Absolutizes the initial path at the GetInitialPath() boundary:
GetOrCreateRepositoryInstance now computes
TaskEnvironment.GetAbsolutePath(initialPath) and passes the resolved
AbsolutePath.Value to GitRepository.TryFindRepository. This removes the
process-CWD dependency (TryFindRepository internally calls
Path.GetFullPath, which only consults the CWD for relative inputs;
feeding it an absolute path is MT-safe).
Note: the implicit AbsolutePath->string conversion returns the original
(possibly relative) input, so .Value is used explicitly.
- Sin 2: both ReportMissingRepositoryWarning call sites keep the ORIGINAL
initialPath so warnings show the user's input, not the absolutized path.
- Edge case: GetAbsolutePath throws ArgumentException on null/empty,
matching the previous Path.GetFullPath("") behavior.
Concrete LocateRepository:
- Annotated with [MSBuildMultiThreadableTask].
Call-chain audit: GitOperations.GetRepositoryUrl / GetSourceRoots and the
downstream GitRepository path operations combine paths with the absolute
git/working directories of the located repository, so they become MT-safe
once the initial path is absolute. No remaining
Directory.GetCurrentDirectory / Environment.CurrentDirectory /
Environment.Get/SetEnvironmentVariable / CWD-relative Path.GetFullPath in
the LocateRepository chain.
Test:
- Adds LocateRepositoryTests.RelativePath_ResolvesAgainstTaskEnvironment-
ProjectDirectory_NotProcessCwd (Pattern A decoy CWD): creates a real
minimal git repo, sets the process CWD to a repo-less decoy, sets the
task's TaskEnvironment project directory to the repo, passes a relative
Path, and asserts WorkingDirectory/RepositoryId resolve to the project
repo. Fails if the migration is reverted.
- Extends MockEngine to IBuildEngine4 (in-memory registered task object
store) so the cached success path can run.
- Disables assembly test parallelization to keep the CWD mutation safe.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The implicit string conversion returns the absolute Value, not the original input; correct the comment while keeping the explicit .Value. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Wrap GetAbsolutePath + TryFindRepository so an empty/null initial path reports a missing-repository warning and returns gracefully instead of throwing an unhandled ArgumentException (preserves pre-migration behavior; ExecuteImpl's catch does not handle ArgumentException). - Rewrite the parity comment to describe the control-flow (not just exception type) equivalence with the old TryFindRepository path. - Add EmptyPath_DegradesGracefully regression test (dual-fault verified: fails when the catch is reverted). - Make AssemblyInfo.cs parallelization comment test-agnostic. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Wrap GetAbsolutePath + TryFindRepository so an empty/null initial path reports a missing-repository warning and returns gracefully instead of throwing an unhandled ArgumentException (preserves pre-migration behavior; ExecuteImpl's catch does not handle ArgumentException). Shared base kept byte-identical with mt/locate-repository (dotnet#1734). - Rewrite the parity comment to describe the control-flow equivalence. - Add EmptyProjectDirectory_DegradesGracefully regression test. - Make AssemblyInfo.cs parallelization comment test-agnostic (was wrongly naming LocateRepositoryTests). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Review feedback addressedThanks for the thorough MT + expert reviews. Pushed MAJOR — empty/null Empirical note on "whitespace": I verified against Microsoft.Build 18.6.3 that Inaccurate comment. Rewritten to describe the control-flow equivalence (swallowed → warning → success), not just the matching exception type. Regression test. Added MINOR — assembly-wide parallelization disable. Kept (acknowledged acceptable); the CWD-mutating test makes process-global All 437 |
| var initialPath = GetInitialPath(); | ||
|
|
||
| if (!GitRepository.TryFindRepository(initialPath, out var location)) | ||
| // Resolve the initial path against the task's project directory rather than the process |
There was a problem hiding this comment.
This is not necessary. The initial path is already an absolute path.
https://github.com/dotnet/sourcelink/blob/main/src/Microsoft.Build.Tasks.Git/build/Microsoft.Build.Tasks.Git.targets#L26
You can add a check that reports an error if it's not an absolute path.
There was a problem hiding this comment.
Thanks, I changed it as asked
…itory) Make the MT migration attribute-only: rather than resolving the initial path via TaskEnvironment.GetAbsolutePath, RepositoryTask now rejects any path that is not fully qualified. A relative/drive-relative/root-relative path depends on the shared process CWD/drive, which is unsafe under the multithreaded task model, so it degrades to the standard "missing repository" warning. - Add IsPathFullyQualified polyfill to PathUtilities (net472 lacks the BCL API) with a dedicated test suite (Windows/Unix/relative cases). - Drop IMultiThreadableTask/TaskEnvironment from RepositoryTask; keep [MSBuildMultiThreadableTask] on the concrete LocateRepository. - Simplify tests: revert shared MockEngine to IBuildEngine; localize a minimal IBuildEngine4 to LocateRepositoryTests. Cover historical behavior (absolute path locates repo) and CWD-independence (relative path rejected even when CWD is a repo). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Move the inline IBuildEngine4 test engine into a shared TestUtilities.MockEngine4 next to the existing MockEngine so other tests can reuse it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
2 participants
Summary
Migrates
Microsoft.Build.Tasks.Git.LocateRepository(and its shared abstract baseRepositoryTask) to the MSBuild multithreaded (MT) task model. The MT hazard: repository discovery resolved the initial path throughGitRepository.TryFindRepository, which callsPath.GetFullPathinternally — making it dependent on the process current working directory (CWD), which is shared across projects in the MT model.Approach: reject non-fully-qualified paths (attribute-only)
Rather than absolutizing the initial path via
TaskEnvironment.GetAbsolutePath, this migration rejects any initial path that is not fully qualified. Repository discovery must not depend on the shared process CWD/drive; a relative, drive-relative (C:foo), or root-relative (\foo) path is rejected with the standard "missing repository" warning instead of being silently resolved against process state.This keeps the migration attribute-only: the concrete
LocateRepositorycarries[MSBuildMultiThreadableTask], but the baseRepositoryTaskneeds noIMultiThreadableTask/TaskEnvironmentbecause it no longer performs any path absolutization — it only validates.IsPathFullyQualifiedpolyfillPath.IsPathFullyQualifieddoes not exist onnet472, soPathUtilities.IsPathFullyQualifiedprovides a faithful reimplementation of the runtime'sPathInternal.IsPartiallyQualified(Windows) under#if NETFRAMEWORK, delegating to the BCL API otherwise. It correctly rejects the CWD/drive-dependent forms thatPath.IsPathRootedwould accept (Sin 7). Covered byPathUtilitiesTestswithWindowsOnly/UnixOnly-gated cases (drive-qualified, UNC, device\\?\, drive-relative, root-relative, invalid drive) plus OS-agnostic relative cases.Base
RepositoryTaskGetOrCreateRepositoryInstance()guards the initial path:if (string.IsNullOrEmpty(initialPath) || !PathUtilities.IsPathFullyQualified(initialPath)) { ReportMissingRepositoryWarning(initialPath); return null; }. TheIsNullOrEmptyshort-circuit preserves the pre-migration graceful behavior for empty input and avoids the polyfill'sArgumentNullExceptionon null. Warnings keep the original input for diagnostics (Sin 2).Concrete
LocateRepositoryAnnotated with
[MSBuildMultiThreadableTask](Inherited=false, so each concrete class needs it). NoTaskEnvironmentAPI is required.Behavior change (intentional)
A relative initial path that previously resolved against the process CWD is now rejected → "missing repository" warning. In the shipped targets
LocateRepositoryreceives$(MSBuildProjectDirectory)(always fully qualified), so no production scenario is affected; the change only hardens against CWD-dependent inputs.Scope notes (from review)
GitEnvironment.CreateFromProcessEnvironmentstill reads process env vars (HOME/XDG_CONFIG_HOME/PROGRAMDATA/PATH) — this is unchanged, read-only, and identical across all projects in a build, so it does not carry the per-project shared-state hazard the migration targets.RepositoryTask, so it also affects the siblingGetUntrackedFiles— which is migrated with the same approach in Migrate Tasks.Git.GetUntrackedFiles to multithreaded task model #1735 (and adds its own guard for its secondProjectDirectoryconsumption site).Merge coordination
PathUtilities.cs,PathUtilitiesTests.cs,RepositoryTask.cs, and the test-assemblyAssemblyInfo.csare byte-identical to the sibling PR #1735, so git auto-resolves "both branches added the same content" when the second PR merges — no conflict.Build & test
./build.sh --build→ 0 warnings, 0 errors (bothnet472and modern TFM compile).dotnet test Microsoft.Build.Tasks.Git.UnitTests -f net11.0→ 448 passed, 4 skipped (Windows-only).RelativePath_IsRejected_IndependentOfProcessCwdfails if the migration is reverted (verified);AbsolutePath_LocatesRepositoryconfirms historical behavior.Related: dotnet/msbuild#14093.