From 380d1b0d0af81ca02d99f081c4c13188f57de373 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Thu, 15 Oct 2026 08:22:57 +0000 Subject: [PATCH] tools/bump: carry hand-pushed commits instead of pausing Open PR with commits by others: rebase onto base, build on top, push with lease. Rebase conflict pauses and comments once. --- tools/bump/git.go | 95 ++++++++++++++++++++++++++++++++------- tools/bump/git_test.go | 27 +++++++++++ tools/bump/main.go | 57 ++++++++++++++++++----- tools/bump/pr.go | 47 +++++++++++-------- tools/bump/report.go | 13 +++++- tools/bump/report_test.go | 2 +- 6 files changed, 190 insertions(+), 51 deletions(-) create mode 100644 tools/bump/git_test.go diff --git a/tools/bump/git.go b/tools/bump/git.go index 03f242a2..3d606a43 100644 --- a/tools/bump/git.go +++ b/tools/bump/git.go @@ -2,6 +2,7 @@ package main import ( "context" + "errors" "fmt" "strings" ) @@ -73,36 +74,96 @@ func commitAll(ctx context.Context, r *repo, message string) error { return err } -// startBranch rebuilds the working branch from the base every run. The branch -// therefore never accumulates history, never conflicts, and the bot keeps no -// state on disk between runs. -func startBranch(ctx context.Context, r *repo, remote, base, branch string) error { +var errRebaseConflict = errors.New("hand-pushed commits do not rebase cleanly") + +// startBranch rebuilds the working branch from the base every run, so it never +// accumulates history and the bot keeps no state on disk between runs. The +// exception is an open pull request holding commits someone pushed by hand: +// rebuilding would erase them, so the branch is rebased onto the base and the +// run builds on top. It returns the subjects of the commits it carried. +func startBranch(ctx context.Context, r *repo, remote, base, branch string, carry bool) ([]string, error) { if _, err := r.run(ctx, "git", "fetch", remote, base); err != nil { //nolint:noinlineerr - return err + return nil, err } - _, err := r.run(ctx, "git", "checkout", "-B", branch, remote+"/"+base) + var hand []string - return err + if carry { + var err error + + hand, err = handCommits(ctx, r, remote, base, branch) + if err != nil { + return nil, err + } + } + + start := remote + "/" + base + if len(hand) > 0 { + start = remote + "/" + branch + } + + if _, err := r.run(ctx, "git", "checkout", "-B", branch, start); err != nil { //nolint:noinlineerr + return nil, err + } + + if len(hand) == 0 { + return nil, nil + } + + if _, err := r.run(ctx, "git", "rebase", remote+"/"+base); err != nil { //nolint:noinlineerr + _, _ = r.run(ctx, "git", "rebase", "--abort") + + return nil, fmt.Errorf("%w onto %s: %w", errRebaseConflict, base, err) + } + + return hand, nil } -// remoteBranchSHA is the tip of branch on remote, or "" when it does not exist. -func remoteBranchSHA(ctx context.Context, r *repo, remote, branch string) string { - out, err := r.run(ctx, "git", "ls-remote", "--heads", remote, branch) +// handCommits lists, oldest first, the subjects of commits on the remote branch +// authored by anyone but the configured committer. +func handCommits(ctx context.Context, r *repo, remote, base, branch string) ([]string, error) { + if _, err := r.run(ctx, "git", "fetch", remote, branch); err != nil { //nolint:noinlineerr + return nil, err + } + + self, err := r.run(ctx, "git", "config", "user.email") if err != nil { - return "" + return nil, err } - fields := strings.Fields(out) - if len(fields) == 0 { - return "" + out, err := r.run(ctx, "git", "log", "--reverse", "--format=%ae%x00%s", + remote+"/"+base+".."+remote+"/"+branch) + if err != nil { + return nil, err } - return fields[0] + return notAuthoredBy(out, strings.TrimSpace(self)), nil } -func forcePush(ctx context.Context, r *repo, remote, branch string) error { - _, err := r.run(ctx, "git", "push", "--force", remote, "HEAD:refs/heads/"+branch) +// notAuthoredBy filters `git log --format=%ae%x00%s` output to the subjects of +// commits whose author is not email. +func notAuthoredBy(log, email string) []string { + var subjects []string + + for line := range strings.Lines(log) { + author, subject, _ := strings.Cut(strings.TrimRight(line, "\n"), "\x00") + if author != email { + subjects = append(subjects, subject) + } + } + + return subjects +} + +// forcePush replaces branch on remote. A non-empty lease is the tip the run +// started from; the push is refused if anyone moved the branch since. +func forcePush(ctx context.Context, r *repo, remote, branch, lease string) error { + force := "--force" + if lease != "" { + force = "--force-with-lease=refs/heads/" + branch + ":" + lease + } + + _, err := r.run(ctx, "git", "push", force, remote, "HEAD:refs/heads/"+branch) if err != nil { return fmt.Errorf("pushing %s: %w", branch, err) } diff --git a/tools/bump/git_test.go b/tools/bump/git_test.go new file mode 100644 index 00000000..5daaff3d --- /dev/null +++ b/tools/bump/git_test.go @@ -0,0 +1,27 @@ +package main + +import ( + "slices" + "testing" +) + +// The bot's own commits are rebuilt every run; only someone else's are carried. +func TestNotAuthoredBy(t *testing.T) { + const bot = "41898282+github-actions[bot]@users.noreply.github.com" + + log := bot + "\x00flake.lock: update nixpkgs\n" + + "dev@example.com\x00noise: fix deprecation\n" + + bot + "\x00go.mod: update dependencies\n" + + "dev@example.com\x00capver: regenerate\n" + + got := notAuthoredBy(log, bot) + + want := []string{"noise: fix deprecation", "capver: regenerate"} + if !slices.Equal(got, want) { + t.Errorf("notAuthoredBy = %q, want %q", got, want) + } + + if got := notAuthoredBy("", bot); len(got) != 0 { + t.Errorf("empty log carried %q", got) + } +} diff --git a/tools/bump/main.go b/tools/bump/main.go index 0aebe2a4..b8069d18 100644 --- a/tools/bump/main.go +++ b/tools/bump/main.go @@ -19,6 +19,7 @@ package main import ( "context" + "errors" "fmt" "log" "os" @@ -112,7 +113,37 @@ func cmdRun(ctx context.Context) error { return err } - if err := startBranch(ctx, r, runCfg.Remote, runCfg.Base, runCfg.Branch); err != nil { //nolint:noinlineerr + // Looked up before the branch is built: an open pull request holding + // hand-pushed commits decides what the branch starts from. + var ( + slug string + open *prState + ) + + if !noPR { + slug, err = currentSlug(ctx, r, runCfg.Repo) + if err != nil { + return err + } + + pr, err := lookupPR(ctx, r, slug, runCfg.Branch) + if err != nil { + return err + } + + if pr != nil && pr.State == "OPEN" && !pr.hasLabel(skipLabel) { + open = pr + } + } + + carried, err := startBranch(ctx, r, runCfg.Remote, runCfg.Base, runCfg.Branch, open != nil) + if errors.Is(err, errRebaseConflict) { + _ = commentOnce(ctx, r, slug, open.Number, + "The commits pushed by hand here no longer rebase onto "+runCfg.Base+ + ", so the automation has paused. Rebase the branch, or merge or close this pull request.") + } + + if err != nil { return err } @@ -136,7 +167,7 @@ func cmdRun(ctx context.Context) error { return err } - body := renderBody(results, markerOf(results, tree, head), gate) + body := renderBody(results, markerOf(results, tree, head), gate, carried) if err := writeStepSummary(body); err != nil { //nolint:noinlineerr return err @@ -154,19 +185,21 @@ func cmdRun(ctx context.Context) error { return nil } - slug, err := currentSlug(ctx, r, runCfg.Repo) - if err != nil { - return err + var lease string + if open != nil { + lease = open.HeadRefOid } return publish(ctx, r, publishOptions{ - Slug: slug, - Remote: runCfg.Remote, - Branch: runCfg.Branch, - Base: runCfg.Base, - Title: "all: bump pinned versions", - Gate: gate, - Force: runCfg.Force, + Slug: slug, + Remote: runCfg.Remote, + Branch: runCfg.Branch, + Base: runCfg.Base, + Title: "all: bump pinned versions", + Gate: gate, + Force: runCfg.Force, + Lease: lease, + Carried: carried, }, results) } diff --git a/tools/bump/pr.go b/tools/bump/pr.go index 7cb71864..fdd5fe91 100644 --- a/tools/bump/pr.go +++ b/tools/bump/pr.go @@ -3,7 +3,6 @@ package main import ( "context" "encoding/json" - "errors" "fmt" "log" "strconv" @@ -18,8 +17,6 @@ const skipLabel = "no-autoupdate" // same diff the next morning is how an automation earns itself an ignore rule. const closedCooldown = 7 * 24 * time.Hour -var errHumanCommits = errors.New("branch has commits the bot did not push") - type prState struct { Number int `json:"number"` State string `json:"state"` @@ -83,6 +80,10 @@ type publishOptions struct { Title string Gate string Force bool + // Lease is the branch tip the run started from, when it updates an open + // pull request; Carried lists hand-pushed commits it built on. + Lease string + Carried []string } // publish force-pushes the rebuilt branch and puts it in front of a human: @@ -99,7 +100,7 @@ func publish(ctx context.Context, r *repo, opt publishOptions, results []result) return err } - body := renderBody(results, markerOf(results, tree, head), opt.Gate) + body := renderBody(results, markerOf(results, tree, head), opt.Gate, opt.Carried) pr, err := lookupPR(ctx, r, opt.Slug, opt.Branch) if err != nil { @@ -128,28 +129,18 @@ func publish(ctx context.Context, r *repo, opt publishOptions, results []result) body = fmt.Sprintf("Previously closed unmerged: #%d\n\n%s", pr.Number, body) } - if err := forcePush(ctx, r, opt.Remote, opt.Branch); err != nil { //nolint:noinlineerr + if err := forcePush(ctx, r, opt.Remote, opt.Branch, ""); err != nil { //nolint:noinlineerr return err } return createPR(ctx, r, opt, body) } -// updateOpen refreshes an existing pull request in place, but refuses to -// force-push over work the bot did not author. +// updateOpen refreshes an existing pull request in place. Hand-pushed commits +// were carried when the branch was built; the lease keeps the push from +// overwriting any that landed during the run. func updateOpen(ctx context.Context, r *repo, opt publishOptions, pr *prState, body string) error { - prev, hadMarker := parseMarker(pr.Body) - - remoteTip := remoteBranchSHA(ctx, r, opt.Remote, opt.Branch) - if hadMarker && prev.Head != "" && remoteTip != "" && remoteTip != prev.Head { - _, _ = r.run(ctx, "gh", "pr", "comment", strconv.Itoa(pr.Number), "--repo", opt.Slug, - "--body", "This branch has commits the automation did not push, so it has paused. "+ - "Merge or close this pull request to resume.") - - return fmt.Errorf("%w: %s is at %s, expected %s", errHumanCommits, opt.Branch, remoteTip, prev.Head) - } - - err := forcePush(ctx, r, opt.Remote, opt.Branch) + err := forcePush(ctx, r, opt.Remote, opt.Branch, opt.Lease) if err != nil { return err } @@ -183,3 +174,21 @@ func createPR(ctx context.Context, r *repo, opt publishOptions, body string) err return nil } + +// commentOnce posts body unless the pull request already has it, so a +// condition that persists across daily runs is reported once. +func commentOnce(ctx context.Context, r *repo, slug string, number int, body string) error { + out, err := r.run(ctx, "gh", "pr", "view", strconv.Itoa(number), "--repo", slug, + "--json", "comments", "--jq", ".comments[].body") + if err != nil { + return err + } + + if strings.Contains(out, body) { + return nil + } + + _, err = r.run(ctx, "gh", "pr", "comment", strconv.Itoa(number), "--repo", slug, "--body", body) + + return err +} diff --git a/tools/bump/report.go b/tools/bump/report.go index e4bfc135..1fbaf44b 100644 --- a/tools/bump/report.go +++ b/tools/bump/report.go @@ -8,7 +8,8 @@ import ( // markerPrefix tags the machine-readable footer of a bot-authored pull request // body. It is the bot's only persistent state: everything else is rebuilt from -// the base branch on every run. +// the base branch on every run, or rebased onto it when carrying hand-pushed +// commits. const markerPrefix = "