Migrate Tasks.Git.GetUntrackedFiles to multithreaded task model#1735
Migrate Tasks.Git.GetUntrackedFiles to multithreaded task model#1735jankratochvilcz wants to merge 1 commit into
Conversation
Review feedback addressedThanks for the MT + expert reviews. Pushed MINOR — empty/null I empirically verified NIT — Regression test. Added
All 437 |
| // process CWD before it is used as the base for resolving the (relative) file ItemSpecs inside | ||
| // GitOperations.GetUntrackedFiles. The absolutized path is only used internally for the ignore | ||
| // check; the returned [Output] items keep their original ItemSpecs (Sin 1). | ||
| AbsolutePath absProjectDir = TaskEnvironment.GetAbsolutePath(ProjectDirectory); |
There was a problem hiding this comment.
The ProjectDirectory is already expected to be an absolute path. An error can be reported if it is not.
There was a problem hiding this comment.
Done. Rebased onto main (now includes the merged #1734), so this PR is down to just the GetUntrackedFiles-specific changes; the shared RepositoryTask/PathUtilities/MockEngine/test-AssemblyInfo all come from #1734 now.
The task-level guard (only reachable on the cached-repository path, where RepositoryId is set and the base performs no path validation) now reports an error (Resources.PathMustBeAbsolute) instead of the "missing repository" warning, since ProjectDirectory is expected to be absolute. Updated RelativeProjectDirectory_OnCachedRepository_IsRejected to assert Execute() == false + logged error. Also dropped the separate MockEngine4 in favor of the extended MockEngine from #1734.
72f4f90 to
8f3abff
Compare
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
8f3abff to
05fa51f
Compare
| /// cached path). It documents the observable end-to-end behavior. | ||
| /// </summary> | ||
| [Fact] | ||
| public void RelativeProjectDirectory_IsRejected_IndependentOfProcessCwd() |
Summary
Migrates
Microsoft.Build.Tasks.Git.GetUntrackedFilesto the MSBuild multithreaded (MT) task model so it no longer relies on the process current working directory (CWD) for path resolution. See dotnet/msbuild#14093 for the MT task model.Approach: reject non-fully-qualified paths (attribute-only)
Uses the same design as the sibling
LocateRepositoryPR (#1734): instead of absolutizing paths viaTaskEnvironment.GetAbsolutePath, non-fully-qualified paths are rejected. This keeps the migration attribute-only (noIMultiThreadableTask/TaskEnvironment).Base
RepositoryTask(byte-identical to the sibling PR)GetOrCreateRepositoryInstance()rejects a non-fully-qualified initial path (viaPathUtilities.IsPathFullyQualified, an#if NETFRAMEWORKpolyfill of the BCL API thatnet472lacks) and reports the "missing repository" warning.GetUntrackedFiles— twoProjectDirectoryconsumption sites, both guardedProjectDirectoryis used in two places, and both must be MT-safe:RepositoryIdisnull): the initial path isProjectDirectory; the base guard rejects it if not fully qualified.Execute, on both paths):ProjectDirectoryis the base for resolving the relativeFilesItemSpecs inGitOperations.GetUntrackedFiles. WhenRepositoryIdis set the base serves the repository from theBuildEngine4cache and performs no initial-path validation, soGetUntrackedFiles.Executeadds its own guard rejecting a non-fully-qualifiedProjectDirectory(with the same warning, honoringNoWarnOnMissingInfo). In the shipped targetsRepositoryIdis always set (Condition="'$(_GitRepositoryId)' != ''"), so this task-level guard is the load-bearing one.[Output] UntrackedFilesare the original filteredFilesitems and keep their original ItemSpecs.Behavior change (intentional)
A relative
ProjectDirectorythat previously resolved against the process CWD is now rejected → "missing repository" warning +UntrackedFilesleft null (matching the established null-on-failure contract). In the shipped targetsProjectDirectoryis$(MSBuildProjectDirectory)(always fully qualified), so no production scenario is affected.Comment scope note (from review)
GitOperations.GetUntrackedFilescomputesPath.GetFullPath(Path.Combine(projectDirectory, file.ItemSpec)). WithprojectDirectoryguaranteed fully qualified this is MT-safe for relative or absolute item specs. A drive-/root-relativefile.ItemSpecwould causePath.Combineto discard the base and remain CWD/drive-dependent — but@(Compile)never produces such specs, so it is out of contract (documented in the code comment).Tests
AbsoluteProjectDirectory_ReportsIgnoredFileAsUntracked— historical behavior: an absoluteProjectDirectorylocates the repo and reports exactly the ignored file.RelativeProjectDirectory_IsRejected_IndependentOfProcessCwd— CWD pointed at a real repo, relativeProjectDirectory(noRepositoryId) → rejected end-to-end (no CWD resolution), warning, output null.RelativeProjectDirectory_OnCachedRepository_IsRejected— primes the shared repository cache via a realLocateRepositoryrun, then reuses it withRepositoryIdset + a relativeProjectDirectory, exercising the task-level guard on the cached path (fails if that guard is reverted).IBuildEngine4mock (the sharedMockEngineonly implementsIBuildEngine); assembly test parallelization disabled for the CWD-mutating test.Merge coordination
PathUtilities.cs,PathUtilitiesTests.cs,RepositoryTask.cs, and the test-assemblyAssemblyInfo.csare byte-identical to #1734, so git auto-resolves "both branches added the same content" when the second PR merges — no conflict.Validation
./build.sh --build→ 0 warnings, 0 errors (bothnet472and modern TFM compile).dotnet test src/Microsoft.Build.Tasks.Git.UnitTests -f net11.0→ 449 passed, 4 skipped (Windows-only), 0 failed.Related: dotnet/msbuild#14093.