fix(git config settings pages): Add system-wide settings #12185

mstv · 2025-02-08T22:51:35Z

Fixes #10914
Fixes #11764

Proposed changes

Get git settings using git config list in order to correctly display effective settings
Display effective settings read-only (in order to make users explicitly decide in which scope to configure git)
Display system-wide settings (read-only)
Add advanced git config "merge.autostash"
Several refactorings:
- Extract IPersistentConfigValueStore from IConfigFileSettings
- Remove SettingsSourceExtension.ByPath
- Rename to GitConfigSettingsSet and to GitConfigBaseSettingsPage
- Add GitConfig/DistributedSettingsSet.Save
- Extract GitEncodingSettings from ConfigFileSettings

Review commit by commit is recommended.

Screenshots

Before

After

Test methodology

manual

Please do not squash merge

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

pmiossec · 2025-02-08T23:08:42Z

Display effective settings read-only (in order to make users explicitly decide in which scope to configure git)

😍

mstv · 2025-02-08T23:20:51Z

Display effective settings read-only (in order to make users explicitly decide in which scope to configure git)

😍

me included

Do you have a clue why this PR makes "your" testcase RebasePatchesBuiltAccordingToGitRebaseFiles fail, Philippe?

pmiossec · 2025-02-08T23:26:55Z

Do you have a clue why this PR makes "your" testcase RebasePatchesBuiltAccordingToGitRebaseFiles fail, Philippe?

The name of the test made me think that it was a flaky test so I relaunched the build.

I didn't remembered I wrote the test and had not looked at the possible reason.
I will need to investigate on a computer (and not from my smartphone)

pmiossec · 2025-02-11T23:26:33Z

src/app/GitCommands/Git/GitModule.cs

        public string? GetGitSetting(string setting, string scopeArg, bool cache = false)
        {
-            GitArgumentBuilder args = new("config") { "--includes", scopeArg, "--get", setting };
+            GitArgumentBuilder args = new("config") { "get", scopeArg, "--includes", setting };


Suggested change

GitArgumentBuilder args = new("config") { "get", scopeArg, "--includes", setting };

GitArgumentBuilder args = new("config") { "--get", scopeArg, "--includes", setting };

Sorry, I was not aware of this git command line change in v2.46
I had to update my git version otherwise got exceptions. So this PR won't work with <v2.46...

Good to have this caught before merging!

pmiossec · 2025-02-12T00:32:20Z

I don't know if the behavior was like that before but now, when switching between git config "setting source", changes are saved immediately (without clicking 'Apply').

mstv · 2025-02-12T23:49:18Z

I don't know if the behavior was like that before but now, when switching between git config "setting source", changes are saved immediately (without clicking 'Apply').

Yes, this behavior has changed.
Global / local settings must be saved in order to update effective settings.
This could be postponed until effective settings become selected. A confirmation popup could be added, too.
But I think this is not needed because other settings are even saved at once.
Thoughts?

RussKie

🚀

RussKie · 2025-02-14T13:00:00Z

src/app/GitCommands/Git/GitModule.cs

@@ -316,11 +317,15 @@ public IConfigFileSettings LocalConfigFile
        // 4) branch, tag name, errors, warnings, hints encoded in system default encoding
        public static readonly Encoding LosslessEncoding = Encoding.GetEncoding("ISO-8859-1"); // is any better?

-        public Encoding FilesEncoding => ((ConfigFileSettings)EffectiveConfigFile).FilesEncoding ?? new UTF8Encoding(false);
+        private Encoding? _defaultEncoding;


[nit] move to the top

RussKie · 2025-02-14T13:02:12Z

src/app/GitCommands/Git/GitModule.cs

-        public Encoding FilesEncoding => ((ConfigFileSettings)EffectiveConfigFile).FilesEncoding ?? new UTF8Encoding(false);
+        private Encoding? _defaultEncoding;
+
+        private Encoding DefaultEncoding => _defaultEncoding ??= new UTF8Encoding(encoderShouldEmitUTF8Identifier: false);


Why is the field needed? Why not init the property straight away? Why not make it static?

👍
Yes, lazy instantiation is not worth it.

RussKie · 2025-02-14T13:05:15Z

src/app/GitCommands/Settings/EffectiveGitConfigSettings.cs

+namespace GitCommands.Settings;
+
+[DebuggerDisplay("{" + nameof(DebuggerDisplay) + ",nq}")]
+public sealed class EffectiveGitConfigSettings(IExecutable gitExecutable) : GitConfigSettingsBase(gitExecutable, GitSettingLevel.Effective), IConfigValueStore


Does this and the rest of the hierarchy need to be public?
Would be great to have some xml-docs for the types and public members

public because used by GUI project.
I have added xml-docs to classes and to protected stuff.
Public methods are interface implementations and inherit the xml-docs.

RussKie · 2025-02-14T13:06:46Z

src/app/GitCommands/Settings/EffectiveGitConfigSettings.cs

+
+    protected override void Update() => Update(Clear, StoreSetting);
+
+    private void Clear()


[nit] Keeping methods in alphabetical order helps navigating the code.

RussKie · 2025-02-14T13:09:09Z

src/app/GitCommands/Settings/GitConfigSettings.cs

+    {
+        if (gitSettingLevel == GitSettingLevel.Effective)
+        {
+            throw new ArgumentOutOfRangeException(nameof(gitSettingLevel), $"Use read-only {nameof(EffectiveGitConfigSettings)} instance instead.");


ArgumentOutOfRangeException has a number of Throw* helpers, may worth using those.

ArgumentOutOfRangeException has a number of Throw* helpers, may worth using those.

already checked, not enough: it is unable to compare enum values

These aren't flags, so compare as ints?

RussKie · 2025-02-14T13:12:03Z

src/app/GitUI/CommandsDialogs/SettingsDialog/GitConfigBaseSettingsPage.cs

+using GitExtensions.Extensibility.Settings;
+using Microsoft;
+
+namespace GitUI.CommandsDialogs.SettingsDialog


Suggested change

namespace GitUI.CommandsDialogs.SettingsDialog

namespace GitUI.CommandsDialogs.SettingsDialog;

Done. I had just renamed this class (in a sub-optimal way, but it is similar to GitConfigAdvancedSettingsPage).

pmiossec · 2025-02-15T13:20:37Z

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

+        bool isSet = !string.IsNullOrEmpty(value);
+        GitArgumentBuilder args = new("config")
+            {
+#if UseNewGitConfigSyntax2_46


Is there a particular reason to not use GitVersion.Current (i.e. creating a property GitVersion.Current.SupportNewGitConfigSyntax) instead?

gerhardol · 2025-02-18T13:53:47Z

This does not work with WSL

Some paths like editor and mergetool is expected to be in Windows format.
This change picks up the WSL settings if you edit for a WSL repo, so the Windows settings are garbled.
GE expects Windows paths for these tools.

In some way it is an improvement, but not working yet

gerhardol

WSL Git settings are messed up, not possible to e.g. merge in WSL

gerhardol · 2025-02-22T22:37:46Z

How to proceed with this PR (and the follow up)?
This PR has some issues with WSL (like an issue to save), most is in the follow up.
This could though promote WSL Git from being a semi hidden use case to give full support.

Some issues I see, no full investigation.

GE uses win-Git also in WSL for some operations. Most prominently for merges. I guess most of this could be implemented in WSL.
GE reading Git settings (like mergetool path, checking existence) to determine what feature to enable. Ignore for WSL?
GE saving/updating settings. Only read for WSL?

mstv · 2025-02-22T23:32:47Z

I have already changed it to always use the native - i.e. Windows - git for getting & setting git config.

We could add another selection to these two git settings pages which switches between native git and WSL git.

gerhardol · 2025-02-23T09:48:16Z

I have already changed it to always use the native - i.e. Windows - git for getting & setting git config.

We could add another selection to these two git settings pages which switches between native git and WSL git.

You still get an exception in WSL just switching to local view.
I have not debugged, you have so many changes going on here so it would just be conflicts

GitExtensions.Extensibility.ExternalOperationException
HResult=0x80131500
Message=Error getting config values
fatal: --local can only be used inside a git repository

Source=GitExtensions.Extensibility
StackTrace:
at GitExtensions.Extensibility.ExecutionResult.ThrowIfErrorExit(String errorMessage) in C:\dev\gc\gitextensions_4\src\app\GitExtensions.Extensibility\ExecutionResult.cs:line 36
at GitCommands.Git.Commands.GetGitSettings(IExecutable gitExecutable, GitSettingLevel settingLevel) in C:\dev\gc\gitextensions_4\src\app\GitCommands\Git\Commands.Execution.cs:line 43
at GitCommands.Settings.GitConfigSettingsBase.Update(Action clear, Action2 storeSetting) in C:\dev\gc\gitextensions_4\src\app\GitCommands\Settings\GitConfigSettingsBase.cs:line 121 at GitCommands.Settings.GitConfigSettings.Update() in C:\dev\gc\gitextensions_4\src\app\GitCommands\Settings\GitConfigSettings.cs:line 113 at GitCommands.Settings.GitConfigSettings.GetValue(String name) in C:\dev\gc\gitextensions_4\src\app\GitCommands\Settings\GitConfigSettings.cs:line 46 at GitCommands.Settings.SettingsSource1.GetValue(String name) in C:\dev\gc\gitextensions_4\src\app\GitCommands\Settings\SettingsSource_T.cs:line 17
at GitCommands.DiffMergeTools.DiffMergeToolConfigurationManager.get_ConfiguredMergeTool() in C:\dev\gc\gitextensions_4\src\app\GitCommands\DiffMergeTools\DiffMergeToolConfigurationManager.cs:line 41
at GitUI.CommandsDialogs.SettingsDialog.Pages.GitConfigSettingsPage.SettingsToPage() in C:\dev\gc\gitextensions_4\src\app\GitUI\CommandsDialogs\SettingsDialog\Pages\GitConfigSettingsPage.cs:line 84
at GitUI.CommandsDialogs.SettingsDialog.SettingsPageBase.LoadSettings() in C:\dev\gc\gitextensions_4\src\app\GitUI\CommandsDialogs\SettingsDialog\SettingsPageBase.cs:line 109
at GitUI.CommandsDialogs.SettingsDialog.GitConfigBaseSettingsPage.SetCurrentSettings(SettingsSource settings) in C:\dev\gc\gitextensions_4\src\app\GitUI\CommandsDialogs\SettingsDialog\GitConfigBaseSettingsPage.cs:line 66
at GitUI.CommandsDialogs.SettingsDialog.GitConfigBaseSettingsPage.SetLocalSettings() in C:\dev\gc\gitextensions_4\src\app\GitUI\CommandsDialogs\SettingsDialog\GitConfigBaseSettingsPage.cs:line 44
at GitUI.CommandsDialogs.SettingsDialog.SettingsPageHeader.<>c__DisplayClass5_0.b__0(Object s, EventArgs e) in C:\dev\gc\gitextensions_4\src\app\GitUI\CommandsDialogs\SettingsDialog\SettingsPageHeader.cs:line 78
at System.Windows.Forms.RadioButton.OnCheckedChanged(EventArgs e)
at System.Windows.Forms.RadioButton.set_Checked(Boolean value)
at System.Windows.Forms.RadioButton.OnClick(EventArgs e)
at System.Windows.Forms.RadioButton.OnMouseUp(MouseEventArgs mevent)
at System.Windows.Forms.Control.WmMouseUp(Message& m, MouseButtons button, Int32 clicks)
at System.Windows.Forms.Control.WndProc(Message& m)
at System.Windows.Forms.ButtonBase.WndProc(Message& m)
at System.Windows.Forms.Control.ControlNativeWindow.WndProc(Message& m)
at System.Windows.Forms.NativeWindow.Callback(HWND hWnd, MessageId msg, WPARAM wparam, LPARAM lparam)

Inner Exception 1:
Exception: Error getting config values
fatal: --local can only be used inside a git repository

mstv · 2025-02-24T20:54:46Z

I have already changed it to always use the native - i.e. Windows - git for getting & setting git config.

This was nonsense because git config settings must be set in WSL. Reverted.
The '$' in "$LOCAL" etc. must be escaped for WSL!
In addition, I have adapted the suggested command lines for diff/merge tools (not fully tested yet.)

We could add another selection to these two git settings pages which switches between native git and WSL git.

not really necessary:
Git settings pages affect WSL while a WSL repo is loaded.
(Global) Windows git settings can be set using settings pages opened from Dashboard.
(The Windows git cannot be used to access local settings of a WSL repo!)

GE uses win-Git also in WSL for some operations. Most prominently for merges. I guess most of this could be implemented in WSL.

should be done in a follow-up

GE reading Git settings (like mergetool path, checking existence) to determine what feature to enable. Ignore for WSL?

We should always use WSL commands. Then only WSL git config settings apply. And these are shown and controlled in the settings pages.

gerhardol · 2025-02-24T21:58:22Z

PR seem reasonable (and is an improvement especially for WSL eventually).
However, I cannot take it for a spin yet. Dashboard settings fail (readonly/lazy), WSL fails for me with popup.
(will review after initial tryout).

Refs: gitextensions#12185

and use it in GitConfigSettingsSet Refs: gitextensions#12185

Refs: gitextensions#12185

and enable ThreeState for all settings checkboxes Refs: gitextensions#12185

Refs: gitextensions#12185

mstv · 2025-03-16T20:16:02Z

Mergetool to use win need to be handled before next release, if not in this pr

Both ways to start a merge tool work well with this PR (with both suggestions adapting to WSL).
Only the follow-up PR #12201 (branch "git_config") needs something like GetNativeGitSetting.

and remove its non-null replacement ConfigFileSettings.GetValue and remove redundant, hardly used IConfigFileSettings.GetString Refs: gitextensions#12185

from IConfigFileSettings Move implementation of GetValue<T> to ISettingsValueGetter Move implementation of SetPathValue to PathSetting.ConvertPathToGitSetting SettingsSource.SettingLevel must not be modified Refs: gitextensions#12185

and IGitModule.GetEffectiveSettingsByPath and SettingsSource.Get/SetNullableEnum Refs: gitextensions#12185

and to GitConfigBaseSettingsPage Refs: gitextensions#12185

Refs: gitextensions#12185

from ConfigFileSettings Refs: gitextensions#12185

Refs: gitextensions#12185

and use it in GitConfigSettingsSet Refs: gitextensions#12185

Refs: gitextensions#12185

and enable ThreeState for all settings checkboxes Refs: gitextensions#12185

Refs: gitextensions#12185

gerhardol · 2025-03-25T22:46:18Z

not for me (Otherwise, I would have noticed the inconsistency, too, I guess)

only in some situations, but now I lost where I got it
another issue...

mstv self-assigned this Feb 8, 2025

mstv marked this pull request as draft February 9, 2025 07:41

mstv marked this pull request as ready for review February 9, 2025 09:24

mstv force-pushed the fix/i10914_system_config branch 2 times, most recently from 9d20c5a to 4814663 Compare February 10, 2025 22:03

mstv marked this pull request as draft February 11, 2025 18:45

mstv force-pushed the fix/i10914_system_config branch 2 times, most recently from 8544e32 to 5d6fac6 Compare February 11, 2025 22:15

mstv marked this pull request as ready for review February 11, 2025 22:58

pmiossec reviewed Feb 11, 2025

View reviewed changes

pmiossec force-pushed the fix/i10914_system_config branch 2 times, most recently from 5948041 to 5d6fac6 Compare February 12, 2025 00:24

mstv force-pushed the fix/i10914_system_config branch from 5d6fac6 to df542cc Compare February 12, 2025 23:34

mstv force-pushed the fix/i10914_system_config branch from df542cc to 3e22e0e Compare February 13, 2025 23:09

RussKie reviewed Feb 14, 2025

View reviewed changes

pmiossec reviewed Feb 15, 2025

View reviewed changes

mstv mentioned this pull request Feb 17, 2025

Git config #12201

Merged

gerhardol previously requested changes Feb 18, 2025

View reviewed changes

mstv force-pushed the fix/i10914_system_config branch from 0ac781a to 7ee491c Compare February 19, 2025 18:53

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

feat: Make effective settings read-only

e66bfdd

Refs: gitextensions#12185

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

chore: Use lowercase names for git config

b8fa199

Refs: gitextensions#12185

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

feat: Add GitConfigSettings

c183152

and use it in GitConfigSettingsSet Refs: gitextensions#12185

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

fix: Add settings source "System"

870531c

Refs: gitextensions#12185

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

feat(GitConfigAdvancedSettingsPage): Add merge.autoStash

39c73cc

and enable ThreeState for all settings checkboxes Refs: gitextensions#12185

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

fix(Files Encoding): Correctly display "unset"

b4d43c7

Refs: gitextensions#12185

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

chore(GitConfigBaseSettingsPage): Unindent namespace

d93ca83

Refs: gitextensions#12185

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

feat(GitConfig): Adapt diff/merge suggestion to WSL

fb3992c

Refs: gitextensions#12185

mstv force-pushed the fix/i10914_system_config branch from 6045a84 to 15c1383 Compare March 16, 2025 20:04

mstv added 14 commits March 25, 2025 21:24

refactor: nullable ISettingsValueGetter.GetValue

56acb64

and remove its non-null replacement ConfigFileSettings.GetValue and remove redundant, hardly used IConfigFileSettings.GetString Refs: gitextensions#12185

refactor: Extract IPersistentConfigValueStore

8408551

from IConfigFileSettings Move implementation of GetValue<T> to ISettingsValueGetter Move implementation of SetPathValue to PathSetting.ConvertPathToGitSetting SettingsSource.SettingLevel must not be modified Refs: gitextensions#12185

refactor: Remove SettingsSourceExtension.ByPath

af6a0da

and IGitModule.GetEffectiveSettingsByPath and SettingsSource.Get/SetNullableEnum Refs: gitextensions#12185

chore: Rename to GitConfigSettingsSet

f8a7cad

and to GitConfigBaseSettingsPage Refs: gitextensions#12185

refactor: Add GitConfig/DistributedSettingsSet.Save

377ee78

Refs: gitextensions#12185

refactor: Extract GitEncodingSettings

48c49f5

from ConfigFileSettings Refs: gitextensions#12185

feat: Make effective settings read-only

07e96c4

Refs: gitextensions#12185

chore: Use lowercase names for git config

c8df8d9

Refs: gitextensions#12185

feat: Add GitConfigSettings

63406a2

and use it in GitConfigSettingsSet Refs: gitextensions#12185

fix: Add settings source "System"

1242824

Refs: gitextensions#12185

feat(GitConfigAdvancedSettingsPage): Add merge.autoStash

41a924e

and enable ThreeState for all settings checkboxes Refs: gitextensions#12185

fix(Files Encoding): Correctly display "unset"

f47253c

Refs: gitextensions#12185

chore(GitConfigBaseSettingsPage): Unindent namespace

43c4f62

Refs: gitextensions#12185

feat(GitConfig): Adapt diff/merge suggestion to WSL

f3e694d

Refs: gitextensions#12185

mstv force-pushed the fix/i10914_system_config branch from b61926a to f3e694d Compare March 25, 2025 22:39

gerhardol approved these changes Mar 25, 2025

View reviewed changes

mstv merged commit f3e694d into gitextensions:master Mar 27, 2025
4 checks passed

mstv deleted the fix/i10914_system_config branch March 27, 2025 17:54

mstv added this to the v5.3 milestone Mar 27, 2025

	GitArgumentBuilder args = new("config") { "get", scopeArg, "--includes", setting };
	GitArgumentBuilder args = new("config") { "--get", scopeArg, "--includes", setting };


		protected override void Update() => Update(Clear, StoreSetting);

		private void Clear()

	namespace GitUI.CommandsDialogs.SettingsDialog
	namespace GitUI.CommandsDialogs.SettingsDialog;

Uh oh!

fix(git config settings pages): Add system-wide settings #12185

fix(git config settings pages): Add system-wide settings #12185

Uh oh!

Conversation

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

Uh oh!

Proposed changes

Screenshots

Before

After

Test methodology

Please do not squash merge

Uh oh!

pmiossec commented Feb 8, 2025

Uh oh!

mstv commented Feb 8, 2025

Uh oh!

pmiossec commented Feb 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmiossec commented Feb 12, 2025

Uh oh!

mstv commented Feb 12, 2025

Uh oh!

RussKie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gerhardol commented Feb 18, 2025

Uh oh!

gerhardol left a comment

Choose a reason for hiding this comment

Uh oh!

gerhardol commented Feb 22, 2025

Uh oh!

mstv commented Feb 22, 2025

Uh oh!

gerhardol commented Feb 23, 2025

Uh oh!

mstv commented Feb 24, 2025

Uh oh!

gerhardol commented Feb 24, 2025

Uh oh!

mstv commented Mar 16, 2025

Uh oh!

gerhardol commented Mar 25, 2025

Uh oh!

Uh oh!

Uh oh!

mstv commented Feb 8, 2025 •

edited

Loading

pmiossec Feb 12, 2025 •

edited

Loading