-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Fix various problems with install path and app path variables #968
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
Changes from all commits
4c0cfb6
4851182
7ba0258
f2d6ceb
b848716
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,7 +22,10 @@ internal WindowsEnvironment(IFileSystem fileSystem, IReadOnlyDictionary<string, | |
|
||
protected override string[] SplitPathVariable(string value) | ||
{ | ||
return value.Split(';'); | ||
// Ensure we don't return empty values here - callers may use this as the base | ||
// path for `Path.Combine(..)`, for which an empty value means 'current directory'. | ||
// We only ever want to use the current directory for path resolution explicitly. | ||
return value.Split(new[] { ';' }, StringSplitOptions.RemoveEmptyEntries); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is so much easier than what I would have done. 👍 |
||
} | ||
|
||
public override void AddDirectoryToPath(string directoryPath, EnvironmentVariableTarget target) | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -289,7 +289,11 @@ private static string GetWindowsEntryPath() | |||||
IntPtr argv0Ptr = Marshal.ReadIntPtr(argvPtr); | ||||||
string argv0 = Marshal.PtrToStringAuto(argv0Ptr); | ||||||
Interop.Windows.Native.Kernel32.LocalFree(argvPtr); | ||||||
return argv0; | ||||||
|
||||||
// If this isn't absolute then we should return null to prevent any | ||||||
// caller that expect only an absolute path from mis-using this result. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
// They will have to fall-back to other mechanisms for getting the entry path. | ||||||
return Path.IsPathRooted(argv0) ? argv0 : null; | ||||||
} | ||||||
|
||||||
#endregion | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -43,16 +43,24 @@ public static void Main(string[] args) | |
// | ||
// On UNIX systems we do the same check, except instead of a copy we use a symlink. | ||
// | ||
string oldName = PlatformUtils.IsWindows() | ||
? "git-credential-manager-core.exe" | ||
: "git-credential-manager-core"; | ||
|
||
if (appPath?.EndsWith(oldName, StringComparison.OrdinalIgnoreCase) ?? false) | ||
if (!string.IsNullOrWhiteSpace(appPath)) | ||
{ | ||
context.Streams.Error.WriteLine( | ||
"warning: git-credential-manager-core was renamed to git-credential-manager"); | ||
context.Streams.Error.WriteLine( | ||
$"warning: see {Constants.HelpUrls.GcmExecRename} for more information"); | ||
// Trim any (.exe) file extension if we're on Windows | ||
// Note that in some circumstances (like being called by Git when config is set | ||
// to just `helper = manager-core`) we don't always have ".exe" at the end. | ||
if (PlatformUtils.IsWindows() && appPath.EndsWith(".exe", StringComparison.OrdinalIgnoreCase)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point about ignoring the case here. |
||
{ | ||
appPath = appPath.Substring(0, appPath.Length - 4); | ||
} | ||
|
||
if (appPath.EndsWith("git-credential-manager-core", StringComparison.OrdinalIgnoreCase)) | ||
dscho marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
context.Streams.Error.WriteLine( | ||
"warning: git-credential-manager-core was renamed to git-credential-manager"); | ||
context.Streams.Error.WriteLine( | ||
$"warning: see {Constants.HelpUrls.GcmExecRename} for more information"); | ||
} | ||
} | ||
|
||
// Register all supported host providers at the normal priority. | ||
|
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 fantastic that you tested all those scenarios and wrote about it in the commit message. Otherwise I would have had concerns that this break things left and right. But that testing builds confidence in the correctness of this change!