From cf64fccb62818eb52b5041f873e4488fea62dfa1 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Mon, 9 Sep 2024 07:28:15 -0500 Subject: [PATCH] Some notes for discussion --- R/git-default-branch.R | 24 +-------- R/utils-github.R | 113 ++++++++++++++++++++++++++++------------- R/utils.R | 9 ++++ 3 files changed, 87 insertions(+), 59 deletions(-) diff --git a/R/git-default-branch.R b/R/git-default-branch.R index fdcaac79d..38cbd38f6 100644 --- a/R/git-default-branch.R +++ b/R/git-default-branch.R @@ -100,32 +100,10 @@ git_default_branch <- function() { # target_repo(). In that case, we don't need to start from scratch as we do # here. But I'm not sure it's worth adding complexity to allow passing this # data in. - - # TODO: this critique feels somewhat mis-placed, i.e. it brings up a general - # concern about a repo's config (or the user's permissions and creds) - # related to whether github_remotes() should be as silent as it is about - # 404s - critique_remote <- function(remote) { - if (remote$is_configured && is.na(remote$default_branch)) { - ui_bullets(c( - "x" = "The {.val {remote$name}} remote is configured, but we can't - determine its default branch.", - " " = "Possible reasons:", - "*" = "The remote repo no longer exists, suggesting the local remote - should be deleted.", - "*" = "We are offline or that specific Git server is down.", - "*" = "You don't have the necessary permission or something is wrong - with your credentials." - )) - } - } - upstream <- git_default_branch_remote("upstream") if (is.na(upstream$default_branch)) { - critique_remote(upstream) origin <- git_default_branch_remote("origin") if (is.na(origin$default_branch)) { - critique_remote(origin) db_source <- list() } else { db_source <- origin @@ -209,7 +187,7 @@ git_default_branch_remote <- function(remote = "origin") { # if the protocol is ssh, I suppose we can't assume a PAT, i.e. it's better # to use the Git approach vs. the GitHub API approach if (grepl("github", parsed$host) && parsed$protocol == "https") { - remote_dat <- github_remotes(remote, github_get = NA) + remote_dat <- github_remotes(remote, github_get = TRUE) out$repo_spec <- remote_dat$repo_spec out$default_branch <- remote_dat$default_branch return(out) diff --git a/R/utils-github.R b/R/utils-github.R index abba6d408..2560eeb84 100644 --- a/R/utils-github.R +++ b/R/utils-github.R @@ -171,41 +171,33 @@ github_remote_list <- function(these = c("origin", "upstream"), x = NULL) { #' @noRd github_remotes <- function(these = c("origin", "upstream"), github_get = NA, - x = NULL) { + x = NULL, + error_call = caller_env()) { + check_bool(github_get, allow_na = TRUE) + grl <- github_remote_list(these = these, x = x) - get_gh_repo <- function(repo_owner, repo_name, - api_url = "https://api.github.com") { - if (isFALSE(github_get)) { - f <- function(...) list() + + get_gh_repo <- function(repo_owner, repo_name, api_url) { + if (isTRUE(github_get)) { + github_remote_api(repo_owner, repo_name, api_url, error_call = error_call) + } else if (is.na(github_get)) { + # Use if possible + tryCatch( + github_remote_api(repo_owner, repo_name, api_url, error_call = error_call), + error = function(cnd) list() + ) } else { - f <- purrr::possibly(gh::gh, otherwise = list()) + # Only use local info + list() } - f( - "GET /repos/{owner}/{repo}", - owner = repo_owner, repo = repo_name, .api_url = api_url - ) } - repo_info <- purrr::pmap( - grl[c("repo_owner", "repo_name", "api_url")], - get_gh_repo - ) + + remote_cols <- grl[c("repo_owner", "repo_name", "api_url")] + repo_info <- unwrap_purrr_error(purrr::pmap(remote_cols, get_gh_repo)) + # NOTE: these can be two separate matters: # 1. Did we call the GitHub API? Means we know `is_fork` and the parent repo. # 2. If so, did we call it with auth? Means we know if we can push. - grl$github_got <- map_lgl(repo_info, ~ length(.x) > 0) - if (isTRUE(github_get) && !all(grl$github_got)) { - oops <- which(!grl$github_got) - oops_remotes <- grl$remote[oops] - oops_hosts <- unique(grl$host[oops]) - ui_abort(c( - "Unable to get GitHub info for these remotes: {.val {oops_remotes}}.", - "Are we offline? Is GitHub down? Has the repo been deleted?", - "Otherwise, you probably need to configure a personal access token (PAT) - for {.val {oops_hosts}}.", - "See {.run usethis::gh_token_help()} for advice." - )) - } - grl$default_branch <- map_chr(repo_info, "default_branch", .default = NA) grl$is_fork <- map_lgl(repo_info, "fork", .default = NA) # `permissions` is an example of data that is not present if the request @@ -213,25 +205,69 @@ github_remotes <- function(these = c("origin", "upstream"), grl$can_push <- map_lgl(repo_info, c("permissions", "push"), .default = NA) grl$can_admin <- map_lgl(repo_info, c("permissions", "admin"), .default = NA) grl$perm_known <- !is.na(grl$can_push) + grl$parent_repo_owner <- map_chr(repo_info, c("parent", "owner", "login"), .default = NA) grl$parent_repo_name <- map_chr(repo_info, c("parent", "name"), .default = NA) grl$parent_repo_spec <- make_spec(grl$parent_repo_owner, grl$parent_repo_name) - parent_info <- purrr::pmap( - set_names( - grl[c("parent_repo_owner", "parent_repo_name", "api_url")], - ~ sub("parent_", "", .x) - ), - get_gh_repo + parent_cols <- set_names( + grl[c("parent_repo_owner", "parent_repo_name", "api_url")], + ~ sub("parent_", "", .x) ) - grl$can_push_to_parent <- + parent_info <- unwrap_purrr_error(purrr::pmap(parent_cols[grl$is_fork, , drop = FALSE], get_gh_repo)) + grl$can_push_to_parent <- rep(FALSE, nrow(grl)) + grl$can_push_to_parent[grl$is_fork] <- map_lgl(parent_info, c("permissions", "push"), .default = NA) grl } +# Wrap GET /repos/{owner}/{repo} with user friendly errors +github_remote_api <- function(repo_owner, + repo_name, + api_url = "https://api.github.com", + error_call = caller_env()) { + + withCallingHandlers( + gh::gh( + "GET /repos/{owner}/{repo}", + owner = repo_owner, + repo = repo_name, + .api_url = api_url + ), + httr2_failure = function(cnd) { + ui_abort( + c( + "Can't reach {.url {api_url}}.", + "Are you offline? Is GitHub down?" + ), + parent = cnd, + call = error_call + ) + }, + httr2_http_404 = function(cnd) { + # Might be because it's a private repo & no PAT + ui_abort( + c( + "Can't find {repo_owner}/{repo_name}.", + if (!gh::gh_token_exists()) c( + "Do you need to configure a GitHub token?", + "See {.run usethis::gh_token_help()} for advice." + ) + ), + parent = cnd, + call = error_call + ) + }, + httr2_http_403 = function(cnd) { + # No token? + ui_abort(c("Can't find {repo_owner}/{repo_name}"), parent = cnd, call = error_call) + } + ) +} + #' Classify the GitHub remote configuration #' #' @description @@ -324,7 +360,9 @@ new_github_remote_config <- function() { github_remote_config <- function(github_get = NA) { cfg <- new_github_remote_config() - grl <- github_remotes(github_get = github_get) + + # TODO: need to figure out how to get error messages from get_github_Remote to here + grl <- github_remotes(github_get = TRUE) if (nrow(grl) == 0) { return(cfg_no_github(cfg)) @@ -764,6 +802,9 @@ cfg_theirs <- function(cfg) { } cfg_maybe_ours_or_theirs <- function(cfg) { + # TODO: figure out how to get information from github_remote_api to + # here because that actually has precise information about what's wrong + if (cfg$origin$is_configured) { configured <- "origin" not_configured <- "upstream" diff --git a/R/utils.R b/R/utils.R index fcba6e889..f5531ac83 100644 --- a/R/utils.R +++ b/R/utils.R @@ -111,3 +111,12 @@ maybe_name <- function(x, ..., arg = caller_arg(x), check_name(x, ..., allow_null = TRUE, arg = arg, call = call) } + +unwrap_purrr_error <- function(code) { + withCallingHandlers( + code, + purrr_error_indexed = function(err) { + cnd_signal(err$parent) + } + ) +}