Skip to content

Migrate Tasks.Git.GetUntrackedFiles to multithreaded task model#1735

Draft
jankratochvilcz wants to merge 1 commit into
dotnet:mainfrom
jankratochvilcz:mt/get-untracked-files
Draft

Migrate Tasks.Git.GetUntrackedFiles to multithreaded task model#1735
jankratochvilcz wants to merge 1 commit into
dotnet:mainfrom
jankratochvilcz:mt/get-untracked-files

Conversation

@jankratochvilcz

@jankratochvilcz jankratochvilcz commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Summary

Migrates Microsoft.Build.Tasks.Git.GetUntrackedFiles to 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 LocateRepository PR (#1734): instead of absolutizing paths via TaskEnvironment.GetAbsolutePath, non-fully-qualified paths are rejected. This keeps the migration attribute-only (no IMultiThreadableTask / TaskEnvironment).

Base RepositoryTask (byte-identical to the sibling PR)

GetOrCreateRepositoryInstance() rejects a non-fully-qualified initial path (via PathUtilities.IsPathFullyQualified, an #if NETFRAMEWORK polyfill of the BCL API that net472 lacks) and reports the "missing repository" warning.

GetUntrackedFiles — two ProjectDirectory consumption sites, both guarded

ProjectDirectory is used in two places, and both must be MT-safe:

  1. Repository discovery (base, only when RepositoryId is null): the initial path is ProjectDirectory; the base guard rejects it if not fully qualified.
  2. File ItemSpec base (Execute, on both paths): ProjectDirectory is the base for resolving the relative Files ItemSpecs in GitOperations.GetUntrackedFiles. When RepositoryId is set the base serves the repository from the BuildEngine4 cache and performs no initial-path validation, so GetUntrackedFiles.Execute adds its own guard rejecting a non-fully-qualified ProjectDirectory (with the same warning, honoring NoWarnOnMissingInfo). In the shipped targets RepositoryId is always set (Condition="'$(_GitRepositoryId)' != ''"), so this task-level guard is the load-bearing one.
  • Sin 1: [Output] UntrackedFiles are the original filtered Files items and keep their original ItemSpecs.

Behavior change (intentional)

A relative ProjectDirectory that previously resolved against the process CWD is now rejected → "missing repository" warning + UntrackedFiles left null (matching the established null-on-failure contract). In the shipped targets ProjectDirectory is $(MSBuildProjectDirectory) (always fully qualified), so no production scenario is affected.

Comment scope note (from review)

GitOperations.GetUntrackedFiles computes Path.GetFullPath(Path.Combine(projectDirectory, file.ItemSpec)). With projectDirectory guaranteed fully qualified this is MT-safe for relative or absolute item specs. A drive-/root-relative file.ItemSpec would cause Path.Combine to 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 absolute ProjectDirectory locates the repo and reports exactly the ignored file.
  • RelativeProjectDirectory_IsRejected_IndependentOfProcessCwd — CWD pointed at a real repo, relative ProjectDirectory (no RepositoryId) → rejected end-to-end (no CWD resolution), warning, output null.
  • RelativeProjectDirectory_OnCachedRepository_IsRejected — primes the shared repository cache via a real LocateRepository run, then reuses it with RepositoryId set + a relative ProjectDirectory, exercising the task-level guard on the cached path (fails if that guard is reverted).
  • A minimal inline IBuildEngine4 mock (the shared MockEngine only implements IBuildEngine); assembly test parallelization disabled for the CWD-mutating test.

Merge coordination

PathUtilities.cs, PathUtilitiesTests.cs, RepositoryTask.cs, and the test-assembly AssemblyInfo.cs are 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 (both net472 and 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.

@jankratochvilcz jankratochvilcz requested a review from tmat as a code owner June 30, 2026 13:57
@jankratochvilcz

Copy link
Copy Markdown
Contributor Author

Review feedback addressed

Thanks for the MT + expert reviews. Pushed e133a13:

MINOR — empty/null ProjectDirectory graceful → crash divergence (shared base, Sin 6). Fixed in the shared RepositoryTask base (kept byte-identical with #1734). Post-migration GetAbsolutePath("") threw ArgumentException before TryFindRepository, escaping ExecuteImpl's IOException/InvalidDataException/NotSupportedException catch (→ MSB4018). Pre-migration the empty path reached TryFindRepository's internal Path.GetFullPath(""), which threw the same exception but had it swallowed → graceful warning + Execute() == true. Now wrapped in try { … } catch (ArgumentException) { ReportMissingRepositoryWarning(initialPath); return null; }.

I empirically verified GetAbsolutePath throws only on null/"", not whitespace-only, so the reachable case is empty string via direct Execute(). The misleading parity comment was rewritten to state the control-flow (not just exception-type) equivalence.

NIT — AssemblyInfo.cs comment named LocateRepositoryTests. Reworded test-agnostically (the directive applies to whichever test mutates CWD), keeping the file byte-identical with #1734.

Regression test. Added EmptyProjectDirectory_DegradesGracefully_DoesNotThrow (asserts Execute() == true, no errors, null output). Dual-fault verified against the base revert.

GitEnvironment.Create reading process env directly (MT call-chain gap). Acknowledged — this is pre-existing foundation/shared code (reached only when ConfigurationScope is unset; these are machine-global vars, not per-project). The test pins ConfigurationScope = "local". Out of scope for this task migration; suggest a follow-up issue to route GitEnvironment discovery through TaskEnvironment.

All 437 Microsoft.Build.Tasks.Git.UnitTests (net11.0) pass; full build clean. Sin 1 (output ItemSpecs unchanged) and Sin 5 (canonicalization preserved) verified by the reviewers.

// 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);

@tmat tmat Jul 1, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ProjectDirectory is already expected to be an absolute path. An error can be reported if it is not.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@jankratochvilcz jankratochvilcz force-pushed the mt/get-untracked-files branch from 72f4f90 to 8f3abff Compare July 8, 2026 09:56
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@jankratochvilcz jankratochvilcz force-pushed the mt/get-untracked-files branch from 8f3abff to 05fa51f Compare July 8, 2026 13:54
/// cached path). It documents the observable end-to-end behavior.
/// </summary>
[Fact]
public void RelativeProjectDirectory_IsRejected_IndependentOfProcessCwd()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to validate setting CWD.

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.

2 participants