-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP GPG Improvements Part 1 #10473
base: master
Are you sure you want to change the base?
WIP GPG Improvements Part 1 #10473
Conversation
d692a2b
to
8a727ba
Compare
2c45fb9
to
43d2060
Compare
dc20831
to
ad7102b
Compare
@vbjay a friendly nudge. Is this still something you're working on? |
Yes. I need to rework some forms and add more tests. |
0f058a9
to
a9e6d43
Compare
10396a9
to
c1166f5
Compare
Some comments from my initial review before I look at this again:
|
I am trying to make it reflect what will happen when committing. As in if user has commit/tag.GpgSign set then it will automatically say using default key in the right ui. Same with what key. For this to be reliable it MUST fetch to the minute config. Same with the idea of getting what keys exist at the moment. I am sorry but 108MS is nothing and I don't see the issue. |
180 ms is certainly noticeable (and this may be a lot more in other setups). Some more features like this and a few seconds are added. If you use the functions, the delay is fine, but irritating otherwise. |
So basically neuter the feature behind a flag that will never get set like it took forever for us to turn on the show gpg tab before? KInda irritated with your feedback. You are basically complaining because I am adding functionality that it take more time to load. I reject this idea of neutering behind a flag because then it will fail to reflect what will happen and is kinda the whole point of this feature. It is to not only make it easy to manage the signing in the ui and also provide visibility into what will happen when git is told to create the commit or tag.
|
What I can do is maybe see if I can put some of the setup in onshown so it doesn't affect first show time. diff --git a/GitUI/CommandsDialogs/FormCommit.cs b/GitUI/CommandsDialogs/FormCommit.cs
index ac9882b48..711533eaa 100644
--- a/GitUI/CommandsDialogs/FormCommit.cs
+++ b/GitUI/CommandsDialogs/FormCommit.cs
@@ -156,6 +156,8 @@ public sealed partial class FormCommit : GitModuleForm
private TranslationString _invalidSignCaption = new("Commit Signing Options");
#endregion
+ private Stopwatch _st = new();
+
private event Action? OnStageAreaLoaded;
private readonly ICommitTemplateManager _commitTemplateManager;
@@ -347,8 +349,6 @@ public FormCommit(GitUICommands commands, CommitKind commitKind = CommitKind.Nor
InitializeComplete();
- toolStripGpgKeyComboBox.UICommandsSource = this;
-
// By calling this in the constructor, we prevent flickering caused by resizing the
// form, for example when it is restored to maximised, but first drawn as a smaller window.
RestorePosition();
@@ -514,14 +514,6 @@ protected override void OnLoad(EventArgs e)
// The problem is likely caused by 'splitRight.FixedPanel = FixedPanel.Panel2' fact, but other forms
// have the same setting, and don't appear to suffer from the same bug.
splitRight.SplitterDistance -= 6;
-
- // If not changed, by default show "no sign commit"
- if (gpgSignCommitToolStripComboBox.SelectedIndex == -1)
- {
- var gpg = toolStripGpgKeyComboBox.KeysUIController;
- bool commitSign = gpg.GetCommitGPGSign();
- gpgSignCommitToolStripComboBox.SelectedIndex = commitSign ? 1 : 0;
- }
}
protected override void OnShown(EventArgs e)
@@ -571,6 +563,7 @@ protected override void OnShown(EventArgs e)
}
base.OnShown(e);
+ Debugger.Log(0, "GpgInfo", $"GPG stuff: {_st.ElapsedMilliseconds}ms{Environment.NewLine}");
return;
@@ -1021,6 +1014,23 @@ private async Task UpdateBranchNameDisplayAsync()
private void Initialize(bool loadUnstaged = true)
{
+ if (!_initialized)
+ {
+ _st.Start();
+
+ // If not changed, by default show "no sign commit"
+
+ toolStripGpgKeyComboBox.UICommandsSource = this;
+ if (gpgSignCommitToolStripComboBox.SelectedIndex == -1)
+ {
+ var gpg = toolStripGpgKeyComboBox.KeysUIController;
+ bool commitSign = gpg.GetCommitGPGSign();
+ gpgSignCommitToolStripComboBox.SelectedIndex = commitSign ? 1 : 0;
+ }
+
+ _st.Stop();
+ }
+
_initialized = true;
ThreadHelper.JoinableTaskFactory.RunAsync(() => UpdateBranchNameDisplayAsync());
|
Why not async (as for user name & e-mail)? |
@@ -110,15 +110,15 @@ public static class ExecutableExtensions | |||
if (input is not null) | |||
{ | |||
#if DEBUG | |||
System.Diagnostics.Debug.WriteLine($"git {arguments} {Encoding.UTF8.GetString(input)}"); | |||
System.Diagnostics.Debug.WriteLine($"{executable.FileNameProvider()} {arguments} {Encoding.UTF8.GetString(input)}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not convinced this is efficient. This method essentially re-runs the method that was run by executable.Start
(line 130). There's no guarantee this method is side-effect free.
I suggest we do something like this:
diff --git a/GitCommands/Git/Executable.cs b/GitCommands/Git/Executable.cs
index 3942ddec3..c81d29056 100644
--- a/GitCommands/Git/Executable.cs
+++ b/GitCommands/Git/Executable.cs
@@ -13,6 +13,7 @@ public sealed class Executable : IExecutable
{
private readonly string _workingDir;
private readonly Func<string> _fileNameProvider;
+ private string? _executableFileName;
private readonly string _prefixArguments;
public Executable(string fileName, string workingDir = "")
@@ -27,6 +28,10 @@ public Executable(Func<string> fileNameProvider, string workingDir = "", string
_prefixArguments = prefixArguments;
}
+#if DEBUG
+ internal string? ExecutableFileName => _executableFileName;
+#endif
+
/// <inheritdoc />
public IProcess Start(ArgumentString arguments = default,
bool createWindow = false,
@@ -41,9 +46,9 @@ public Executable(Func<string> fileNameProvider, string workingDir = "", string
var args = (arguments.Arguments ?? "").Replace("$QUOTE$", "\\\"");
- var fileName = _fileNameProvider();
+ _executableFileName = _fileNameProvider();
- return new ProcessWrapper(fileName, _prefixArguments, args, _workingDir, createWindow, redirectInput, redirectOutput, outputEncoding, useShellExecute, throwOnErrorExit);
+ return new ProcessWrapper(_executableFileName, _prefixArguments, args, _workingDir, createWindow, redirectInput, redirectOutput, outputEncoding, useShellExecute, throwOnErrorExit);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure.
@@ -10,6 +10,7 @@ public class GitVersion : IComparable<GitVersion> | |||
private static readonly GitVersion v2_32_0 = new("2.32.0"); | |||
private static readonly GitVersion v2_35_0 = new("2.35.0"); | |||
private static readonly GitVersion v2_38_0 = new("2.38.0"); | |||
private static readonly GitVersion v2_30_0 = new("2.30.0"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep these ordered.
GitUI/CommandsDialogs/FormCommit.cs
Outdated
GPGKeysUIController gpg = toolStripGpgKeyComboBox.KeysUIController; | ||
if (!gpg.ValidateCommitSign(gpgSignCommitToolStripComboBox.SelectedIndex > 0, toolStripGpgKeyComboBox.KeyID)) | ||
{ | ||
MessageBox.Show(this, TranslatedStrings.InvalidGpgSignOptions, _invalidSignCaption.Text, MessageBoxButtons.OK, MessageBoxIcon.Asterisk); | ||
toolStripMenuItem3.ShowDropDown(); | ||
gpgSignCommitToolStripComboBox.Focus(); | ||
|
||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks as though we're leaking implementation details. This should be a validation event on the combobox itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a validation of 2 fields. Index 0 is Do not sign Commit 1 and 2 are sign with default or sign with selected key. This validation is a you can't commit with these 2 values of these fields. The cases this covers are
- User has configured the commit.gpgSign value to true but does not have a default key so the key combobox will be no key selected. Since git config is not fully in our hands this state is possible
- would result in state of
git commit ... -S
along with the user.signingKey not being set. Git would complain and fail the commit.
- would result in state of
- User has selected sign with specific key and then failed to select a key.
User can skip the options menu completely. So I disagree with the validation event. Where the validation needs to occur is when they hit commit.
The validation logic is the same for tag.gpgSign also. Along with the validation being coded once in the controller and the ui needing to use it can do what it needs when it detects the state.
|
||
namespace GitUI.UserControls.GPGKeys | ||
{ | ||
public partial class GPGSecretKeysCombobox : GitModuleControl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example:
gitextensions/GitUI/CommandsDialogs/FormBrowse.cs
Lines 266 to 268 in 99ece76
fileToolStripMenuItem.Initialize(() => UICommands); | |
helpToolStripMenuItem.Initialize(() => UICommands); | |
toolsToolStripMenuItem.Initialize(() => UICommands); |
ToolStripFilters.Bind(() => Module, RevisionGrid); |
|
||
private void InitControllers() | ||
{ | ||
_secretKeysParser = new GPGSecretKeysParser(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should avoid newing up concrete implementations as that makes the testing story much more complicated. Absent of a DI, this should be passed in via Bind()
or Initialize
mechanisms as implemented in other parts of the codebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. WIll look into. Yes. DI would have helped a ton in this.
@@ -0,0 +1,50 @@ | |||
namespace GitUI.UserControls.GPGKeys | |||
{ | |||
public class ToolStripGPGSecretKeysComboBox : ToolStripControlHost |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
2cacfba
to
56bfe2e
Compare
Updates the method to return not only the value of the config but also the status based on the ExitCodes from git config documentation. This allows for deciding what to do if config key doesn't exist by the user of GetEffectiveGitSetting
Prevent gpg signing Prevents tests from waiting for user to unlock key if a commit is done in a test Set gpg path to "gpg" and expect it to be in git tools or path
Use GPG dropdowns in CreateTag form Use GPG dropdowns in FormCommit Use GPG dropdowns in FormMergeBranch Use GPG dropdowns in FormRebase
pass key id when needed Pass -S, -S{KeyID} or --no-gpg-sign as needed If no-gpg-sign is not supported by git version, fall back to passing -c commit.gpgSign=false
- commit.gpgSign is set and no default key and user did not select sign with specific key - tag.gpgSign is set and no default key and user did not select sign with specific key - user selected sign with specific key but failed to select a key
56bfe2e
to
8561995
Compare
Proposed changes
Screenshots
Before
TODO
After
Test methodology
Test environment(s)
Merge strategy