Skip to content

Commit c6e4b33

Browse files
authored
fix: Skip redundant git config writes in SignIn when already correct (#2298)
## Summary - Fixes a bug where `git fetch` on Azure Repos would unconditionally write `git config --global credential.azrepos:org/<org>.username` and `git config --local --unset credential.azrepos:org/<org>.username` on every invocation, even when the binding state was already correct. - `SignIn` now checks whether the desired state is in place and skips config writes when nothing needs to change. - Adds `SetCallCount`/`UnsetCallCount` counters to `TestGitConfiguration` so tests can assert whether config operations actually fired. ## Root cause On every `git fetch`, git calls `credential approve` after successful authentication, which invokes `StoreCredentialAsync` → `SignIn`. The `else` branch of `SignIn` (global absent or matches signing-in user) called `Bind(global)` + `Unbind(local)` unconditionally. In the common single-user steady state (`A | - → A | -`) this issued two no-op `git config` writes per fetch. ## Behavior changes | State before SignIn | Before fix | After fix | |---------------------|------------|-----------| | `A \| -` (steady state) | `Set(global)` + `Unset(local)` | no writes | | `A \| A` | `Set(global)` + `Unset(local)` | `Unset(local)` only | | `A \| B` | `Set(global)` + `Unset(local)` | `Unset(local)` only | | `B \| A` (steady state) | `Bind(local)` (no-op) | no writes | ## Test plan - [x] 4 new tests assert exact write counts for each no-op/minimal-write case - [x] All 165 existing `Microsoft.AzureRepos.Tests` tests pass
2 parents 27ee403 + efcd785 commit c6e4b33

File tree

3 files changed

+107
-3
lines changed

3 files changed

+107
-3
lines changed

src/shared/Microsoft.AzureRepos.Tests/AzureReposBindingManagerTests.cs

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -628,6 +628,93 @@ public void AzureReposBindingManager_SignIn_OtherGlobalOtherLocal_BindsLocal()
628628
Assert.Equal(user2, actualGlobalUser);
629629
}
630630

631+
// Idempotency: SignIn when state is already correct should not write to git config
632+
633+
[Fact]
634+
public void AzureReposBindingManager_SignIn_SameGlobalNoLocal_NoConfigWrites()
635+
{
636+
// Steady-state: global already bound to signing-in user, no local override.
637+
// This is the common case on every 'git fetch' after the first sign-in.
638+
const string orgName = "org";
639+
const string user1 = "user1";
640+
641+
var git = new TestGit();
642+
var trace = new NullTrace();
643+
var manager = new AzureReposBindingManager(trace, git);
644+
645+
git.Configuration.Global[CreateKey(orgName)] = new[] {user1};
646+
647+
manager.SignIn(orgName, user1);
648+
649+
Assert.Equal(0, git.Configuration.SetCallCount);
650+
Assert.Equal(0, git.Configuration.UnsetCallCount);
651+
}
652+
653+
[Fact]
654+
public void AzureReposBindingManager_SignIn_OtherGlobalSameLocal_NoConfigWrites()
655+
{
656+
// Steady-state: a different user holds the global binding, and local is already
657+
// bound to the signing-in user. No change needed.
658+
const string orgName = "org";
659+
const string user1 = "user1";
660+
const string user2 = "user2";
661+
662+
var git = new TestGit();
663+
var trace = new NullTrace();
664+
var manager = new AzureReposBindingManager(trace, git);
665+
666+
git.Configuration.Global[CreateKey(orgName)] = new[] {user2};
667+
git.Configuration.Local[CreateKey(orgName)] = new[] {user1};
668+
669+
manager.SignIn(orgName, user1);
670+
671+
Assert.Equal(0, git.Configuration.SetCallCount);
672+
Assert.Equal(0, git.Configuration.UnsetCallCount);
673+
}
674+
675+
[Fact]
676+
public void AzureReposBindingManager_SignIn_SameGlobalSameLocal_OnlyUnbindsLocal()
677+
{
678+
// Global already matches, local redundantly mirrors it.
679+
// Only the local unset is needed; re-writing the global value is wasteful.
680+
const string orgName = "org";
681+
const string user1 = "user1";
682+
683+
var git = new TestGit();
684+
var trace = new NullTrace();
685+
var manager = new AzureReposBindingManager(trace, git);
686+
687+
git.Configuration.Global[CreateKey(orgName)] = new[] {user1};
688+
git.Configuration.Local[CreateKey(orgName)] = new[] {user1};
689+
690+
manager.SignIn(orgName, user1);
691+
692+
Assert.Equal(0, git.Configuration.SetCallCount);
693+
Assert.Equal(1, git.Configuration.UnsetCallCount);
694+
}
695+
696+
[Fact]
697+
public void AzureReposBindingManager_SignIn_SameGlobalOtherLocal_OnlyUnbindsLocal()
698+
{
699+
// Global already matches, local has a different user that needs removing.
700+
// Only the local unset is needed; re-writing the global value is wasteful.
701+
const string orgName = "org";
702+
const string user1 = "user1";
703+
const string user2 = "user2";
704+
705+
var git = new TestGit();
706+
var trace = new NullTrace();
707+
var manager = new AzureReposBindingManager(trace, git);
708+
709+
git.Configuration.Global[CreateKey(orgName)] = new[] {user1};
710+
git.Configuration.Local[CreateKey(orgName)] = new[] {user2};
711+
712+
manager.SignIn(orgName, user1);
713+
714+
Assert.Equal(0, git.Configuration.SetCallCount);
715+
Assert.Equal(1, git.Configuration.UnsetCallCount);
716+
}
717+
631718
#endregion
632719

633720
#region SignOut

src/shared/Microsoft.AzureRepos/AzureReposBindingManager.cs

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -283,12 +283,24 @@ public static void SignIn(this IAzureReposBindingManager bindingManager, string
283283
if (existingBinding?.GlobalUserName != null &&
284284
!StringComparer.OrdinalIgnoreCase.Equals(existingBinding.GlobalUserName, userName))
285285
{
286-
bindingManager.Bind(orgName, userName, local: true);
286+
// Global is bound to a different user (B); bind this user locally (-> B | A).
287+
// Skip the write if local is already correct.
288+
if (!StringComparer.OrdinalIgnoreCase.Equals(existingBinding.LocalUserName, userName))
289+
{
290+
bindingManager.Bind(orgName, userName, local: true);
291+
}
287292
}
288293
else
289294
{
290-
bindingManager.Bind(orgName, userName, local: false);
291-
bindingManager.Unbind(orgName, local: true);
295+
// Global is absent or already matches; ensure global is set and local is clear.
296+
if (existingBinding?.GlobalUserName is null)
297+
{
298+
bindingManager.Bind(orgName, userName, local: false);
299+
}
300+
if (existingBinding?.LocalUserName is not null)
301+
{
302+
bindingManager.Unbind(orgName, local: true);
303+
}
292304
}
293305
}
294306

src/shared/TestInfrastructure/Objects/TestGitConfiguration.cs

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,9 @@ public class TestGitConfiguration : IGitConfiguration
1616
public IDictionary<string, IList<string>> Local { get; set; } =
1717
new Dictionary<string, IList<string>>(GitConfigurationKeyComparer.Instance);
1818

19+
public int SetCallCount { get; private set; }
20+
public int UnsetCallCount { get; private set; }
21+
1922
#region IGitConfiguration
2023

2124
public void Enumerate(GitConfigurationLevel level, GitConfigurationEnumerationCallback cb)
@@ -68,6 +71,7 @@ public bool TryGet(GitConfigurationLevel level, GitConfigurationType type, strin
6871

6972
public void Set(GitConfigurationLevel level, string name, string value)
7073
{
74+
SetCallCount++;
7175
IDictionary<string, IList<string>> dict = GetDictionary(level);
7276

7377
if (!dict.TryGetValue(name, out var values))
@@ -107,6 +111,7 @@ public void Add(GitConfigurationLevel level, string name, string value)
107111

108112
public void Unset(GitConfigurationLevel level, string name)
109113
{
114+
UnsetCallCount++;
110115
IDictionary<string, IList<string>> dict = GetDictionary(level);
111116

112117
// Simulate git

0 commit comments

Comments
 (0)