Skip to content

Commit 2226b75

Browse files
derrickstoleeclaude
andcommitted
fix: Skip redundant git config writes in SignIn when already correct
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/<org>.username` and `git config --local --unset credential.azrepos:org/<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 <noreply@anthropic.com>
1 parent 944726b commit 2226b75

File tree

2 files changed

+102
-3
lines changed

2 files changed

+102
-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 == null)
297+
{
298+
bindingManager.Bind(orgName, userName, local: false);
299+
}
300+
if (existingBinding?.LocalUserName != null)
301+
{
302+
bindingManager.Unbind(orgName, local: true);
303+
}
292304
}
293305
}
294306

0 commit comments

Comments
 (0)