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.

Sign up for GitHub

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

Where possible, use existing github config to determine default branch #2062

Merged
merged 2 commits into from
Oct 2, 2024
Merged

Where possible, use existing github config to determine default branch #2062

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1d439f4
Where possible use existing github config when determining default br…
hadley Sep 20, 2024
113ade0
Reduce a few more uses
hadley Sep 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 18 additions & 42 deletions R/git-default-branch.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,39 +93,18 @@
#' git_default_branch()
#' }
git_default_branch <- function() {
repo <- git_repo()
git_default_branch_(github_remote_config())
}

# 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.
Comment on lines -98 to -102
Copy link
Member Author

Choose a reason for hiding this comment

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

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."
))
}
}
Comment on lines -104 to -121
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@hadley hadley Sep 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

# If config is available, we can use it to avoid an additional lookup
# on the GitHub API
git_default_branch_ <- function(cfg) {
repo <- git_repo()

upstream <- git_default_branch_remote("upstream")
upstream <- git_default_branch_remote(cfg, "upstream")
if (is.na(upstream$default_branch)) {
critique_remote(upstream)
origin <- git_default_branch_remote("origin")
origin <- git_default_branch_remote(cfg, "origin")
if (is.na(origin$default_branch)) {
critique_remote(origin)
db_source <- list()
} else {
db_source <- origin
Expand Down Expand Up @@ -186,7 +165,7 @@

# returns a whole data structure, because the caller needs the surrounding
# context to produce a helpful error message
git_default_branch_remote <- function(remote = "origin") {
git_default_branch_remote <- function(cfg, remote = "origin") {
repo <- git_repo()
out <- list(
name = remote,
Expand All @@ -196,25 +175,22 @@
default_branch = NA_character_
)

url <- git_remotes()[[remote]]
if (length(url) == 0) {
cfg_remote <- cfg[[remote]]
if (!cfg_remote$is_configured) {
out$is_configured <- FALSE
return(out)
}

out$is_configured <- TRUE
out$url <- url

# TODO: generalize here for GHE hosts that don't include 'github'
parsed <- parse_github_remotes(url)
# 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)
out$repo_spec <- remote_dat$repo_spec
out$default_branch <- remote_dat$default_branch
out$url <- cfg_remote$url

Check warning on line 185 in R/git-default-branch.R

View check run for this annotation

Codecov / codecov/patch

R/git-default-branch.R#L185 

Added line #L185 was not covered by tests

if (!is.na(cfg_remote$default_branch)) {
out$repo_spec <- cfg_remote$repo_spec
out$default_branch <- cfg_remote$default_branch

Check warning on line 189 in R/git-default-branch.R

View check run for this annotation

Codecov / codecov/patch

R/git-default-branch.R#L187-L189 

Added lines #L187 - L189 were not covered by tests
return(out)
}

# Fall back to pure git based approach
out$default_branch <- tryCatch(
{
gert::git_fetch(remote = remote, repo = repo, verbose = FALSE)
Expand Down
2 changes: 1 addition & 1 deletion R/github.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@
visibility <- match.arg(visibility)
check_protocol(protocol)
check_uses_git()
default_branch <- git_default_branch()
default_branch <- guess_local_default_branch()

Check warning on line 64 in R/github.R

View check run for this annotation

Codecov / codecov/patch

R/github.R#L64 

Added line #L64 was not covered by tests
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

check_current_branch(
is = default_branch,
# glue-ing happens inside check_current_branch(), where `gb` gives the
Expand Down
33 changes: 16 additions & 17 deletions R/pr.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@
}
}

default_branch <- git_default_branch()
default_branch <- git_default_branch_(cfg)

Check warning on line 187 in R/pr.R

View check run for this annotation

Codecov / codecov/patch

R/pr.R#L187 

Added line #L187 was not covered by tests
challenge_non_default_branch(
"Are you sure you want to create a PR branch based on a non-default branch?",
default_branch = default_branch
Expand Down Expand Up @@ -237,7 +237,7 @@
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()

Check warning on line 240 in R/pr.R

View check run for this annotation

Codecov / codecov/patch

R/pr.R#L240 

Added line #L240 was not covered by tests
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

branch <- choose_branch(exclude = default_branch)
if (is.null(branch)) {
ui_bullets(c("x" = "Repo doesn't seem to have any non-default branches."))
Expand Down Expand Up @@ -375,7 +375,7 @@
repo <- git_repo()
cfg <- github_remote_config(github_get = TRUE)
check_for_config(cfg, ok_configs = c("ours", "fork"))
default_branch <- git_default_branch()
default_branch <- git_default_branch_(cfg)

Check warning on line 378 in R/pr.R

View check run for this annotation

Codecov / codecov/patch

R/pr.R#L378 

Added line #L378 was not covered by tests
check_pr_branch(default_branch)
challenge_uncommitted_changes()

Expand Down Expand Up @@ -423,7 +423,7 @@
pr_pull <- function() {
cfg <- github_remote_config(github_get = TRUE)
check_for_config(cfg)
default_branch <- git_default_branch()
default_branch <- git_default_branch_(cfg)

Check warning on line 426 in R/pr.R

View check run for this annotation

Codecov / codecov/patch

R/pr.R#L426 

Added line #L426 was not covered by tests
check_pr_branch(default_branch)
challenge_uncommitted_changes()

Expand All @@ -449,11 +449,12 @@
#' @export
#' @rdname pull-requests
pr_view <- function(number = NULL, target = c("source", "primary")) {
tr <- target_repo(github_get = NA, role = target, ask = FALSE)
cfg <- github_remote_config(github_get = NA)
tr <- target_repo(cfg, github_get = NA, role = target, ask = FALSE)

Check warning on line 453 in R/pr.R

View check run for this annotation

Codecov / codecov/patch

R/pr.R#L452-L453 

Added lines #L452 - L453 were not covered by tests
url <- NULL
if (is.null(number)) {
branch <- git_branch()
default_branch <- git_default_branch()
default_branch <- git_default_branch_(cfg)

Check warning on line 457 in R/pr.R

View check run for this annotation

Codecov / codecov/patch

R/pr.R#L457 

Added line #L457 was not covered by tests
if (branch != default_branch) {
url <- pr_url(branch = branch, tr = tr)
if (is.null(url)) {
Expand Down Expand Up @@ -491,11 +492,11 @@
#' @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)
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

tr <- target_repo(cfg, github_get = NA, ask = FALSE)

Check warning on line 496 in R/pr.R

View check run for this annotation

Codecov / codecov/patch

R/pr.R#L495-L496 

Added lines #L495 - L496 were not covered by tests

ui_bullets(c("v" = "Switching back to the default branch."))
default_branch <- git_default_branch()
default_branch <- git_default_branch_(cfg)

Check warning on line 499 in R/pr.R

View check run for this annotation

Codecov / codecov/patch

R/pr.R#L499 

Added line #L499 was not covered by tests
if (git_branch() == default_branch) {
ui_bullets(c(
"!" = "Already on this repo's default branch ({.val {default_branch}}),
Expand Down Expand Up @@ -535,8 +536,10 @@
withr::defer(rstudio_git_tickle())
mode <- match.arg(mode)
repo <- git_repo()
tr <- target_repo(github_get = NA, role = target, ask = FALSE)
default_branch <- git_default_branch()

cfg <- github_remote_config(github_get = NA)
tr <- target_repo(cfg, github_get = NA, role = target, ask = FALSE)
default_branch <- git_default_branch_(cfg)

Check warning on line 542 in R/pr.R

View check run for this annotation

Codecov / codecov/patch

R/pr.R#L540-L542 

Added lines #L540 - L542 were not covered by tests

if (is.null(number)) {
check_pr_branch(default_branch)
Expand Down Expand Up @@ -629,14 +632,10 @@
# we're in DEFAULT branch of a fork. I wish everyone set up DEFAULT to track the
# DEFAULT branch in the source repo, but this protects us against sub-optimal
# setup.
pr_pull_source_override <- function(tr = NULL, default_branch = NULL) {
# naive selection of target repo; calling function should analyse the config
tr <- tr %||% target_repo(github_get = FALSE, ask = FALSE)
Comment on lines -633 to -634
Copy link
Member

Choose a reason for hiding this comment

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

I definitely remember that the only reason I had NULL default here and did this was for development convenience.

Copy link
Member Author

Choose a reason for hiding this comment

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

My thinking was that it's worth a little development inconvenience in order to ensure that we get efficient code.


pr_pull_source_override <- function(tr, default_branch) {
Copy link
Member Author

Choose a reason for hiding this comment

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

All callers of pr_pull_source_override() supply their arguments and I think we should continue to enforce that.

# 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()
Copy link
Member

Choose a reason for hiding this comment

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

Ditto for this

if (current_branch != default_branch) {
ui_abort("
Internal error: {.fun pr_pull_source_override} should only be used when on
Expand Down Expand Up @@ -994,7 +993,7 @@
invisible(TRUE)
}

check_pr_branch <- function(default_branch = git_default_branch()) {
check_pr_branch <- function(default_branch) {
# the glue-ing happens inside check_current_branch(), where `gb` gives the
# current git branch
check_current_branch(
Expand Down
2 changes: 1 addition & 1 deletion tests/testthat/helper-mocks.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ local_ui_yep <- function(.env = caller_env()) {

local_git_default_branch_remote <- function(.env = caller_env()) {
local_mocked_bindings(
git_default_branch_remote = function(remote) {
git_default_branch_remote = function(cfg, remote) {
list(
name = remote,
is_configured = TRUE,
Expand Down
Loading