Update GetEffectiveGitSetting to provide status and value #11146
Conversation
|
GitCommands/Git/GitModule.cs
Outdated
| return GitExecutable.GetOutput(args, cache: cache ? GitCommandCache : null).Trim(); | ||
| ExecutionResult result = GitExecutable.Execute(args, cache: cache ? GitCommandCache : null, throwOnErrorExit: false); | ||
|
|
||
| int resultCode = result.ExitCode ?? 0; |
There was a problem hiding this comment.
Do not hide execution errors, should be an error.
There was a problem hiding this comment.
Wrong. Kind of what I am pointing out. It is NOT a failure. You and @mstv need to understand exit codes and when they are and not a failure. I detailed in related issue if you will read tied issue.
There was a problem hiding this comment.
Git config on a config value that is not set still returns a config value. Bash scripts don't go nuts because value isn't set. We shouldn't either. What I have done is provide not only the config value but a yes I got a value(config key found) Succes status or here your value is blank but you know config value was unset and if we run a set and get an exit code not 0 we have the reason. How about you take the time to read the related issue and understand the change instead of knee jerk reviewing.
There was a problem hiding this comment.
If there is no exit code, it is definitely an error.
There was a problem hiding this comment.
If there is no exit code, it is definitely an error.
I can change that. It was just a nullable so had to handle that to make it a value. Doubt it would ever be null.
| /// </summary> | ||
| public enum GitConfigStatus | ||
| { | ||
| CannotWriteToConfigFile = 4, |
There was a problem hiding this comment.
With this tight dependency on Git, the behaviour should be tested with real Git commands, where reasonable
There was a problem hiding this comment.
Read on further and note the tests.
There was a problem hiding this comment.
There are no tests to see that for instance CannotWriteToConfigFile is set when expected. This behavior can be changed in Git updates.
If this knowledge is not needed, no need to add the enums.
There was a problem hiding this comment.
I am providing all known git config values in the enum. Just because we use a custom config file work instead of actually reading from parsing git config list run doesn't mean the value can't be used in the future. The idea being we can call git config value and get back an EffectiveGitSetting or what not. Still thinking about correct type name that makes sense for both potentials. So I disagree. I am laying groundwork for both sets and gets. For now doing get but the full potential git config exit codes are represented in enum. I can look at git source code and look for what causes each exit code to better understand that. The rest I have makes sure that GetEffectiveGitSetting does not fail and also lists each config key with the exit code grouping. Running the code for every config so far shows exit code 1 and 0. If you want to pull my branch and run the test and verify...you can do so to provide verification of just 1 and 0.
There was a problem hiding this comment.
If you add something, it must be maintainable. If just 0/1 are expected, all others can be ignored, I see no direct need to handle them in the future. The extra values can be comments.
There was a problem hiding this comment.
I am providing the full enum at time of creation of the enum. I have done the research of the git source code and understand each exit code and when it shows up. I have documented it. This is one of a few prs I am going to be doing in git config. For example down the road I plan on still keeping file source classes from the git config file stuff but I want all git config reads to be read in by actually reading git config instead of us doing something custom with file reads and writes. Still looking into the setting side where we want to set multiple keys. That will be in a whole different pr/ issue. I am laying the foundation for full git config driving and handling things we don't currently handle like multi valued configs and such.
There was a problem hiding this comment.
I think both points are valid. The risk of git changing these return codes is probably quite low, likely they'd break a lot of people worldwide. At the same time, I find @gerhardol point about maintainability slightly more compelling, if we don't have an immediate use for some piece of code, I prefer not to add it. IMO it's totally valid to note all the possible return codes git may return for the config reading operation (thank you @vbjay for spending time finding those), but only expose the values we need. In the future, if a need to handle other return code arise, we can uncomment the necessary lines.
| { | ||
| public record struct EffectiveGitSetting(GitConfigStatus? Status, string Value) | ||
| { | ||
| public static implicit operator (GitConfigStatus? status, string value)(EffectiveGitSetting value) |
There was a problem hiding this comment.
Avoid value for arguments, there are several layers with the same name
There was a problem hiding this comment.
What do you suggest instead of the "Value" from the config entry and still having status and config key?
There was a problem hiding this comment.
or something, instead of the contextual keyword https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/value
| public static implicit operator (GitConfigStatus? status, string value)(EffectiveGitSetting value) | |
| public static implicit operator (GitConfigStatus? status, string errorMessage)(EffectiveGitSetting gitSetting) |
There was a problem hiding this comment.
In this instance Value represents git setting value, so I think the name is appropriate. I would ask for xml-docs clearly explaining the nature and the purpose for all public API though.
There was a problem hiding this comment.
Yep. Def was going to do so once feedback was provided.
There was a problem hiding this comment.
But Value vs value is confusing here, i specific name would add value, this is not a generic interface. What is it that the value holds?
| { | ||
| public record struct EffectiveGitSetting(GitConfigStatus? Status, string Value) | ||
| { | ||
| public static implicit operator (GitConfigStatus? status, string value)(EffectiveGitSetting value) |
There was a problem hiding this comment.
In this instance Value represents git setting value, so I think the name is appropriate. I would ask for xml-docs clearly explaining the nature and the purpose for all public API though.
| CannotWriteToConfigFile = 4, | ||
| CannotUnsetOrSet = 5, | ||
| ConfigKeyInvalidOrNotSet = 1, | ||
| InvalidConfigFileGiven = 3, | ||
| InvalidRegexp = 6, | ||
| NoConfigKeyGiven = 2, | ||
| Success = 0, |
There was a problem hiding this comment.
Please order in ascending order by value.
Xml-docs for each.
IntegrationTests/UI.IntegrationTests/GitUICommands/RunCommandTests.cs
Outdated
Show resolved
Hide resolved
IntegrationTests/UI.IntegrationTests/GitUICommands/RunCommandTests.cs
Outdated
Show resolved
Hide resolved
RussKie
added
the
📭 needs: author feedback
ghost
removed
the
📭 needs: author feedback
vbjay
force-pushed
the
bug/GetEffectiveGitSetting_Error
branch
3 times, most recently
from
7221db4
to
91c192a
Compare
|
I would prefer to replace current method with |
vbjay
force-pushed
the
bug/GetEffectiveGitSetting_Error
branch
from
91c192a
to
1a541a2
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
vbjay
force-pushed
the
bug/GetEffectiveGitSetting_Error
branch
from
1a541a2
to
5bdb122
Compare
No on the if. I am making it so it is readable and understood by all.
Is a TON clearer. It reads and you can look at the enum for what it is doing @gerhardol you aren't seeing the direction I am going. That's ok. The whole point is to actually have a ful git config running feature where we can do the following:
Notice the metadata I have now. Run the test and look at the output. Read the comments. You need to stop with keeping it the same and look for what I am trying to do. I am trying to go for how to make it better and not just tweak. I have told you i am against creating a whole one way or another. I am following the principle of don't use exception's for program flow. How about you actually take the time to see the vision of where I am going and see that I am actually making it better. I challenge you to pull my pr and actually debug it and read the git source code I linked and actually understand my direction instead of whining on complexity or not following stuff. I AM STRONGLY against creating multiple directions of this api. I am trying to make it so it can eventually grow into a overload where we can have a GetEffectiveGitSettings where the same idea instead generates a hashmap, dictionary.... whatever of the keys fetched from a git config -l run and still knowing the scope and file and whatnot. Heck, we already have 2 that are named alike and make it too easy to mix up the one desired. I am trying to actually go in the direction of eliminating ways of getting config. I want @RussKie to chime in and provide feedback. You aren't seeing it and I think he does. Also, I am thinking in the part where i do the set part is where i will split the GitConfigGetResult into base and have a GitConfigGetResult and a GitConfigSetResult |
What other exit code than 1 do you plan to handle and why? This PR hides the error that the setting do not exists, it should be visible for the user.
Can you please take it a little easier with your comments and not go full frontal attacks on everything you do not agree with. It sure demotivates other than me. Note: Please do not squash and rebase so often for the PR branches. It is very hard to follow changes and see when there actually are changes. |
I am not interested in making a partial enum that I have to fix later. I have provided all exit codes and clearly documented them. The enum is named clearly. It is not just exit codes for get. It is all exit codes from git config.
It is. They have at their chosing ability to inspect status and react as needed. If they need to handle not set, they can code for it. I did not lose the fact it is unset. It is just not exceptional flow which it shouldn't be. It should just be known state. You shouldn't be looking at 1 as exceptional state. Git returns an exit code telling you the value isn't just a set blank. Inspecting status and handling that if desired is not hard. Being unset and requesting the value should not result in exceptional state. Git provides a default value when requested and doesn't blow up in shell. We shouldn't either.
Sure. But your reviews have felt a lot like drive by reviews without understanding so a tad bothered.
I can try to keep fixups until final review but I also rebase to keep up to date with master to catch and conflicts and test on what would go if meged without having jay merged origin/master into pr branch over and over. |
Do that in private branches. |
GitCommands/Git/GitModule.cs
Outdated
| string value = result.StandardOutput?.Trim() ?? "" + Delimiters.Null; | ||
|
|
||
| // Scope, file, value split by null. | ||
| string[] values = value.Split(Delimiters.Null, StringSplitOptions.RemoveEmptyEntries); |
There was a problem hiding this comment.
I'll fix. Realized I shouldn't remove empty here. Just a by default thing I do because it makes sense most times.
There was a problem hiding this comment.
Can we use spans here to reduce allocations?
GitCommands/Git/GitModule.cs
Outdated
| string value = result.StandardOutput?.Trim() ?? "" + Delimiters.Null; | ||
|
|
||
| // Scope, file, value split by null. | ||
| string[] values = value.Split(Delimiters.Null, StringSplitOptions.RemoveEmptyEntries); |
There was a problem hiding this comment.
Can we use spans here to reduce allocations?
| foreach (var g in settings.GroupBy(s => s.Status)) | ||
| { | ||
| Console.WriteLine(g.Key?.ToString() ?? "NULL"); | ||
| foreach (var s in g) | ||
| { | ||
| Console.WriteLine($"\t{s}"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Why do we need this?
TBH, I very much dislike it when I run tests the console is getting full of random output. It's not really helpful and often it's extremely difficult to find out where that random output originated from.
There was a problem hiding this comment.
Console writes can be removed. It was more to show the result. Can wrap if debug.
There was a problem hiding this comment.
The main point is that I didn't wrap in a try catch and that the test will cause a failure if an exception occurs which should not happen. The select gathering the values and the shoulds are the actual test. I was just using the console outputs to show the items in the test for now.
There was a problem hiding this comment.
I also use Devexpress Code Rush Test Runner
So outputs for each test are contained and I can do this
| /// </summary> | ||
| public enum GitConfigStatus | ||
| { | ||
| CannotWriteToConfigFile = 4, |
There was a problem hiding this comment.
I think both points are valid. The risk of git changing these return codes is probably quite low, likely they'd break a lot of people worldwide. At the same time, I find @gerhardol point about maintainability slightly more compelling, if we don't have an immediate use for some piece of code, I prefer not to add it. IMO it's totally valid to note all the possible return codes git may return for the config reading operation (thank you @vbjay for spending time finding those), but only expose the values we need. In the future, if a need to handle other return code arise, we can uncomment the necessary lines.
|
One more point, if I get git config all wrapped under our program using actual git config for retrieving values no matter what with the caching and such still working, we can then eliminate the system file not being read issue also. Our git config reading will be consistent with the values as if you called git config --get some.key. See the issues tied to editor and such for an example of what I mean. I don't like that we have a custom read files. Heck we probably have the same issue with worktree git config. Haven't looked and don't want to digress into another issue but I am trying to get to a point of being as consistent as possible with git config results. |
Use ReadonlySpan<Char>
Take reformat of comment Co-authored-by: Igor Velikorossov <RussKie@users.noreply.github.com>
wrap #if debug for output to be ignored on release build
Extract method to support use in other methods down the road
|
Note: This PR is for some future handling (not described in the description, possibly in some comments that I fail to find).
If email (or name?) is not set, it is not possible to commit in Git. Other GE uses should use the internal parsing for performance reasons too, unless there are very specific reasons. |
That'd be great to change in the future.
The values can change though while the app is running, so implmentations must be conscience of that.
Apologies, I'm struggling to parse this. Are you saying you have "no reasons to merge this" or you have "no objections to merging this". |
Exactly why I hit this one first. The next work will be looking at the other ways we read git config. Goal is to get it so all git config reading is actually done by using git config so stuff like our skipping system reading and worktree level settings will be fixed. Either git config --get or --get-all or git config -l to get all values. Still managing the different scopes with --show-scope and --show-origin and such. So still being able to show this is distributed with repo, this local, this global and so on. I dislike the internal processing that lacks reading all levels.
Kinda where I'm disagreeing. Our internal reading stinks. It doesn't support --system level config along with --workreee along with multiple value configs. Think of it this way.
Group by scope and you have your config levels and can even include file paths to go further in grouping. The issues like #1018 and #10579 shows that even if global is not set we should be reading system level. With the -l we can cache the whole config set in one shot. The fact we diverged from actual git config output is part of the problem. |
|
Ran git config --global -unset user.name and email .
So I don't see the problem here. If just installed we get them to setup. The window shows that the user info is blank and user can click link in status bar to fix. |
|
I do not want to merge this PR. In the short run, I find the error message worse. Not occurring often and nothing major but still worse. From implementation point of view (if we want to have the changes above), I find this to add complexity that is not needed. Only exit code 1 is checked, a lot of extra handling added. From a longer perspective, replacing GE config parsing with Git config parsing in general will add a lot of delays to the application, adding hundreds of ms to opening of forms and delays in other situations (no analyze). |
|
With mine you can see the fact that the committer is blank and it doesn't just silently bomb. I am sick and tired of you bull with the timing. A. I did mention we could still cache and run a git config -l and capture all of config. I think you are just wanting to not change into something that is different. I think you sit in your chair and look at github pull requests and don't take the time to investigate and actually try it out and compare before and after. You just sit there on github.com and don't pull it down and compare behavior. At least I know @mstv and @RussKie actually try stuff out. With you you just quickly and blindly without understanding do reviews and it irritates the snot out of me. @mstv raises points that are issues that are advised to change. |
|
Do the following before you review. build latest master and run the following Run your build and act like you are going to commit. note the behaviour and status bar. Even better debug it while doing so. Pull my pull request and do the same. Note any behaviour changes. |
|
https://git-scm.com/docs/git-worktree#_configuration_file work tree There are things that cna be done like have a memory cacheof settings and have it expire after x time and for only specific settings we always fetch current like user name and email. Others can be fetched from the cache. When the cache expires, make a git config fetcher reload it. All kinds of solutions for timing and when to run git config but still hav equick load times but still fight stale config issue. But the first part is to get it so that stuff is using the official git config parsing all the way through. |
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
Fixes #11145
Proposed changes
Test methodology
Test environment(s)
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.