From f4a82236b7591e161efb1fd4e5e9f1e33e1e02d3 Mon Sep 17 00:00:00 2001 From: Alyssa Ross Date: Thu, 11 Oct 2018 13:18:52 +0100 Subject: [PATCH 1/3] bump-formula-pr: add --no-fork GitHub seems to be discouraging forking private repositories[0]: > By default, new organizations are configured to disallow the forking > of private repositories. bump-formula-pr tries to create its pull requests from a fork, so it can't be used for private taps set up in this way. I've added a --no-fork option that will create PRs in the tap repo itself, rather than in a fork, to accommodate this use case. [0]: https://help.github.com/articles/allowing-people-to-fork-private-repositories-in-your-organization/ --- Library/Homebrew/dev-cmd/bump-formula-pr.rb | 38 +++++++++++++-------- 1 file changed, 24 insertions(+), 14 deletions(-) diff --git a/Library/Homebrew/dev-cmd/bump-formula-pr.rb b/Library/Homebrew/dev-cmd/bump-formula-pr.rb index 2cd9ed86f2..2d834d234b 100644 --- a/Library/Homebrew/dev-cmd/bump-formula-pr.rb +++ b/Library/Homebrew/dev-cmd/bump-formula-pr.rb @@ -36,6 +36,9 @@ #: If `--quiet` is passed, don't output replacement messages or warn about #: duplicate pull requests. #: +#: If `--no-fork` is passed, create a pull request from a branch in the tap +#: repository rather than a fork. +#: #: Note that this command cannot be used to transition a formula from a #: URL-and-sha256 style specification into a tag-and-revision style #: specification, nor vice versa. It must use whichever style specification @@ -79,6 +82,8 @@ module Homebrew description: "Run `brew audit --strict` before opening the PR." switch "--no-browse", description: "Output the pull request URL instead of opening in a browser" + switch "--no-fork", + description: "Create pull request directly from tap repo, not a fork" flag "--url=", description: "Provide new for the formula. If a is specified, the "\ "checksum of the new download must also be specified." @@ -345,7 +350,7 @@ module Homebrew shallow = !git_dir.empty? && File.exist?("#{git_dir}/shallow") if args.dry_run? - ohai "fork repository with GitHub API" + ohai "fork repository with GitHub API" unless args.no_fork? ohai "git fetch --unshallow origin" if shallow ohai "git checkout --no-track -b #{branch} origin/master" ohai "git commit --no-edit --verbose --message='#{formula.name} " \ @@ -355,21 +360,26 @@ module Homebrew ohai "git checkout -" else - begin - response = GitHub.create_fork(formula.tap.full_name) - # GitHub API responds immediately but fork takes a few seconds to be ready. - sleep 3 - rescue *GitHub.api_errors => e - formula.path.atomic_write(backup_file) unless args.dry_run? - odie "Unable to fork: #{e.message}!" - end - - if system("git", "config", "--local", "--get-regexp", "remote\..*\.url", "git@github.com:.*") - remote_url = response.fetch("ssh_url") + if args.no_fork? + remote_url = Utils.popen_read("git remote get-url --push origin").chomp + username = remote_url.match(%r{\A(?:\w+://.*/|[^/]*:)(.*)/})[1] else - remote_url = response.fetch("clone_url") + begin + response = GitHub.create_fork(formula.tap.full_name) + # GitHub API responds immediately but fork takes a few seconds to be ready. + sleep 3 + rescue *GitHub.api_errors => e + formula.path.atomic_write(backup_file) unless args.dry_run? + odie "Unable to fork: #{e.message}!" + end + + if system("git", "config", "--local", "--get-regexp", "remote\..*\.url", "git@github.com:.*") + remote_url = response.fetch("ssh_url") + else + remote_url = response.fetch("clone_url") + end + username = response.fetch("owner").fetch("login") end - username = response.fetch("owner").fetch("login") safe_system "git", "fetch", "--unshallow", "origin" if shallow safe_system "git", "checkout", "--no-track", "-b", branch, "origin/master" From 23984273f3a5f54260436e1ae43407d12a0e1860 Mon Sep 17 00:00:00 2001 From: Alyssa Ross Date: Thu, 11 Oct 2018 16:05:41 +0100 Subject: [PATCH 2/3] bump-formula-pr: always use base if unforkable --- Library/Homebrew/dev-cmd/bump-formula-pr.rb | 35 ++++++++++----------- 1 file changed, 17 insertions(+), 18 deletions(-) diff --git a/Library/Homebrew/dev-cmd/bump-formula-pr.rb b/Library/Homebrew/dev-cmd/bump-formula-pr.rb index 2d834d234b..69f1450f2b 100644 --- a/Library/Homebrew/dev-cmd/bump-formula-pr.rb +++ b/Library/Homebrew/dev-cmd/bump-formula-pr.rb @@ -36,9 +36,6 @@ #: If `--quiet` is passed, don't output replacement messages or warn about #: duplicate pull requests. #: -#: If `--no-fork` is passed, create a pull request from a branch in the tap -#: repository rather than a fork. -#: #: Note that this command cannot be used to transition a formula from a #: URL-and-sha256 style specification into a tag-and-revision style #: specification, nor vice versa. It must use whichever style specification @@ -82,8 +79,6 @@ module Homebrew description: "Run `brew audit --strict` before opening the PR." switch "--no-browse", description: "Output the pull request URL instead of opening in a browser" - switch "--no-fork", - description: "Create pull request directly from tap repo, not a fork" flag "--url=", description: "Provide new for the formula. If a is specified, the "\ "checksum of the new download must also be specified." @@ -350,7 +345,7 @@ module Homebrew shallow = !git_dir.empty? && File.exist?("#{git_dir}/shallow") if args.dry_run? - ohai "fork repository with GitHub API" unless args.no_fork? + ohai "try to fork repository with GitHub API" ohai "git fetch --unshallow origin" if shallow ohai "git checkout --no-track -b #{branch} origin/master" ohai "git commit --no-edit --verbose --message='#{formula.name} " \ @@ -360,18 +355,10 @@ module Homebrew ohai "git checkout -" else - if args.no_fork? - remote_url = Utils.popen_read("git remote get-url --push origin").chomp - username = remote_url.match(%r{\A(?:\w+://.*/|[^/]*:)(.*)/})[1] - else - begin - response = GitHub.create_fork(formula.tap.full_name) - # GitHub API responds immediately but fork takes a few seconds to be ready. - sleep 3 - rescue *GitHub.api_errors => e - formula.path.atomic_write(backup_file) unless args.dry_run? - odie "Unable to fork: #{e.message}!" - end + begin + response = GitHub.create_fork(formula.tap.full_name) + # GitHub API responds immediately but fork takes a few seconds to be ready. + sleep 3 if system("git", "config", "--local", "--get-regexp", "remote\..*\.url", "git@github.com:.*") remote_url = response.fetch("ssh_url") @@ -379,6 +366,18 @@ module Homebrew remote_url = response.fetch("clone_url") end username = response.fetch("owner").fetch("login") + rescue *GitHub.api_errors + # If the repository is private, forking might be disabled. + # Create branches in the repository itself instead. + remote_url = Utils.popen_read("git remote get-url --push origin").chomp + username = formula.tap.user + repo_name = "homebrew-#{formula.tap.repo}" + unless GitHub.repository(username, repo_name).fetch("private") + raise + end + rescue *GitHub.api_errors => e + formula.path.atomic_write(backup_file) unless args.dry_run? + odie "Unable to fork: #{e.message}!" end safe_system "git", "fetch", "--unshallow", "origin" if shallow From 864475e14fa4f40d676206beda46967e0c598b10 Mon Sep 17 00:00:00 2001 From: Alyssa Ross Date: Thu, 11 Oct 2018 16:27:14 +0100 Subject: [PATCH 3/3] bump-formula-pr: use GitHub error message info This saves an API call, and is more accurate, because the repo API doesn't actually say whether forking is enabled, but this error message does. To do this, the original GitHub error message had to be accessible on the GitHub exceptions. --- Library/Homebrew/dev-cmd/bump-formula-pr.rb | 7 +--- Library/Homebrew/utils/github.rb | 44 +++++++++++++-------- 2 files changed, 29 insertions(+), 22 deletions(-) diff --git a/Library/Homebrew/dev-cmd/bump-formula-pr.rb b/Library/Homebrew/dev-cmd/bump-formula-pr.rb index 69f1450f2b..16f3d24af6 100644 --- a/Library/Homebrew/dev-cmd/bump-formula-pr.rb +++ b/Library/Homebrew/dev-cmd/bump-formula-pr.rb @@ -366,15 +366,12 @@ module Homebrew remote_url = response.fetch("clone_url") end username = response.fetch("owner").fetch("login") - rescue *GitHub.api_errors + rescue GitHub::AuthenticationFailedError => e + raise unless e.github_message =~ /forking is disabled/ # If the repository is private, forking might be disabled. # Create branches in the repository itself instead. remote_url = Utils.popen_read("git remote get-url --push origin").chomp username = formula.tap.user - repo_name = "homebrew-#{formula.tap.repo}" - unless GitHub.repository(username, repo_name).fetch("private") - raise - end rescue *GitHub.api_errors => e formula.path.atomic_write(backup_file) unless args.dry_run? odie "Unable to fork: #{e.message}!" diff --git a/Library/Homebrew/utils/github.rb b/Library/Homebrew/utils/github.rb index e6fc4ef43a..cbfe76a218 100644 --- a/Library/Homebrew/utils/github.rb +++ b/Library/Homebrew/utils/github.rb @@ -15,13 +15,22 @@ module GitHub PR_ENV_KEY = "HOMEBREW_NEW_FORMULA_PULL_REQUEST_URL".freeze PR_ENV = ENV[PR_ENV_KEY] - Error = Class.new(RuntimeError) - HTTPNotFoundError = Class.new(Error) + class Error < RuntimeError + attr_reader :github_message + end + + class HTTPNotFoundError < Error + def initialize(github_message) + @github_message = github_message + super + end + end class RateLimitExceededError < Error - def initialize(reset, error) + def initialize(reset, github_message) + @github_message = github_message super <<~EOS - GitHub API Error: #{error} + GitHub API Error: #{github_message} Try again in #{pretty_ratelimit_reset(reset)}, or create a personal access token: #{ALL_SCOPES_URL} and then set the token as: export HOMEBREW_GITHUB_API_TOKEN="your_new_token" @@ -34,8 +43,9 @@ module GitHub end class AuthenticationFailedError < Error - def initialize(error) - message = "GitHub #{error}\n" + def initialize(github_message) + @github_message = github_message + message = "GitHub #{github_message}\n" if ENV["HOMEBREW_GITHUB_API_TOKEN"] message << <<~EOS HOMEBREW_GITHUB_API_TOKEN may be invalid or expired; check: @@ -193,6 +203,13 @@ module GitHub end def raise_api_error(output, errors, http_code, headers, scopes) + json = begin + JSON.parse(output) + rescue + nil + end + message = json&.[]("message") || "curl failed! #{errors}" + meta = {} headers.lines.each do |l| key, _, value = l.delete(":").partition(" ") @@ -204,25 +221,18 @@ module GitHub if meta.fetch("x-ratelimit-remaining", 1).to_i <= 0 reset = meta.fetch("x-ratelimit-reset").to_i - error = JSON.parse(output)["message"] - raise RateLimitExceededError.new(reset, error) + raise RateLimitExceededError.new(reset, message) end GitHub.api_credentials_error_message(meta, scopes) case http_code when "401", "403" - raise AuthenticationFailedError, output + raise AuthenticationFailedError, message when "404" - raise HTTPNotFoundError, output + raise HTTPNotFoundError, message else - error = begin - JSON.parse(output)["message"] - rescue - nil - end - error ||= "curl failed! #{errors}" - raise Error, error + raise Error, message end end