Skip to content

Fix clone authentication when includeIf is in git config #2100

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
tyrielv wants to merge 1 commit into
base: vnext
Choose a base branch
from
Open

Fix clone authentication when includeIf is in git config #2100

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 36 additions & 5 deletions AGENTS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,8 @@ Skips `dotnet publish`, AOT, native C++ projects, payload assembly, installer.
For changes that need the GVFS payload (`gvfs.exe`, hooks, service) but not
an installer. `PublishAot=false` skips ilc (~3–4 min saved);
`SkipCreateInstaller=true` skips Inno Setup (~95 s saved).
`GVFS.Payload` cascades to its dependencies (GVFS, GVFS.Mount, GVFS.Hooks,
GVFS.Service) via `ProjectReference`.
`GVFS.Payload` only assembles the payload directory — it does **not** build or
publish the projects it copies from (see the warning below).

> **Prerequisite: the native C++ projects must already be built.** They are
> `.vcxproj` (see [Native C++ projects](#native-c-projects-need-msbuild-not-dotnet-build)
Expand All @@ -78,22 +78,53 @@ GVFS.Service) via `ProjectReference`.
```powershell
dotnet publish src\GVFS\GVFS.FunctionalTests\GVFS.FunctionalTests.csproj `
-c Debug /p:PublishAot=false
dotnet publish src\GVFS\GVFS\GVFS.csproj `
-c Debug /p:PublishAot=false
dotnet publish src\GVFS\GVFS.Payload\GVFS.Payload.csproj `
-c Debug /p:PublishAot=false /p:SkipCreateInstaller=true

src\scripts\RunFunctionalTests-Dev.ps1 Debug --test=GVFS.FunctionalTests.Tests.<Namespace>.<Class>.<Method>
src\scripts\RunFunctionalTests-Dev.ps1 -Configuration Debug -Arch x64 `
--test=GVFS.FunctionalTests.Tests.<Namespace>.<Class>.<Method>
```

`layout.bat` (invoked by GVFS.Payload) `xcopy`s from each project's `publish\`
or native-output directory — the C# projects do not require AOT, so
`PublishAot=false` produces a fully functional test payload. The native
hook binaries are copied straight from the vcxproj output.

> **⚠️ Publish each changed project explicitly — `GVFS.Payload` does not do it
> for you.** `GVFS.Payload.csproj` is a `Microsoft.Build.NoTargets` project with
> **no `ProjectReference` items at all**; its `CreatePayload` target just runs
> `layout.bat`, which `xcopy`s from each project's existing output/publish
> directory. Nothing in that chain rebuilds or republishes those projects, so a
> project you did not publish contributes whatever was left there by the last
> `Build.bat` — an AOT binary that predates your edit. The build succeeds and the
> test then runs stale code, which looks exactly like "my fix did not work".
>
> Verify the payload actually changed before you trust a functional-test result:
>
> ```powershell
> Get-Item out\GVFS.Payload\bin\Debug\win-x64\GVFS.exe |
> Format-List LastWriteTime, Length
> ```
>
> A ~27 MB `GVFS.exe` is the AOT build from `Build.bat`; a ~160 KB one is the
> `PublishAot=false` build from the command above. If the timestamp predates
> your edit, publish the project that owns the changed code and re-run the
> payload publish. The same applies to `GVFS.Mount`, `GVFS.Hooks`, and
> `GVFS.Service` when you change those.

`RunFunctionalTests-Dev.ps1` runs functional tests against the build output
without requiring admin or a system-wide install. It launches the test
service as a console process. Each invocation gets a unique service name
and data dir, so concurrent runs from different worktrees don't collide.

> **Pass `-Configuration` and `-Arch` by name.** The script's first two
> positional parameters are `Configuration` and `Arch`, so a bare
> `RunFunctionalTests-Dev.ps1 Debug --test=...` binds `--test=...` to `-Arch`
> and fails its `ValidateSet`. Name both parameters, then let `--test=` fall
> through to `ExtraArgs`.

### Path C — Installer build (~5 min — only when you need an installer)

For producing an installable package (testing install/upgrade flows, or
Expand Down Expand Up @@ -136,11 +167,11 @@ participate in the C# inner-loop paths above.
```powershell
# ✅ Correct
& "out\GVFS.UnitTests\bin\...\GVFS.UnitTests.exe" --test "GVFS.UnitTests.Common.WorktreeInfoTests"
src\scripts\RunFunctionalTests-Dev.ps1 Debug --test=GVFS.FunctionalTests.Tests.GVFSVerbTests.UnknownVerb
src\scripts\RunFunctionalTests-Dev.ps1 -Configuration Debug -Arch x64 --test=GVFS.FunctionalTests.Tests.GVFSVerbTests.UnknownVerb

# ❌ Wrong — silently runs the entire suite
& "out\GVFS.UnitTests\bin\...\GVFS.UnitTests.exe" --where "class =~ Worktree"
src\scripts\RunFunctionalTests-Dev.ps1 Debug --where "cat == Smoke"
src\scripts\RunFunctionalTests-Dev.ps1 -Configuration Debug -Arch x64 --where "cat == Smoke"
```

For unit tests, `--where` is merely annoying (the whole suite runs in
Expand Down
116 changes: 103 additions & 13 deletions GVFS/GVFS.Common/Git/GitProcess.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,10 +210,9 @@ public virtual bool TryDeleteCredential(ITracer tracer, string repoUrl, string u

string stdinConfig = sb.ToString();

Result result = this.InvokeGitAgainstDotGitFolder(
Result result = this.InvokeGitAgainstDotGitFolderOrOutsideEnlistment(
GenerateCredentialVerbCommand("reject"),
stdin => stdin.Write(stdinConfig),
null,
usePreCommandHook: false);

if (result.ExitCodeIsFailure)
Expand All @@ -238,10 +237,9 @@ public virtual bool TryStoreCredential(ITracer tracer, string repoUrl, string us

string stdinConfig = sb.ToString();

Result result = this.InvokeGitAgainstDotGitFolder(
Result result = this.InvokeGitAgainstDotGitFolderOrOutsideEnlistment(
GenerateCredentialVerbCommand("approve"),
stdin => stdin.Write(stdinConfig),
null,
usePreCommandHook: false);

if (result.ExitCodeIsFailure)
Expand Down Expand Up @@ -275,10 +273,9 @@ public virtual bool TryGetCertificatePassword(
{
// See GetFromConfig for why pre-command hook is disabled
// for bootstrap-time git operations.
Result gitCredentialOutput = this.InvokeGitAgainstDotGitFolder(
Result gitCredentialOutput = this.InvokeGitAgainstDotGitFolderOrOutsideEnlistment(
"credential fill",
stdin => stdin.Write("protocol=cert\npath=" + certificatePath + "\nusername=\n\n"),
parseStdOutLine: null,
usePreCommandHook: false);

if (gitCredentialOutput.ExitCodeIsFailure)
Expand Down Expand Up @@ -328,19 +325,21 @@ public virtual bool TryGetCredential(

using (ITracer activity = tracer.StartActivity(nameof(this.TryGetCredential), EventLevel.Informational))
{
// See GetFromConfig for why pre-command hook is disabled
// for bootstrap-time git operations.
Result gitCredentialOutput = this.InvokeGitAgainstDotGitFolder(
Result gitCredentialOutput = this.InvokeGitAgainstDotGitFolderOrOutsideEnlistment(
GenerateCredentialVerbCommand("fill"),
stdin => stdin.Write($"url={repoUrl}\n\n"),
parseStdOutLine: null,
out bool usedDotGitFolder,
usePreCommandHook: false,
timeoutMs: timeoutMs);

if (gitCredentialOutput.ExitCodeIsFailure)
{
EventMetadata errorData = new EventMetadata();

// Records whether repo-local configuration (and therefore a
// repo-local credential.helper) was visible to git.
errorData.Add(nameof(usedDotGitFolder), usedDotGitFolder);

if (gitCredentialOutput.Errors.StartsWith("Operation timed out"))
{
errorMessage = "Credential manager did not respond within " + (timeoutMs / 1000) + " seconds";
Expand Down Expand Up @@ -442,7 +441,13 @@ public Result SetInFileConfig(string configFile, string settingName, string valu
public bool TryGetConfigUrlMatch(string section, string repositoryUrl, out Dictionary<string, GitConfigSetting> configSettings)
{
// See GetFromConfig for why pre-command hook is disabled.
Result result = this.InvokeGitAgainstDotGitFolder($"config --get-urlmatch {section} {repositoryUrl}", usePreCommandHook: false);
// This runs from the GitAuthentication constructor, which happens before
// clone creates the enlistment, so it must tolerate a missing .git folder.
Result result = this.InvokeGitAgainstDotGitFolderOrOutsideEnlistment(
$"config --get-urlmatch {section} {repositoryUrl}",
writeStdIn: null,
usePreCommandHook: false);

if (result.ExitCodeIsFailure)
{
configSettings = null;
Expand Down Expand Up @@ -1109,7 +1114,8 @@ private Result InvokeGitOutsideEnlistment(
string command,
Action<StreamWriter> writeStdIn,
Action<string> parseStdOutLine,
int timeout = -1)
int timeout = -1,
bool usePreCommandHook = true)
{
return this.InvokeGitImpl(
command,
Expand All @@ -1118,7 +1124,91 @@ private Result InvokeGitOutsideEnlistment(
useReadObjectHook: false,
writeStdIn: writeStdIn,
parseStdOutLine: parseStdOutLine,
timeoutMs: timeout);
timeoutMs: timeout,
usePreCommandHook: usePreCommandHook);
}

/// <summary>
/// Invokes git.exe against an enlistment's .git folder when that folder exists,
/// and outside the enlistment when it does not.
/// </summary>
/// <remarks>
/// For commands that prefer an enlistment's configuration but do not require a
/// repository to run. Naming a --git-dir that is absent is not merely redundant:
/// git resolves that path before it evaluates an 'includeIf "gitdir:..."'
/// condition, so a user who has such a section in their config gets
/// "fatal: Invalid path ...: No such file or directory" and the command never
/// runs. The condition does not have to match for this to happen.
///
/// The credential verbs need this because 'gvfs clone' authenticates before it
/// creates the enlistment. This method should be used only with commands that
/// still behave correctly with no repository, because there is no repo-local
/// configuration to read on the fallback path.
/// </remarks>
private Result InvokeGitAgainstDotGitFolderOrOutsideEnlistment(
string command,
Action<StreamWriter> writeStdIn,
Action<string> parseStdOutLine = null,
bool usePreCommandHook = true,
int timeoutMs = -1)
{
return this.InvokeGitAgainstDotGitFolderOrOutsideEnlistment(
command,
writeStdIn,
out bool _,
parseStdOutLine,
usePreCommandHook,
timeoutMs);
}

/// <summary>
/// Overload that reports which route was taken, so callers can record it.
/// </summary>
/// <param name="usedDotGitFolder">
/// True when --git-dir was passed and repo-local configuration was therefore
/// visible to git; false when the command ran with no repository.
/// </param>
private Result InvokeGitAgainstDotGitFolderOrOutsideEnlistment(
string command,
Action<StreamWriter> writeStdIn,
out bool usedDotGitFolder,
Action<string> parseStdOutLine = null,
bool usePreCommandHook = true,
int timeoutMs = -1)
{
// Evaluate once so the reported route always matches the route taken.
usedDotGitFolder = this.DotGitRootExists();

if (usedDotGitFolder)
{
return this.InvokeGitAgainstDotGitFolder(
command,
writeStdIn,
parseStdOutLine,
usePreCommandHook: usePreCommandHook,
timeoutMs: timeoutMs);
}

return this.InvokeGitOutsideEnlistment(
command,
writeStdIn,
parseStdOutLine,
timeout: timeoutMs,
usePreCommandHook: usePreCommandHook);
}

/// <summary>
/// Determines whether the enlistment's .git exists. In a linked worktree .git is a
/// file that points at the real git directory rather than a folder, so check for both.
/// </summary>
private bool DotGitRootExists()
{
if (string.IsNullOrEmpty(this.dotGitRoot))
{
return false;
}

return Directory.Exists(this.dotGitRoot) || File.Exists(this.dotGitRoot);
}

/// <summary>
Expand Down
Loading
Loading