Remote delete #982
Remote delete #982
Conversation
| func DoesLocalBranchExist(branch string) bool { | ||
| configCmd := GitCommand("rev-parse", "--verify", branch) | ||
| _, err := run.PrepareCmd(configCmd).Output() | ||
| return err == nil |
There was a problem hiding this comment.
I made the assumption here that any error means the branch doesn't exist. I figured that will be more stable in the future than doing a string match on the error message.
There was a problem hiding this comment.
It's a fair assumption! rev-parse --verify is sufficiently low-level
command/pr.go
Outdated
| branchSwitchString := "" | ||
| if branchToSwitchTo != "" { | ||
| branchSwitchString = fmt.Sprintf(" and switched to branch %s", utils.Cyan(branchToSwitchTo)) | ||
| } | ||
|
|
||
| fmt.Fprintf(colorableOut(cmd), "%s Deleted branch %s%s\n", utils.Red("✔"), utils.Cyan(pr.HeadRefName), branchSwitchString) |
There was a problem hiding this comment.
This is kind of messy, but I haven't figured out a good way to do simple string interpolated stuff in go.
There was a problem hiding this comment.
Until we switch to some kind of template-based output with embedded if/else control flow, I think this is perfectly fine! 👍
command/pr.go
Outdated
| branchSwitchString := "" | ||
| if branchToSwitchTo != "" { | ||
| branchSwitchString = fmt.Sprintf(" and switched to branch %s", utils.Cyan(branchToSwitchTo)) | ||
| } | ||
|
|
||
| fmt.Fprintf(colorableOut(cmd), "%s Deleted branch %s%s\n", utils.Red("✔"), utils.Cyan(pr.HeadRefName), branchSwitchString) |
There was a problem hiding this comment.
Until we switch to some kind of template-based output with embedded if/else control flow, I think this is perfectly fine! 👍
git/git.go
Outdated
| @@ -209,6 +209,18 @@ func DeleteLocalBranch(branch string) error { | |||
| return err | |||
| } | |||
|
|
|||
| func DeleteRemoteBranch(branch string) error { | |||
| configCmd := GitCommand("push", "origin", "--delete", branch) | |||
There was a problem hiding this comment.
-
This works when the
--repoflag was not used, but I think it might be wiser to delete branches via API operations if we were to support scripting via--repo OWNER/REPO. In the latter mode, the local git repo will likely not correspond to the repo chosen via--repoargument. -
If we're keeping this approach, then it might be prudent to delete the
refs/heads/<branch>ref instead of just<branch>to disambiguate when the branch name matches an existing tag (so we don't delete a tag by accident).
There was a problem hiding this comment.
I'm sorry for the extra work but I agree that deleting via the API is safer.
There was a problem hiding this comment.
Makes sense. It now deletes the remote branch using the API. If the repo flag is set it will not delete the local branch, only the remote branch.
| func DoesLocalBranchExist(branch string) bool { | ||
| configCmd := GitCommand("rev-parse", "--verify", branch) | ||
| _, err := run.PrepareCmd(configCmd).Output() | ||
| return err == nil |
There was a problem hiding this comment.
It's a fair assumption! rev-parse --verify is sufficiently low-level
git/git.go
Outdated
| @@ -209,6 +209,18 @@ func DeleteLocalBranch(branch string) error { | |||
| return err | |||
| } | |||
|
|
|||
| func DeleteRemoteBranch(branch string) error { | |||
| configCmd := GitCommand("push", "origin", "--delete", branch) | |||
There was a problem hiding this comment.
I'm sorry for the extra work but I agree that deleting via the API is safer.
|
All right! This now uses the github API to delete the ref. It also does some work to make sure only remote branches are deleted when the repo flag is set |
| @@ -209,6 +209,12 @@ func DeleteLocalBranch(branch string) error { | |||
| return err | |||
| } | |||
|
|
|||
| func HasLocalBranch(branch string) bool { | |||
| configCmd := GitCommand("rev-parse", "--verify", "refs/heads/"+branch) | |||
| _, err := run.PrepareCmd(configCmd).Output() | |||
There was a problem hiding this comment.
Tip: to run a command without being interested in its output, you can use Run() instead of Output().
command/pr.go
Outdated
| } | ||
| } | ||
|
|
||
| err = git.DeleteLocalBranch(pr.HeadRefName) | ||
| err = api.BranchDeleteRemote(apiClient, baseRepo, pr.HeadRefName) |
There was a problem hiding this comment.
-
If this PR comes from a fork, we probably don't want to attempt to delete a same-named branch in the base repo since that won't be the same branch. We most likely don't have the privileges to delete a branch on someone's fork, but we could optionally check for the off chance that we have write privileges on the PR head repo.
-
We could change the order of the logic: delete the remote branch before attempting to delete the local branch. That way, if something went wrong with deleting the local branch (e.g. the local working copy was dirty and
git checkoutfailed), at least we've already cleaned up the remote branch.
There was a problem hiding this comment.
this PR comes from a fork, we probably don't want to attempt to delete a same-named branch in the base repo since that won't be the same branch.
👍 I've added a cross-repo PR check to prevent branch deletion on PRs that come from forks.
We could change the order of the logic: delete the remote branch before attempting to delete the local branch.
I'm going to skip this for now because I want to reduce the number of changes before we release this.
This PR needs #899 to be merged first.
Remember when I didn't explain how
pr mergedidn't have the ability to delete the remote branch 😬. Well this PR is my atonement. I also added some more error checking so if the local branch doesn't exist it won't try and delete it.Here are examples of it in use.
When you are on the branch being deleted
Handle when the local branch doesn’t exist. I opted for no noticeable output here.
Remote branch doesn’t exist (it was already deleted). The error here is pretty ugly, but maybe this is ok since it is so rare?
I was worried about the situation where you wouldn't have access to delete the remote branch, but I couldn't get any example of that working.