GetEffectiveGitSetting treats non 0 results as failures #11145
Comments
ghost
added
the
|
All commands where 0 is not a failure explicitly handles this. There are probably more of these commands than those that fails. (And it is mstv that has the credit of introducing this handling, I have though been involved in handling git errors. A number of unknown issues were fixed this way.) |
Which of these exit codes is not an error for If you need a |
Exactly my point but if you will take the time to do something like add a test that shows current implementation raises an exception if config key is not set. Try adding a test that looks for either a valid git config key found from |
|
Again, if you need a TryGet, implement one. If the error from the BTW: "config file cannot be written" is completely off-topic for a Get function. |
|
I don't think you see my point. I will walk you through it.
Exit code: 1 diff --git a/GitUI/CommandsDialogs/FormCommit.cs b/GitUI/CommandsDialogs/FormCommit.cs
index 8ee22871b..980fa7f5e 100644
--- a/GitUI/CommandsDialogs/FormCommit.cs
+++ b/GitUI/CommandsDialogs/FormCommit.cs
@@ -1319,6 +1319,9 @@ bool ConfirmAndStageAllUnstaged()
void DoCommit()
{
+ var catDog = Module.GetEffectiveGitSetting("cat.dog");
+ Console.WriteLine($"cat.dog:'{catDog}'");
+
if (Module.InTheMiddleOfConflictedMerge())
{
MessageBox.Show(this, _mergeConflicts.Text, _mergeConflictsCaption.Text, MessageBoxButtons.OK, MessageBoxIcon.Error);This is DEAD wrong. The config result is a blank string and exit code 1 says value unset. This is NOT an exception. I got the expected result. It is valid and did not fail and is not exceptional state. My fix does the following
I am against creating a try... function and leaving another function that just exceptions. My way provides status and allows for exceptional handling if needed with null status value and will prevent users reporting x error when it was really just that their config value was unset.
Setting config values. Note the exit codes 3, 4, 5. I know I am not dealing with setting values but I did provide the values just the same so that they could be used. |
|
With my change |
|
Yes I can add the config name and potentially when retrieved. There is room for metadata around this. |
|
What is unclear with ? What you want, is a |
That is the same error-hiding attitude as
That's why it is an |
|
Why raise a dialog saying it is an exception? Why say the blank value is wrong. Exit code of 1 means config value is unset and if specific state that user needs to respond...code it in a case or if...
|
|
Again: |
|
Do you run git try-getconfig? No. I am doing same 5hing here. Run git config and get the output and also store the result from exit code. Then use this result value as needed where you called. Exit code 1 is NOT an exceptional state. If git config blew up and returned say exit code 99. Yes that would be a exception. We can definitely show that. What I don't like is this get setting and it blows up vs a result that will always provide you details and each caller can deal as needed. |
|
Have you ever used a |
|
Sure I have worked with a generic dictionary. But I was working with potential more fields like contig key asked for just in case we do a whole git config fetch down the road. That is where I would do a Dictionary<string, MySettingType). I was more using this to define the result with OOP principles like being able to add ConfigKey, Value, Status, WhenRetrieved...Whatever down the road. I knew both of you would push back because you a don't quite seem to get exit codes and the fact that they don't always mean error and that I am pushing for consistency and one result of a git config value that provides all the info from one method. I hate the idea of a user trying to figure out do I do TryGetEffectiveGtSetting vs the other thing. Yes if exit code isn't defined in enum.. sure I can see an exception being thrown.. Frankly it is too easy to goof the Git Version of GetEffectiveGitSettkng vs the GetEffectiveSetting. But that is another topic. Yes expose git errors where needed but read the exit codes. 1 is clearly my example of cat.dog and attempting to retrieve it. Good example is if you don't have gpg.program set. In my gpg work and without me changing functionality, you get an error dialog with exit code 1. The program should internally handle that and based on what is desired ( just accept blank value, write special case code to handle this state...). Exceptions are for Exceptional state. An unset git config value is understood by everyone to return a blank value. Not everyone understands 1 is unset. Our program can help by going. I see that the value is blank and the status is unset... I can not create an exception and instead provide the blanks and Unset status. If I as a non admin user tried to run code that tried to set a system level git config value, our program could look at the status and see maybe a 3 or 4 result. http://www.codinghell.ch/blog/2013/03/31/how-and-when-not-to-use-exceptions/#:~:text=Don't%20use%20exceptions%20to,fields%20for%20flow%20control%20instead. If I can see the status and skip the whole exception crud, I would rather do that. Yes can add throw where needed but if 1, not an exception...Just return blank and unset to user. No need for exception building and throwing. |
|
Get in the context of git config does NOT bomb on unset is my main point. Your point about Get assumes values is set is silly. I have git config values that you probably don't. I would expect it to work where if unset it provides the blank. Pull my branch from the pull request and run the test
diff --git a/IntegrationTests/UI.IntegrationTests/GitUICommands/RunCommandTests.cs b/IntegrationTests/UI.IntegrationTests/GitUICommands/RunCommandTests.cs
index 701e9affa..0002a71c3 100644
--- a/IntegrationTests/UI.IntegrationTests/GitUICommands/RunCommandTests.cs
+++ b/IntegrationTests/UI.IntegrationTests/GitUICommands/RunCommandTests.cs
@@ -4,6 +4,7 @@
using GitCommands;
using GitExtensions.UITests;
using GitExtensions.UITests.CommandsDialogs;
+using GitExtUtils;
using GitUI;
using GitUI.CommandsDialogs;
using GitUIPluginInterfaces;
@@ -17,7 +18,7 @@ public sealed class RunCommandTests
{
// Created once for the fixture
private ReferenceRepository _referenceRepository;
-
+ private string[] _allConfigs;
// Created once for each test
private GitUICommands _commands;
@@ -35,6 +36,10 @@ public void SetUp()
_referenceRepository.Module.GitExecutable.RunCommand("config --local diff.guitool cmd").Should().BeTrue();
_referenceRepository.Module.GitExecutable.RunCommand("config --local merge.guitool cmd").Should().BeTrue();
+ ExecutionResult getAllComfigsResult = _referenceRepository.Module.GitExecutable.Execute(new GitArgumentBuilder("help", gitOptions: (ArgumentString)"--no-pager") { "--config" });
+ _allConfigs = getAllComfigsResult.StandardOutput.Split('\n').Where(cfg => !string.IsNullOrWhiteSpace(cfg) && !cfg.Contains("git help config"))
+ .Concat(new string[] { "a.a.C", "$#@@#$%&#@", "1234", "~x.Y", "", " ", " test", null })
+ .ToArray();
AppSettings.UseConsoleEmulatorForCommands = false;
AppSettings.CloseProcessDialog = true;
AppSettings.UseBrowseForFileHistory.Value = false;
@@ -361,6 +366,21 @@ public void RunCommandBasedOnArgument_unsupported(string command)
});
}
+ [Test]
+ [Description("Makes sure getting a git config value does not raise an exception "
+ + "and even if config value is not set it will just return a blank value")]
+ public void ValidateAllConfigOptionsWork()
+ {
+ var settings = _allConfigs.Select(cfg => new { Setting = _referenceRepository.Module.GetEffectiveGitSetting(cfg, false), ConfigName = cfg }).ToArray();
+ settings.Should().NotContainNulls();
+ settings.Select(s => s.Setting).Should().NotContainNulls();
+
+ foreach (var setting in settings)
+ {
+ Console.WriteLine($"{setting.ConfigName}:{setting.Setting}");
+ }
+ }
+
private static void ClickButton(Form form, string buttonName)
=> form.FindDescendantOfType<Button>(button => button.Name == buttonName).PerformClick();
GetEffectiveGitSetting should not assume config is set. it should mimic the way git config works and provide the value like git config does where unset. |
|
If the caller is OK with exit code 1 is OK, then it may be OK to have a specific access function like The other error codes still seem as errors to me - or they need special handling too. |
Environment
Issue description
As seen in https://git-scm.com/docs/git-config#_description it is not an error to get an exit code of non 0. Right now we get an exception because of the changes to GetOutput and treating all non 0 exit codes as exceptions instead of handing the result.
Not understanding exit codes as non errors is a failure on executable handling. Found during gpg work on a clean machine ant it looking for gpg.Program and commit.gpgSign and such. Which when not set, exit code is 1. Which tells you that value is not set.
Steps to reproduce
Change the name and email fetching in formCommit to get
usser.ValueDid this work in previous version of GitExtensions?
Maybe before the changes @mstv and @gerhardol did with changing the way executable output is handled with GetOutput from the executable.
Diagnostics
No response
The text was updated successfully, but these errors were encountered: