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

elldritch · 2023-04-17T04:55:21Z

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

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.

jglick · 2023-04-19T15:50:09Z

pkg/cmd/pr/cleanup/cleanup.go

+ // 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).


🤔 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 · 2023-04-19T17:45:05Z

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 · 2023-04-19T17:47:33Z

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 · 2023-04-19T20:10:25Z

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 · 2023-04-20T07:10:21Z

@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:

cli/pkg/cmd/pr/cleanup/cleanup.go

Lines 119 to 123 in 8ce601e

 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:

cli/pkg/cmd/pr/cleanup/cleanup.go

Line 294 in 8ce601e

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:

cli/pkg/cmd/pr/cleanup/cleanup.go

Line 206 in 8ce601e

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 · 2023-04-20T11:33:58Z

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

elldritch added 5 commits April 16, 2023 02:17

Create initial scaffold

9d11fa2

feat: Implement cleanup --all

0f7251f

refactor: Improve performance and rate limit cost

be65688

feat: Implement cleanup {<number> | <url> | <branch>}

be63803

refactor: Remove unused Factory fields

8ce601e

elldritch mentioned this pull request Apr 17, 2023

CLI command to clean up after a merged PR #380

Open

jglick reviewed Apr 19, 2023

View reviewed changes

williammartin requested review from williammartin and removed request for williammartin September 11, 2023 16:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

elldritch commented Apr 17, 2023 •

edited

jglick left a comment

jglick Apr 19, 2023

jglick commented Apr 19, 2023 •

edited

jglick commented Apr 19, 2023

jglick commented Apr 19, 2023

elldritch commented Apr 20, 2023

jglick commented Apr 20, 2023

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

Are you sure you want to change the base?

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

Conversation

elldritch commented Apr 17, 2023 • edited

jglick left a comment

Choose a reason for hiding this comment

jglick Apr 19, 2023

Choose a reason for hiding this comment

jglick commented Apr 19, 2023 • edited

jglick commented Apr 19, 2023

jglick commented Apr 19, 2023

elldritch commented Apr 20, 2023

jglick commented Apr 20, 2023

elldritch commented Apr 17, 2023 •

edited

jglick commented Apr 19, 2023 •

edited