Git config #12201

mstv · 2025-02-17T21:02:50Z

Fixes #4389

Proposed changes

Remove IConfigFileSettings including its implementation so that GE does not rewrite git config files any longer
Instead, use "git config" commands (with caching) for reading and writing git config settings
using IGitModule:
- GetEffectiveSetting
- GetEffectiveSetting<T>
- GetSettings(string setting) (for multi-value settings, kept existing sub-optimal name)
- SetSetting
- UnsetSetting
Provide new API in IGitModule:
- GetAllLocalSettings
- InvalidateGitSettings
- RemoveConfigSection
Remove API:
- from IGitModule:
  - GetSetting (kept, but declared "deprecated", redirected to GetEffectiveSetting)
  - GetSetting<T> (unused)
  - GetGitSetting
  - GetEffectiveGitSetting
  - EffectiveConfigFile
  - LocalConfigFile
- fromIGitRef
  - GetTrackingRemote & GetMergeWith (redundant to existing properties)

(Review commit by commit is recommended.)

Screenshots

N/A

Test methodology

existing tests (with adaptations)
tested with WSL

Please do not squash merge

✒️ I contribute this code under The Developer Certificate of Origin.

and declare EffectiveConfigFile read-only & private and fixup signature of IGitModule.PullCmd Refs: gitextensions#12201

because it brings insignificant speedup in its current form Refs: gitextensions#12201

and use factory method Refs: gitextensions#12201

and SetGitSetting over using LocalConfigFile Refs: gitextensions#12201

with EffectiveGitConfigSettings and remove IGitModule.GetGitSetting & GetEffectiveGitSetting Refs: gitextensions#12201

with GitConfigSettings Refs: gitextensions#12201

as replacement for IConfigFileSettings.GetValues Refs: gitextensions#12201

using extended IGitConfigSettingsGetter Refs: gitextensions#12201

Refs: gitextensions#12201

with GetAllLocalSettings & SetSetting Refs: gitextensions#12201

including implementation Refs: gitextensions#12201

and SetGitSetting over using LocalConfigFile Refs: gitextensions#12201

Refs: gitextensions#12201

with GetAllLocalSettings & SetSetting Refs: gitextensions#12201

including implementation Refs: gitextensions#12201

Copilot

Copilot reviewed 36 out of 36 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

src/app/GitCommands/Settings/GitConfigSettings.cs:145

The use of the null-forgiving operator on 'firstValue' assumes it is non-null. Consider adding a safeguard or verifying that 'firstValue' can never be null to avoid potential runtime issues.

_multiValueSettings.Add(name, [firstValue!, value]);

src/app/GitCommands/Git/Commands.Execution.cs:87

The introduction of the append flag in SetGitSetting alters the command line arguments for git config settings. Verify that this change aligns with the intended behavior and that all callers supply the appropriate value for the new parameter.

{ isSet && append, newSyntax ? "--append" : "--add" },

src/app/GitUI/Editor/FileViewer.cs

RussKie

Not a deep review, but I think this is a good change.
How good is the test coverage?

src/app/GitCommands/Git/GitModule.cs

vbjay · 2025-04-28T11:56:59Z

More a point of concern to keep an eye out for is your (with caching) point.

Maybe a test could be added

Generate random number
git config test.number number here
Read test.number
Confirm got expected value
New number and save to config
Read one more time and confirm not cached value.

My concern is the cache may hold onto values that have been changed and annoy people. I love the use of git config. Maybe a watcher on the git config files to invalidate the cache may be needed.

mstv · 2025-04-28T19:41:59Z

My concern is the cache may hold onto values that have been changed and annoy people.

There are only two locations in GE which change settings:

GitModule.SetGitSetting, which is used by all related IGitModule functions.
GitConfigSettingsSet.Save is used by the settings dialog.

Both invalidate all sibling caches.
If this would not work well, a lot of integration tests would not pass.

Though you are right. I need to invalidate the caches of GitModule yet after closing the settings dialog.

Maybe a watcher on the git config files to invalidate the cache may be needed.

This would become complex (think of "include") up to not feasible (FileWatcher does not work for WSL).
I have chosen a simple approach, which should be clear and good enough for the user (but does not and does not need to address the invalidation after the settings dialog).

gitextensions/src/app/GitUI/GitModuleForm.cs

Lines 130 to 142 in 479107f

    
           protected override void OnApplicationActivated() 
        
           { 
        
               base.OnApplicationActivated(); 
        
               if (_isReactivation) 
        
               { 
        
                   Module.InvalidateGitSettings(); 
        
               } 
        
               else 
        
               { 
        
                   _isReactivation = true; 
        
               } 
        
           }

.

vbjay · 2025-04-29T00:10:24Z

Makes sense. Then maybe a force refresh config button to handle the case of config being altered outside of app.

and declare EffectiveConfigFile read-only & private and fixup signature of IGitModule.PullCmd Refs: gitextensions#12201

because it brings insignificant speedup in its current form Refs: gitextensions#12201

and use factory method Refs: gitextensions#12201

and SetGitSetting over using LocalConfigFile Refs: gitextensions#12201

with EffectiveGitConfigSettings and remove IGitModule.GetGitSetting & GetEffectiveGitSetting Refs: gitextensions#12201

with GitConfigSettings Refs: gitextensions#12201

as replacement for IConfigFileSettings.GetValues Refs: gitextensions#12201

using extended IGitConfigSettingsGetter Refs: gitextensions#12201

Refs: gitextensions#12201

with GetAllLocalSettings & SetSetting Refs: gitextensions#12201

including implementation Refs: gitextensions#12201

mstv · 2025-05-02T16:11:36Z

Then maybe a force refresh config button to handle the case of config being altered outside of app.

Just hit the Windows key twice (in order to trigger OnApplicationActivated) if one really changed git config without GE but not leaving GE.

vbjay · 2025-05-04T17:32:42Z

I'm thinking more along the lines of say Visual Studio changes a config or whatnot. The app calls on git config to get needed settings often and if stale and used cache it will be out of sync. I'd imagine it would be a pain to load settings page to force invalidating cache. If cache had a clock on it that self invalidates after 10 minutes then ok. I'm just saying the cache invalidation needs to be considered.

If ge loads settings at noon and knows x config. If visual studio changes at 12:05 and you do more in ge when does ge catch up to current config is my point.

mstv self-assigned this Feb 17, 2025

mstv marked this pull request as draft February 17, 2025 21:28

mstv force-pushed the git_config branch 5 times, most recently from db9f16c to f1bc5c2 Compare February 25, 2025 17:24

mstv added a commit to mstv/gitextensions that referenced this pull request Feb 25, 2025

chore: Remove downcasts to ConfigFileSettings

dcabdf7

and declare EffectiveConfigFile read-only & private and fixup signature of IGitModule.PullCmd Refs: gitextensions#12201

mstv added a commit to mstv/gitextensions that referenced this pull request Feb 25, 2025

refactor(IGitRef): Remove redundant API

e361f0b

because it brings insignificant speedup in its current form Refs: gitextensions#12201

mstv added a commit to mstv/gitextensions that referenced this pull request Feb 25, 2025

fix(LocalConfigFile): Avoid always creating new instance

c1c7f86

and use factory method Refs: gitextensions#12201

mstv added a commit to mstv/gitextensions that referenced this pull request Feb 25, 2025

chore: Prefer read-only effective settings

cc5c869

and SetGitSetting over using LocalConfigFile Refs: gitextensions#12201

mstv force-pushed the git_config branch from f1bc5c2 to 60be0fb Compare February 25, 2025 22:37

mstv added a commit to mstv/gitextensions that referenced this pull request Feb 25, 2025

refactor: Replace ConfigFileSettings.CreateEffective

6629f72

with EffectiveGitConfigSettings and remove IGitModule.GetGitSetting & GetEffectiveGitSetting Refs: gitextensions#12201

mstv added a commit to mstv/gitextensions that referenced this pull request Feb 25, 2025

refactor: Replace ConfigFileSettings.CreateGlobal

76fcd0b

with GitConfigSettings Refs: gitextensions#12201

mstv added a commit to mstv/gitextensions that referenced this pull request Feb 25, 2025

refactor: Add IGitConfigSettingsGetter.GetValues

d133506

as replacement for IConfigFileSettings.GetValues Refs: gitextensions#12201

mstv added a commit to mstv/gitextensions that referenced this pull request Feb 25, 2025

refactor: Implement GetLocalTrackingBranchName

17db299

using extended IGitConfigSettingsGetter Refs: gitextensions#12201

mstv added a commit to mstv/gitextensions that referenced this pull request Feb 25, 2025

refactor: Add IGitModule.RemoveConfigSection

9a49b97

Refs: gitextensions#12201

mstv added a commit to mstv/gitextensions that referenced this pull request Feb 25, 2025

refactor: Replace GetConfigSections & AddConfigSection

44d5c53

with GetAllLocalSettings & SetSetting Refs: gitextensions#12201

mstv added a commit to mstv/gitextensions that referenced this pull request Feb 25, 2025

chore: Remove IConfigFileSettings

60be0fb

including implementation Refs: gitextensions#12201

mstv force-pushed the git_config branch 2 times, most recently from b3e54f1 to daa8a0d Compare March 3, 2025 17:50

mstv mentioned this pull request Mar 4, 2025

CI does not build a merge of the PR with the master branch #12237

Closed

mstv force-pushed the git_config branch 7 times, most recently from b1e055c to 1473e00 Compare March 10, 2025 22:27

mstv added a commit to mstv/gitextensions that referenced this pull request Mar 11, 2025

chore: Prefer read-only effective settings

c6b90e9

and SetGitSetting over using LocalConfigFile Refs: gitextensions#12201

mstv added a commit to mstv/gitextensions that referenced this pull request Mar 31, 2025

refactor: Add IGitModule.RemoveConfigSection

a77aa4d

Refs: gitextensions#12201

mstv force-pushed the git_config branch from 6abe7ed to 5a194f3 Compare March 31, 2025 21:05

mstv added a commit to mstv/gitextensions that referenced this pull request Mar 31, 2025

refactor: Replace GetConfigSections & AddConfigSection

13859a7

with GetAllLocalSettings & SetSetting Refs: gitextensions#12201

mstv added a commit to mstv/gitextensions that referenced this pull request Mar 31, 2025

chore: Remove IConfigFileSettings

5a194f3

including implementation Refs: gitextensions#12201

mstv requested a review from Copilot April 6, 2025 15:50

Copilot AI reviewed Apr 6, 2025

View reviewed changes

src/app/GitUI/Editor/FileViewer.cs Show resolved Hide resolved

mstv added the status: ready label Apr 7, 2025

RussKie reviewed Apr 27, 2025

View reviewed changes

src/app/GitCommands/Git/GitModule.cs Outdated Show resolved Hide resolved

RussKie approved these changes Apr 28, 2025

View reviewed changes

mstv removed the status: ready label May 2, 2025

mstv added this to the v5.3 milestone May 2, 2025

mstv added 11 commits May 2, 2025 14:37

chore: Remove downcasts to ConfigFileSettings

d27b29a

and declare EffectiveConfigFile read-only & private and fixup signature of IGitModule.PullCmd Refs: gitextensions#12201

refactor(IGitRef): Remove redundant API

625b463

because it brings insignificant speedup in its current form Refs: gitextensions#12201

fix(LocalConfigFile): Avoid always creating new instance

0576faf

and use factory method Refs: gitextensions#12201

chore: Prefer read-only effective settings

09a02fa

and SetGitSetting over using LocalConfigFile Refs: gitextensions#12201

refactor: Replace ConfigFileSettings.CreateEffective

5791a69

with EffectiveGitConfigSettings and remove IGitModule.GetGitSetting & GetEffectiveGitSetting Refs: gitextensions#12201

refactor: Replace ConfigFileSettings.CreateGlobal

f1bb94b

with GitConfigSettings Refs: gitextensions#12201

refactor: Add IGitConfigSettingsGetter.GetValues

9563678

as replacement for IConfigFileSettings.GetValues Refs: gitextensions#12201

refactor: Implement GetLocalTrackingBranchName

d000c00

using extended IGitConfigSettingsGetter Refs: gitextensions#12201

refactor: Add IGitModule.RemoveConfigSection

89b296f

Refs: gitextensions#12201

refactor: Replace GetConfigSections & AddConfigSection

5eacca1

with GetAllLocalSettings & SetSetting Refs: gitextensions#12201

chore: Remove IConfigFileSettings

be7a870

including implementation Refs: gitextensions#12201

mstv force-pushed the git_config branch from fa4300d to be7a870 Compare May 2, 2025 12:43

mstv merged commit be7a870 into gitextensions:master May 2, 2025
4 checks passed

mstv deleted the git_config branch May 2, 2025 16:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Git config #12201

Git config #12201

Uh oh!

mstv commented Feb 17, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

RussKie left a comment

Uh oh!

Uh oh!

vbjay commented Apr 28, 2025 •

edited

Loading

Uh oh!

mstv commented Apr 28, 2025

Uh oh!

vbjay commented Apr 29, 2025

Uh oh!

Uh oh!

mstv commented May 2, 2025

Uh oh!

vbjay commented May 4, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Git config #12201

Git config #12201

Uh oh!

Conversation

mstv commented Feb 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes

Screenshots

Test methodology

Please do not squash merge

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

RussKie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vbjay commented Apr 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mstv commented Apr 28, 2025

Uh oh!

vbjay commented Apr 29, 2025

Uh oh!

Uh oh!

mstv commented May 2, 2025

Uh oh!

vbjay commented May 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

mstv commented Feb 17, 2025 •

edited

Loading

vbjay commented Apr 28, 2025 •

edited

Loading

vbjay commented May 4, 2025 •

edited

Loading