Skip to content
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

Ignore Homebrew Git shim #731

Merged
merged 3 commits into from
Jun 13, 2022

Conversation

ldennington
Copy link
Contributor

Users running brew uninstall or brew upgrade currently see the following message (indicating that the operation wasn't fully successful):

Unconfiguring component 'Azure Repos provider'...
fatal: Failed to unset Git configuration entry 'credential.https://dev.azure.com.useHttpPath' [1]
git: This shim is internal and must be run via brew.

This is because GCM is finding the Homebrew Git shim on the PATH and calling it instead of the user's Git installation. Specifying that we should ignore the Homebrew shim when attempting to locate the Git executable on the PATH resolves the issue.

Fixes #676.

@ldennington ldennington self-assigned this Jun 9, 2022
@ldennington ldennington marked this pull request as ready for review June 9, 2022 22:44
Copy link
Collaborator

@mjcheetham mjcheetham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments on using sudo -E instead and caching the ignore list.

return TryLocateExecutable(program, new List<string>(), out path);
}

public bool TryLocateExecutable(string program, List<string> pathsToIgnore, out string path)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make this method protected virtual and accept an ICollection<string> instead

Suggested change
public bool TryLocateExecutable(string program, List<string> pathsToIgnore, out string path)
protected virtual bool TryLocateExecutable(string program, ICollection<string> pathsToIgnore, out string path)

Copy link
Contributor Author

@ldennington ldennington Jun 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The UT started complaining with protected so I went with internal instead. Let me know if there's a better solution, though.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh right, yes, the UT calls this directly. You could also try protected internal; internal so the UT can call it and protected so only subclasses need to access it from non-test code.

Not important thought - more a nit 😁

src/shared/Core/EnvironmentBase.cs Outdated Show resolved Hide resolved
src/shared/Core/EnvironmentBase.cs Outdated Show resolved Hide resolved
src/shared/Core/Interop/MacOS/MacOSEnvironment.cs Outdated Show resolved Hide resolved
src/shared/Core/Interop/MacOS/MacOSEnvironment.cs Outdated Show resolved Hide resolved
src/shared/Core/Interop/Posix/PosixEnvironment.cs Outdated Show resolved Hide resolved
src/shared/Core.Tests/EnvironmentTests.cs Show resolved Hide resolved
src/osx/Installer.Mac/uninstall.sh Outdated Show resolved Hide resolved
Add ability to ignore specified paths when searching the PATH for an
executable.
Ignore Homebrew shim to ensure the user's installed version of Git is
called during `brew uninstall` (instead of the shim). Note that this
involves explicitly passing the HOMEBREW_PREFIX environment variable,
since the `sudo -u `/usr/bin/logname` "$GCMBIN" unconfigure` command
does not inherit it.
Add a unit test to verify ignoring paths when searching the PATH for
executables works as expected.
Copy link
Collaborator

@mjcheetham mjcheetham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

src/shared/Core.Tests/EnvironmentTests.cs Show resolved Hide resolved
return TryLocateExecutable(program, new List<string>(), out path);
}

public bool TryLocateExecutable(string program, List<string> pathsToIgnore, out string path)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh right, yes, the UT calls this directly. You could also try protected internal; internal so the UT can call it and protected so only subclasses need to access it from non-test code.

Not important thought - more a nit 😁

@ldennington ldennington merged commit d6834d6 into git-ecosystem:main Jun 13, 2022
@ldennington ldennington mentioned this pull request Jun 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants