Skip to content

feat(pr): Add cleanup subcommand to delete merged local branches - #7322

Closed
elldritch wants to merge 5 commits into
cli:trunkfrom
elldritch:feat/pr-cleanup
Closed

elldritch wants to merge 5 commits into
cli:trunkfrom
elldritch:feat/pr-cleanup

Conversation

@elldritch

@elldritch elldritch commented Apr 17, 2023 •

Copy link
Copy Markdown

I know your contributing guidelines say to avoid issues labelled core, but I hacked this together over the weekend after encountering #380 and it performs pretty well for me, so I thought I'd open a PR in case you folks want to take it over. Notably, this PR is still missing tests.

Here's an example of the UI for this subcommand on one of my repositories (note that actual output in TTY is colorized):

$ gh pr cleanup --all
Loading PRs for 90 local branches with upstreams. This can take a minute if you have many branches...

The following branches can be cleaned up:

BRANCH                                       STATUS    PULL REQUEST
PLAT-132-analysis-failures                   ✓ MERGED  #83 fix: Failed analyses should be reported as failed
PLAT-43-api-endpoints                        ✓ MERGED  #4 PLAT-43: Implement initial API endpoints
PLAT-44-worker-queuing                       ✓ MERGED  #6 PLAT-44: Set up worker queuing
jg/plat-156                                  ! MERGED  #79 [PLAT-156] Track caller metadata (e.g. build ID, task ID) for analyzeSource
jg/plat-330                                  ! MERGED  #111 [PLAT-330] include worker id in worker logs
jg/plat-61                                   ! MERGED  #14 PLAT-61 - Add credentials for git CLI download
jg/plat-74/cli-flags                         ! MERGED  #41 [PLAT-74] Unknown CLI flags should not cause an error 
jg/update/rel8                               ✓ MERGED  #60 update rel8 to 1.4.0.0
leo/refactor/analyze-job-exception-cleanup   ✓ CLOSED  #99 refactor: Avoid extra Either
leo/refactor/bugexception                    ! MERGED  #65 refactor: Migrate `error` to `bug`
plat-508-with-utf8                           ✓ MERGED  #156 fix: Handle UTF-8 input and output in handles
plat-509-worker-timeout-increase             ✓ MERGED  #155 fix: Increase license scanning timeouts one last time
// [... additional branches elided for brevity ...]

! indicates that a local branch is behind its remote.

? Delete all 79 merged or closed branches? No
Not deleting any branches.

Here is the implemented interface:

$ gh pr cleanup --help
Clean up local branches of merged or closed pull requests

USAGE
  gh pr cleanup {<number> | <url> | <branch> | --all} [flags]

FLAGS
  --all              Clean up all merged pull requests
  --exclude-behind   Exclude branches that are behind their remote
  --exclude-closed   Exclude branches of closed pull requests
  --strict           Both --exclude-closed and --exclude-behind
  --yes              Skip deletion confirmation

INHERITED FLAGS
      --help                     Show help for command
  -R, --repo [HOST/]OWNER/REPO   Select another repository using the [HOST/]OWNER/REPO format

LEARN MORE
  Use 'gh <command> <subcommand> --help' for more information about a command.
  Read the manual at https://cli.github.com/manual

Fixes #380.

@jglick jglick left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I was thinking it might be possible to create a separate gh-cleanup executable for testing this before it was released, but given the changes to shared client.go & objects.go I guess that is not possible—the whole gh binary needs to be replaced. Can try building from sources and using this for a few days.

Comment on lines +143 to +146
// Any local branch whose HEAD is a commit of a merged or closed PR is a
// candidate for deletion, because the local branch's history is a prefix of
// the remote branch's history (i.e. there are no local commits that the
// upstream does not have).

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🤔 So for a squash- or rebase-merged PR, this logic verifies that the user’s local commits made it into the PR before it was merged, and we trust that the resulting commit to the base branch actually reflected those changes (perhaps with some amendments from the maintainer). Seems close enough. Compare discussion of git branch -d behavior in #380.

@jglick

jglick commented Apr 19, 2023 •

Copy link
Copy Markdown

Tried it after jenkinsci/github-checks-plugin#307 was merged, and did not work at all unfortunately: --all did not find the stale branch; and explicitly requesting the PR number attempted to delete the base branch rather than the branch of my fork:

$ git remote -v
fork	https://github.com/jglick/github-checks-plugin.git (fetch)
fork	https://github.com/jglick/github-checks-plugin.git (push)
origin	https://github.com/jenkinsci/github-checks-plugin.git (fetch)
origin	https://github.com/jenkinsci/github-checks-plugin.git (push)
$ git branch 
  bumps
* master
$ gh pr cleanup --all
Loading PRs for 2 local branches with upstreams. This can take a minute if you have many branches...
✓ No branches to be cleaned up!
$ gh pr cleanup 307

The following branches can be cleaned up:

BRANCH  STATUS    PULL REQUEST
master  ! MERGED  #307 Standardized & simplified & updated build & test infrastructure

! indicates that a local branch is behind its remote.

? Delete all 1 merged or closed branches? Yes
failed to run git: error: Cannot delete branch 'master' checked out at '…/github-checks-plugin'

@jglick

jglick commented Apr 19, 2023

Copy link
Copy Markdown

My repo config at this point:

[core]
	repositoryformatversion = 0
	filemode = true
	bare = false
	logallrefupdates = true
[remote "origin"]
	url = [email protected]:jenkinsci/github-checks-plugin.git
	fetch = +refs/heads/*:refs/remotes/origin/*
	gh-resolved = base
[branch "master"]
	remote = origin
	merge = refs/heads/master
[remote "fork"]
	url = https://github.com/jglick/github-checks-plugin.git
	fetch = +refs/heads/*:refs/remotes/fork/*
[gitg]
	mainline = refs/heads/master
[branch "bumps"]
	remote = fork
	merge = refs/heads/bumps

@jglick

jglick commented Apr 19, 2023

Copy link
Copy Markdown

In another case I tried, --all did not work (did not claim to find any branches to clean up), but specifying the PR number did work. This case also involved a PR filed from a fork; it was however against a private repo, the PR was true-merged rather than squash-merged, and I had not yet git pulled from the base branch at the time I ran gh pr cleanup.

@elldritch

Copy link
Copy Markdown
Author

@jglick:

I was thinking it might be possible to create a separate gh-cleanup executable

This is possible, and there are a couple plugins. See for example gh-poi. I decided to hack out this native command because it felt like it should be native to me, and I was hoping to reuse the existing logic around handling default repositories and configuration and stuff.

In another case I tried, --all did not work (did not claim to find any branches to clean up), but specifying the PR number did work. This case also involved a PR filed from a fork

I haven't tested the behavior of --all against forks. My test cases were all against repositories that I'm branching directly off of at work.

When using --all, the current implementation tries to find PRs using a Finder, and I suspect this might not be looking at forks:

pr, _, err := opts.Finder.Find(shared.FindOptions{
Selector: branch.Upstream.BranchName,
Fields: []string{"headRefOid", "headRefName", "title", "state"},
States: prStates,
})

Another possibility is that if your PR was not up-to-date, the current implementation does not look at PRs against upstream repositories:

pr, err := mergedPRAssociatedWithCommit(httpClient, repo, branch.Local.Hash)

explicitly requesting the PR number attempted to delete the base branch rather than the branch of my fork:

That's odd. I wonder if that's because for not-up-to-date PRs, the current implementation looks at the BaseRepo even when that's not origin:

repo, err := opts.BaseRepo()


I'll take a stab at properly supporting forks if I find the bandwidth, although this is low priority for me because it works for my personal use cases already. I haven't fully worked out what the semantics for forks should be yet (e.g. which PRs to search for, how to find them, and which branches to delete).

@jglick

jglick commented Apr 20, 2023

Copy link
Copy Markdown

Makes sense; forks are trickier to handle. If you find the time, wonderful, or I might.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CLI command to clean up after a merged PR

3 participants