fix: Skip redundant git config writes in SignIn when already correct#2298
Open
derrickstolee wants to merge 2 commits intogit-ecosystem:mainfrom
Open
fix: Skip redundant git config writes in SignIn when already correct#2298derrickstolee wants to merge 2 commits intogit-ecosystem:mainfrom
derrickstolee wants to merge 2 commits intogit-ecosystem:mainfrom
Conversation
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 <noreply@anthropic.com>
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
git fetchon Azure Repos would unconditionally writegit config --global credential.azrepos:org/<org>.usernameandgit config --local --unset credential.azrepos:org/<org>.usernameon every invocation, even when the binding state was already correct.SignInnow checks whether the desired state is in place and skips config writes when nothing needs to change.SetCallCount/UnsetCallCountcounters toTestGitConfigurationso tests can assert whether config operations actually fired.Root cause
On every
git fetch, git callscredential approveafter successful authentication, which invokesStoreCredentialAsync→SignIn. Theelsebranch ofSignIn(global absent or matches signing-in user) calledBind(global)+Unbind(local)unconditionally. In the common single-user steady state (A | - → A | -) this issued two no-opgit configwrites per fetch.Behavior changes
A | -(steady state)Set(global)+Unset(local)A | ASet(global)+Unset(local)Unset(local)onlyA | BSet(global)+Unset(local)Unset(local)onlyB | A(steady state)Bind(local)(no-op)Test plan
Microsoft.AzureRepos.Teststests pass