Let git deny the commit if committer is not specified #11369
Conversation
| @@ -2713,14 +2713,29 @@ private void UpdateAuthorInfo() | |||
| ThreadHelper.FileAndForget(async () => | |||
| { | |||
| // Do not cache results in order to update the info on FormActivate | |||
| string userName = Module.GetEffectiveGitSetting(SettingKeyString.UserName, cache: false); | |||
There was a problem hiding this comment.
Do not try-catch if the reason is known - in this case git-log return 0 is OK
This is the part of #11146 I approved (the rest of the discussion there just got me loose motivation for GE and a dev in general though).
Could be an option to GetEffectiveGitSetting, but this is currently the only caller.
It is also not necessary to call UpdateAuthorInfo() in OnActivate.
There was a problem hiding this comment.
Thank you for the patch. I did not think of its availability when I decided to not touch the ExecutableExtensions.GetOutput() chain.
It is also not necessary to call UpdateAuthorInfo() in OnActivate.
The setting could be changed using another app (instance).
But removing OnActivate was the easiest way to get rid of repeated exception popups.
There was a problem hiding this comment.
Look at the status bar. On activate is on purpose so that the commiter and author info is updated when user activates window because git config could have been executed. It is to make sure user sees up to date info.
There was a problem hiding this comment.
It is to make sure user sees up to date info.
As said it is not worth it. If users use another app / instance for changing the settings, they can reopen FormCommit if they wonder whether the settings are in effect or not. That's not that unusual.
There was a problem hiding this comment.
I agree with @mstv, it's not worth it.
- Return null if not setting is not set - Remove OnActivated
GitUI/CommandsDialogs/FormCommit.cs
Outdated
| string userName = Module.GetEffectiveGitSetting(SettingKeyString.UserName, cache: false) ?? "USER NOT CONFIGURED"; | ||
| string userEmail = Module.GetEffectiveGitSetting(SettingKeyString.UserEmail, cache: false) ?? "E-MAIL NOT CONFIGURED"; |
There was a problem hiding this comment.
I'm not a fan of the screaming strings. Maybe something like this?
| string userName = Module.GetEffectiveGitSetting(SettingKeyString.UserName, cache: false) ?? "USER NOT CONFIGURED"; | |
| string userEmail = Module.GetEffectiveGitSetting(SettingKeyString.UserEmail, cache: false) ?? "E-MAIL NOT CONFIGURED"; | |
| string userName = Module.GetEffectiveGitSetting(SettingKeyString.UserName, cache: false) ?? "<username not configured>"; | |
| string userEmail = Module.GetEffectiveGitSetting(SettingKeyString.UserEmail, cache: false) ?? "<email not configured>"; |
There was a problem hiding this comment.
changed, but using "/" instead of "<>" which occurs in the e-mail address.
1 1/2 review comment
|
Backport to release/4.0? |
- Return null if git config setting is not set and display "/... not configured/" - Remove FormCommit.OnActivated in order to avoid repeated exception popups Co-authored-by: Jay Asbury <vbjay@users.noreply.github.com> Co-authored-by: Gerhard Olsson <gerhardol@users.noreply.github.com> (cherry picked from commit 44be238)
done |
|
Thank you |
Fixes #11366
Fixes #11368
Proposed changes
GetEffectiveGitSettingreturnnullif the setting is not setFormCommit.OnActivatedin order to avoid repeated exception popups from the only called functionUpdateAuthorInfoScreenshots
Before
Exception (repeated exceptions unless quickly repeating
Esc)After
Test methodology
Merge strategy
I agree that the maintainer squash merge this PR (if the commit message is clear).
✒️ I contribute this code under The Developer Certificate of Origin.