Where possible, use existing github config to determine default branch #2062
Conversation
| # TODO: often when we call git_default_branch(), we already have a GitHub | ||
| # configuration or target repo, as produced by github_remote_config() or | ||
| # 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. |
There was a problem hiding this comment.
Deleted because that's what I did 😄
| # 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." | ||
| )) | ||
| } | ||
| } |
There was a problem hiding this comment.
Deleted because I think it's now more clear that it's the responsibility of github_remote_config() to report such problems, and github_remote_config() is now more consistently called before git_default_branch() where it matters.
There was a problem hiding this comment.
I also see this message in cases where it's not usage. For example, if you're offline, pr_resume() will emit this message even though you can still checkout the local branches that it lists.
| @@ -61,7 +61,7 @@ use_github <- function(organisation = NULL, | |||
| visibility <- match.arg(visibility) | |||
| check_protocol(protocol) | |||
| check_uses_git() | |||
| default_branch <- git_default_branch() | |||
| default_branch <- guess_local_default_branch() | |||
There was a problem hiding this comment.
I think this a reasonable substitution because I can't see why origin or upstream would be setup before you're calling use_github(). Please let me know if I've forgotten some likely scenario.
| @@ -237,7 +237,7 @@ pr_resume <- function(branch = NULL) { | |||
| ui_bullets(c( | |||
| "i" = "No branch specified ... looking up local branches and associated PRs." | |||
| )) | |||
| default_branch <- git_default_branch() | |||
| default_branch <- guess_local_default_branch() | |||
There was a problem hiding this comment.
I think it's ok to use a cheap check here because we're only using it to exclude branches, and this (along with the tweak below) allows pr_resume() to work offline.
| @@ -491,11 +492,11 @@ pr_view <- function(number = NULL, target = c("source", "primary")) { | |||
| #' @export | |||
| #' @rdname pull-requests | |||
| pr_pause <- function() { | |||
| # intentionally naive selection of target repo | |||
| tr <- target_repo(github_get = FALSE, ask = FALSE) | |||
| cfg <- github_remote_config(github_get = NA) | |||
There was a problem hiding this comment.
This seems like a change, but isn't really because the call to git_default_branch() on line 498 would have used github_get = NA.
| # naive selection of target repo; calling function should analyse the config | ||
| tr <- tr %||% target_repo(github_get = FALSE, ask = FALSE) | ||
|
|
||
| pr_pull_source_override <- function(tr, default_branch) { |
There was a problem hiding this comment.
All callers of pr_pull_source_override() supply their arguments and I think we should continue to enforce that.
hadley
changed the title
| # naive selection of target repo; calling function should analyse the config | ||
| tr <- tr %||% target_repo(github_get = FALSE, ask = FALSE) |
There was a problem hiding this comment.
I definitely remember that the only reason I had NULL default here and did this was for development convenience.
There was a problem hiding this comment.
My thinking was that it's worth a little development inconvenience in order to ensure that we get efficient code.
| # TODO: why does this not use a check_*() function, i.e. shared helper? | ||
| # I guess to issue a specific error message? | ||
| current_branch <- git_branch() | ||
| default_branch <- default_branch %||% git_default_branch() |
Fixes #2021