From 944726b23ae377fe015830be87044b04a073f8fa Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Fri, 20 Mar 2026 13:47:25 -0400 Subject: [PATCH 1/3] test: Add write-call tracking to TestGitConfiguration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Context: TestGitConfiguration.Set and Unset directly manipulate in-memory dictionaries. Tests can assert final dictionary state easily, but have no way to detect whether redundant writes occurred — a write that sets a key to its already-current value, or unsets a key that is already absent. Justification: Simple integer counters on TestGitConfiguration give tests a lightweight, zero-noise way to detect whether config operations fired at all, without introducing a mocking framework or separate spy class. Counters are reset on construction so they reflect only the writes within a single test. Implementation: Added SetCallCount and UnsetCallCount as auto-incremented public properties. Each increments at the top of the corresponding method, before any dictionary logic. Co-Authored-By: Claude Sonnet 4.6 --- .../TestInfrastructure/Objects/TestGitConfiguration.cs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/shared/TestInfrastructure/Objects/TestGitConfiguration.cs b/src/shared/TestInfrastructure/Objects/TestGitConfiguration.cs index 517a8c7b8..6a887a97c 100644 --- a/src/shared/TestInfrastructure/Objects/TestGitConfiguration.cs +++ b/src/shared/TestInfrastructure/Objects/TestGitConfiguration.cs @@ -16,6 +16,9 @@ public class TestGitConfiguration : IGitConfiguration public IDictionary> Local { get; set; } = new Dictionary>(GitConfigurationKeyComparer.Instance); + public int SetCallCount { get; private set; } + public int UnsetCallCount { get; private set; } + #region IGitConfiguration public void Enumerate(GitConfigurationLevel level, GitConfigurationEnumerationCallback cb) @@ -68,6 +71,7 @@ public bool TryGet(GitConfigurationLevel level, GitConfigurationType type, strin public void Set(GitConfigurationLevel level, string name, string value) { + SetCallCount++; IDictionary> dict = GetDictionary(level); if (!dict.TryGetValue(name, out var values)) @@ -107,6 +111,7 @@ public void Add(GitConfigurationLevel level, string name, string value) public void Unset(GitConfigurationLevel level, string name) { + UnsetCallCount++; IDictionary> dict = GetDictionary(level); // Simulate git From 2226b75fdc5b85464cb7105d44ae06c59dd8b3c7 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Fri, 20 Mar 2026 13:47:50 -0400 Subject: [PATCH 2/3] fix: Skip redundant git config writes in SignIn when already correct MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Context: On every `git fetch`, git calls `credential approve` after successful authentication, which invokes GCM's StoreCredentialAsync. For OAuth flows against Azure Repos, this calls SignIn(orgName, userName) to record the user-to-org binding in git config. In the common single-user steady state — global binding already set to the authenticated user, no local override — SignIn was still issuing `git config --global credential.azrepos:org/.username` and `git config --local --unset credential.azrepos:org/.username` on every invocation, even though neither write was needed. Justification: The else branch of SignIn that handles "global absent or matches" called Bind(global) + Unbind(local) unconditionally, without first checking whether the state was already correct. The fix adds minimal guards: skip Bind(global) if global is already set to the signing-in user, skip Unbind(local) if local is already absent. This eliminates the round-trips to git in the steady state without changing the outcome in any other case. The same pattern applies to the B|A case (different user holds the global binding, local is already set to the signing-in user): that branch also skipped the write check, so it is guarded here too. Implementation: Modified SignIn to skip writes when already in the desired state: A | - -> A | - no writes (previously Set(global)+Unset(local)) A | A -> A | - only Unset(local), Set(global) skipped A | B -> A | - only Unset(local), Set(global) skipped B | A -> B | A no writes (already correct, was also correct) Added four tests to AzureReposBindingManagerTests using the new SetCallCount/UnsetCallCount counters to assert the precise number of config writes for each case. Co-Authored-By: Claude Sonnet 4.6 --- .../AzureReposBindingManagerTests.cs | 87 +++++++++++++++++++ .../AzureReposBindingManager.cs | 18 +++- 2 files changed, 102 insertions(+), 3 deletions(-) diff --git a/src/shared/Microsoft.AzureRepos.Tests/AzureReposBindingManagerTests.cs b/src/shared/Microsoft.AzureRepos.Tests/AzureReposBindingManagerTests.cs index 2c506ae62..ec60d8f70 100644 --- a/src/shared/Microsoft.AzureRepos.Tests/AzureReposBindingManagerTests.cs +++ b/src/shared/Microsoft.AzureRepos.Tests/AzureReposBindingManagerTests.cs @@ -628,6 +628,93 @@ public void AzureReposBindingManager_SignIn_OtherGlobalOtherLocal_BindsLocal() Assert.Equal(user2, actualGlobalUser); } + // Idempotency: SignIn when state is already correct should not write to git config + + [Fact] + public void AzureReposBindingManager_SignIn_SameGlobalNoLocal_NoConfigWrites() + { + // Steady-state: global already bound to signing-in user, no local override. + // This is the common case on every 'git fetch' after the first sign-in. + const string orgName = "org"; + const string user1 = "user1"; + + var git = new TestGit(); + var trace = new NullTrace(); + var manager = new AzureReposBindingManager(trace, git); + + git.Configuration.Global[CreateKey(orgName)] = new[] {user1}; + + manager.SignIn(orgName, user1); + + Assert.Equal(0, git.Configuration.SetCallCount); + Assert.Equal(0, git.Configuration.UnsetCallCount); + } + + [Fact] + public void AzureReposBindingManager_SignIn_OtherGlobalSameLocal_NoConfigWrites() + { + // Steady-state: a different user holds the global binding, and local is already + // bound to the signing-in user. No change needed. + const string orgName = "org"; + const string user1 = "user1"; + const string user2 = "user2"; + + var git = new TestGit(); + var trace = new NullTrace(); + var manager = new AzureReposBindingManager(trace, git); + + git.Configuration.Global[CreateKey(orgName)] = new[] {user2}; + git.Configuration.Local[CreateKey(orgName)] = new[] {user1}; + + manager.SignIn(orgName, user1); + + Assert.Equal(0, git.Configuration.SetCallCount); + Assert.Equal(0, git.Configuration.UnsetCallCount); + } + + [Fact] + public void AzureReposBindingManager_SignIn_SameGlobalSameLocal_OnlyUnbindsLocal() + { + // Global already matches, local redundantly mirrors it. + // Only the local unset is needed; re-writing the global value is wasteful. + const string orgName = "org"; + const string user1 = "user1"; + + var git = new TestGit(); + var trace = new NullTrace(); + var manager = new AzureReposBindingManager(trace, git); + + git.Configuration.Global[CreateKey(orgName)] = new[] {user1}; + git.Configuration.Local[CreateKey(orgName)] = new[] {user1}; + + manager.SignIn(orgName, user1); + + Assert.Equal(0, git.Configuration.SetCallCount); + Assert.Equal(1, git.Configuration.UnsetCallCount); + } + + [Fact] + public void AzureReposBindingManager_SignIn_SameGlobalOtherLocal_OnlyUnbindsLocal() + { + // Global already matches, local has a different user that needs removing. + // Only the local unset is needed; re-writing the global value is wasteful. + const string orgName = "org"; + const string user1 = "user1"; + const string user2 = "user2"; + + var git = new TestGit(); + var trace = new NullTrace(); + var manager = new AzureReposBindingManager(trace, git); + + git.Configuration.Global[CreateKey(orgName)] = new[] {user1}; + git.Configuration.Local[CreateKey(orgName)] = new[] {user2}; + + manager.SignIn(orgName, user1); + + Assert.Equal(0, git.Configuration.SetCallCount); + Assert.Equal(1, git.Configuration.UnsetCallCount); + } + #endregion #region SignOut diff --git a/src/shared/Microsoft.AzureRepos/AzureReposBindingManager.cs b/src/shared/Microsoft.AzureRepos/AzureReposBindingManager.cs index 7e26590bd..65006a8bb 100644 --- a/src/shared/Microsoft.AzureRepos/AzureReposBindingManager.cs +++ b/src/shared/Microsoft.AzureRepos/AzureReposBindingManager.cs @@ -283,12 +283,24 @@ public static void SignIn(this IAzureReposBindingManager bindingManager, string if (existingBinding?.GlobalUserName != null && !StringComparer.OrdinalIgnoreCase.Equals(existingBinding.GlobalUserName, userName)) { - bindingManager.Bind(orgName, userName, local: true); + // Global is bound to a different user (B); bind this user locally (-> B | A). + // Skip the write if local is already correct. + if (!StringComparer.OrdinalIgnoreCase.Equals(existingBinding.LocalUserName, userName)) + { + bindingManager.Bind(orgName, userName, local: true); + } } else { - bindingManager.Bind(orgName, userName, local: false); - bindingManager.Unbind(orgName, local: true); + // Global is absent or already matches; ensure global is set and local is clear. + if (existingBinding?.GlobalUserName == null) + { + bindingManager.Bind(orgName, userName, local: false); + } + if (existingBinding?.LocalUserName != null) + { + bindingManager.Unbind(orgName, local: true); + } } } From efcd7855e458df8dfad2ba35f48961e934244b5f Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Fri, 27 Mar 2026 09:35:41 -0400 Subject: [PATCH 3/3] Use 'is null' over '== null' and similar Co-authored-by: Matthew John Cheetham --- src/shared/Microsoft.AzureRepos/AzureReposBindingManager.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/shared/Microsoft.AzureRepos/AzureReposBindingManager.cs b/src/shared/Microsoft.AzureRepos/AzureReposBindingManager.cs index 65006a8bb..2ae7fd11a 100644 --- a/src/shared/Microsoft.AzureRepos/AzureReposBindingManager.cs +++ b/src/shared/Microsoft.AzureRepos/AzureReposBindingManager.cs @@ -293,11 +293,11 @@ public static void SignIn(this IAzureReposBindingManager bindingManager, string else { // Global is absent or already matches; ensure global is set and local is clear. - if (existingBinding?.GlobalUserName == null) + if (existingBinding?.GlobalUserName is null) { bindingManager.Bind(orgName, userName, local: false); } - if (existingBinding?.LocalUserName != null) + if (existingBinding?.LocalUserName is not null) { bindingManager.Unbind(orgName, local: true); }