Use built-in git path interpolation for config settings
#439
Conversation
There was a problem hiding this comment.
Left a few nit comments, but also think it should be possible to reduce all the changes at the existing call-sites if we put the extra type arg at the end of the argument list, and add some more extension methods.
Also a question about Git version compatibility with the --type=<type> argument.
vdye
force-pushed
the
git-config-prefix-expansion
branch
from
3c195f7 to
fabdfb0
Compare
src/shared/Microsoft.Git.CredentialManager.Tests/GitConfigurationTests.cs
Outdated
Show resolved
Hide resolved
vdye
force-pushed
the
git-config-prefix-expansion
branch
2 times, most recently
from
efaadc2 to
a50c135
Compare
This change adds a `GitConfigurationType` enum to `GitConfiguration` and includes it as an argument to `TryGet`, `GetAll`, and `GetRegex`. This argument (if not `None`) includes the appropriate `--type` flag in the `git config`. In the `TryGet` extension function, an `isPath` boolean maps to usage of `GitConfigurationType.Path` for `true` and `GitConfigurationType.None` for `false`. Existing usage of the updated functions has not changed (that is, all usages specify `GitConfigurationType.None`). Signed-off-by: Victoria Dye <[email protected]>
The `GetSettingValues` function is updated to include an `isPath` flag which, when `true`, will pass the flag to `GitConfiguration.TryGet(...)` when applicable. In order to have this work when a `RemoteUri` is specified, `GitConfiguration.TryGet(...)` is called directly if `isPath` is `true` after enumerating all settings (since `Enumerate` does not allow specifying a canonical type). Additionally, `TryGetPathSetting` is added as a convenience function for getting the first found path setting from `GetSettingValues(...)`. This commit does not change the value of any existing settings - it only introduces the ability to read settings as canonical paths (if sourced from the git config). Signed-off-by: Victoria Dye <[email protected]>
Path interpolation enabled (using `Settings.TryGetPathSetting`) for the credential helper path and custom cert bundle (`httpCAInfo`) path. Signed-off-by: Victoria Dye <[email protected]>
Switch to deprecated settings for config type (for better compatibility).
The `Version` property (calculated only once, on first access) has type `GitVersion`, made up of the pre-custom components of a Git version (i.e., before a `.windows` or `.vfs`). This version is currently only used in `GitConfiguration` in order to determine whether to use the now-deprecated canonical type constraint flags (for backwards-compatibility purposes) when accessing a config value. Signed-off-by: Victoria Dye <[email protected]>
vdye
force-pushed
the
git-config-prefix-expansion
branch
from
a50c135 to
67046ca
Compare
| } | ||
| else | ||
| { | ||
| // Exit at the first non-integer component |
There was a problem hiding this comment.
Makes sense, this would turn 2.33.0.vfs.2.3 into [2, 33, 0].
TBH version comparing is (in my experience) somewhat of an ill-defined problem, and the more simplistic we can make it without breaking our use cases, the better.
As a point of reference, I would like to link to the version comparison in Git for Windows' installer and the code in git update-git-for-windows. As you can see, they both do different things, and neither of them are complete.
I also had a brief hunch that there might be something in .NET, and there is: System.Version. Alas, it seems that the only way to parse a string into an instance of that class is very limited and would choke on any letters (such as vfs).
All this is to say: I think this implementation is nice and small, and good enough for our purposes. Thank you for implementing it!
|
|
||
| namespace Microsoft.Git.CredentialManager | ||
| { | ||
| public class GitVersion : IComparable, IComparable<GitVersion> |
There was a problem hiding this comment.
Implementing IComparable on types can be hard to get right; looks like you've done a great job - nice! 😁
Did you consider also implementing the IEquatable and IEquatable<GitVersion> interfaces?
The implementation is already there in your overridden bool Equals(object) method but adding the interface will mean should we ever put GitVersion in a dictionary or set, that collection can know it can be equated.
This change does not block this PR by the way..
|
Note I merged this without the Ubuntu 16.04 check passing because GitHub Actions is going to remove that particular image shortly anyway (https://github.blog/changelog/2021-04-29-github-actions-ubuntu-16-04-lts-virtual-environment-will-be-removed-on-september-20-2021/)! |
Changes
GitConfigurationgetter functionsTryGetPathSettingtoSettings, including handling URL-scoped config settingssslCAInfo& credential helper settings to use path type from git configNotes
GitConfiguration, an enum (GitConfigurationType) is used to specify the type of the config setting, but theTryGetextension & functions inSettingsinstead use anisPathboolean.GitConfigurationLevel.