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

Implement HOMEBREW_FORCE_BREWED_GIT #4377

Merged
merged 3 commits into from Jun 26, 2018

Conversation

Projects
None yet
4 participants
@jacktose
Copy link
Contributor

jacktose commented Jun 26, 2018

Because of this messing with the user's path, brew uses /usr/bin/git over brewed git, even when the former is problematically old.
There may also be other reasons a user prefers to use brewed git.

There was already a HOMEBREW_FORCE_BREWED_CURL option and a HOMEBREW_SYSTEM_CURL_TOO_OLD check to set it. This mostly copies those to implement HOMEBREW_FORCE_BREWED_GIT & HOMEBREW_SYSTEM_GIT_TOO_OLD.

See also: Linuxbrew/brew#736

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew tests with your changes locally?

Implement HOMEBREW_FORCE_BREWED_GIT
Because of this messing with the user's path:
https://github.com/Homebrew/brew/blob/
    efc0289/bin/brew#L68
brew uses /usr/bin/git over brewed git, even when the former is
problematically old.
There may also be other reasons a user prefers to use brewed git.

There was already a HOMEBREW_FORCE_BREWED_CURL option and a
HOMEBREW_SYSTEM_CURL_TOO_OLD check to set it. This mostly copies those
to implement HOMEBREW_FORCE_BREWED_GIT & HOMEBREW_SYSTEM_GIT_TOO_OLD.

See also: Linuxbrew/brew#736
@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jun 26, 2018

I’m 👍🏻 on this but you’ll need to run ‘brew man’ and commit the result.

Jack Haden-Enneking
@jacktose

This comment has been minimized.

Copy link
Contributor

jacktose commented Jun 26, 2018

Manpages fixed, now failing codecov. Can someone give me a hand with that? I don't really know how coverage works, and I didn't touch the file that it says has changed.

@@ -384,7 +384,8 @@ EOS
fi

if ! git --version &>/dev/null ||
[[ -n "$HOMEBREW_SYSTEM_GIT_TOO_OLD" &&
[[ ( -n "$HOMEBREW_SYSTEM_GIT_TOO_OLD" ||

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jun 26, 2018

Member

Given you always set force brewed git above when git is too old you can just make this check for forced brewed git

This comment has been minimized.

@jacktose

jacktose Jun 26, 2018

Contributor

Oh, above = brew.sh? We can count on that always running first?
I suppose that's what the CURL options are doing, so I'll match that.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jun 26, 2018

Don’t worry about codecov. Sorry I missed the previous comment. Will merge once that’s changed. Sorry for brevity; on my phone.

Simplify check before git install
No need to check for both variables, because they're always set together.
Also, this harmonizes with the CURL equivalent just above.

@MikeMcQuaid MikeMcQuaid merged commit 0d33aba into Homebrew:master Jun 26, 2018

3 checks passed

codecov/patch Coverage not affected when comparing a75a8c6...e63feb7
Details
codecov/project 55.55% (+<.01%) compared to a75a8c6
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jun 26, 2018

Great work, thanks a lot! Hope to see more contributions from you.

@jacktose

This comment has been minimized.

Copy link
Contributor

jacktose commented Jun 26, 2018

Thanks, me too. Trying to get my feet wet in OSS contribution.
Thanks to @sjackman too for the help & encouragement.

@jacktose jacktose deleted the jacktose:force-brewed-git branch Jun 26, 2018

@sjackman

This comment has been minimized.

Copy link
Contributor

sjackman commented Jun 26, 2018

Excellent! Thanks for this work, Jack! 🏅It'll be a big help for Linuxbrew. And thanks for merging, Mike.

@@ -102,6 +102,7 @@ then
if [[ "$HOMEBREW_MACOS_VERSION_NUMERIC" -lt "100900" ]]
then
HOMEBREW_SYSTEM_GIT_TOO_OLD="1"

This comment has been minimized.

@reitermarkus

reitermarkus Jun 26, 2018

Member

Is this still used now?

This comment has been minimized.

@jacktose

jacktose Jun 26, 2018

Contributor

I hope so. There's no other check (that I've seen) for too-old-ness. Without that, it's just a manual option.
Maybe there's a better or additional check, though? Parsing git --version?

This comment has been minimized.

@sjackman

sjackman Jun 26, 2018

Contributor

Yes, parsing git --version would be preferable, at least on Linux.
@reitermarkus Which do you prefer on macOS? Checking the version of Git, or checking the version of macOS?

This comment has been minimized.

@reitermarkus

reitermarkus Jun 26, 2018

Member

I actually just meant if the variable HOMEBREW_SYSTEM_GIT_TOO_OLD is still used, not the check itself.

This comment has been minimized.

@jacktose

jacktose Jun 26, 2018

Contributor

It seems that HOMEBREW_SYSTEM_CURL_TOO_OLD is redundant, because HOMEBREW_FORCE_SYSTEM_CURL is set at the same time. The GIT variables are now in the same situation.
I guess the question is, is there a case for using an auto-determined TOO_OLD separately from a user-settable FORCE?

This comment has been minimized.

@sjackman

sjackman Jun 26, 2018

Contributor

HOMEBREW_SYSTEM_GIT_TOO_OLD does not appear to be used.

HOMEBREW_SYSTEM_CURL_TOO_OLD is used here:

def curl_handles_most_https_certificates?
# The system Curl is too old for some modern HTTPS certificates on
# older macOS versions.
ENV["HOMEBREW_SYSTEM_CURL_TOO_OLD"].nil?
end

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jun 29, 2018

Member

Fixed in #4391.

MikeMcQuaid added a commit to MikeMcQuaid/brew that referenced this pull request Jun 29, 2018

brew.sh: remove unused variable.
As of Homebrew#4377 this is no longer used.

@MikeMcQuaid MikeMcQuaid referenced this pull request Jun 29, 2018

Merged

brew.sh: remove unused variable. #4391

5 of 6 tasks complete

@lock lock bot added the outdated label Jul 29, 2018

@lock lock bot locked as resolved and limited conversation to collaborators Jul 29, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.