This might be edge case-y, but there might not be a local branch equivalent of origin/<default-branch>. In that case, this command would fail. We could maybe fall back to any other branch, as long as we can switch away from the current one so we can delete it.

This seems pretty edge case-y, but maybe this is something we could fix in a future PR. I'm also not sure how i'd fix it and wondering if failing is better than switching to a random branch because that would be pretty unexpected.

This makes me thing that should expand our cmd stubber to be able to assert that a specific command was invoked

I think that would still run into similar problems. When things don't match up correctly you still want to print out the commands that are called. I think there is a better solution to what we're doing with command stubbing, but I've not been able to think of it yet!

I've added this function so we can get an *api.Repository object from a ghrepo.Interface.

Is it worth doing a type check in this to see if a given interface-satisfying value is already an api.Repository? (genuine question, I'm not sure)

You can technically get the default branch from git by doing

git symbolic-ref refs/remotes/origin/HEAD

however, in my experience this value will usually be out of date after the remote repository changes its default branch. So I think it's best to continue to get this information from the server. 👍

Technically, we only need to switch to a different branch if we're doing to delete a branch that happens to be the current branch. If the pr.HeadRefName branch to delete is not the current one, I would say that we should ideally avoid the git checkout step.

That makes sense. I added the behavior you recommended and a test.

For scripting use, I would prefer that this flag would be false by default.

The deletion of the local branch is potentially destructive right now and, in my mind, prone to edge-cases:

• assumes that a local branch corresponding to a PR always exists;
• assumes that the local branch name is always the same as PR head;
• assumes that the branch to be deleted does not have unpushed commits.

If I write a script that programatically merges multiple PRs:

for pr in 12 13 14; do gh pr merge --merge $pr; done

I would get 3 git errors about not being able to delete the local branch if I've never actually checked out those PRs locally before. I could avoid it by doing:

gh pr merge --merge $pr --delete-branch=false

but I feel that it's best if this flag was opt-in instead of opt-out by default.

My only concern here is that the default behavior for deletion would be different between interactive and non-interactive. But if other people don't think this is a problem then I'm cool with the change.

We should try to pass an API client from the outside to avoid instantiating multiple API clients during the lifetime of the same command #943

I think this method of resolving is too complex for this use case and that resolving a "ghrepo" to a *Repository object could be done simpler. Additionally, it doesn't look like the repo argument to the parent function is respected in the lookup.

We have a function that can resolve a ghrepo to a Repository: api.GitHubRepo(). Currently, the payload returned does not include the information about the default branch, but you could expand the field list requested in that query to include defaultBranchRef { name }.

One more optimization that we could easily do: since the repo ghrepo.Interface object might already be an *api.Repository (and often it will be), we could detect and return that and avoid making an API request:

if r, ok := repo.(*api.Repository); ok && r.DefaultBranchRef.Name != "" {
  return r, nil
}
return api.GitHubRepo(repo)

I got rid of the convertRepoInterfaceToRepository method and replaced it with api.GitHubRepo. I'm against the type checking idea though. It feels like extra code without much benefit. But I'm interested in exploring how to ensure we always get an api.Repository (or why we sometimes only need a ghrepo interface)

I noticed that this test already has http.StubRepoResponse("OWNER", "REPO") above. Is this extra stub necessary?

Unfortunately yes, because we have to convert the ghrepo interface into an api.Repository interface which does another lookup.

Is it necessary to request this field if we're never going to consume it when listing pull requests? This is a potentially expensive field as GitHub starts a background job to check mergeability.

"UNKNOWN" usually means that mergeability is currently still being calculated. Instead of presenting this as an error, we could be optimistic and assume that the PR is OK to merge and allow the API merge operation to proceed—it has a chance of succeeding, and if it fails the API error message should let us know why.

I think we should check this state before any checks around the .Mergeable field. If the PR is already merged, its .Mergeable field might report "UNKOWN" or "CONFLICTING", in which case the current logic will trigger one of the error messages above, which would be misleading to the user.

The repo value looks like it's only used inside a conditional, so we can initialize it inside that same conditional as well (skipping an API request if the condition is false).