Skip to content

bump: avoid overwriting skip message with autobump - #23737

Merged
krehel merged 2 commits into
mainfrom
bump-avoid-overwriting-skip-message
Sep 2, 2026
Merged

bump: avoid overwriting skip message with autobump#23737
krehel merged 2 commits into
mainfrom
bump-avoid-overwriting-skip-message

Conversation

@samford

@samford samford commented Sep 1, 2026

Copy link
Copy Markdown
Member

  • Have you followed our Contributing guidelines?
  • Have you checked for other open Pull Requests for the same change?
  • Have you explained what your changes do? Performance claims (e.g. "this is faster") must include brew benchmark results.
  • Have you explained why you'd like these changes included, not just what they do?
  • For bug fixes, have you given step-by-step brew commands to reproduce the bug?
  • Have you written new tests (excluding integration tests)? Here's an example.
  • Have you successfully run brew lgtm (style, typechecking and tests) locally?

  • I did not use AI/LLM to create this PR, or I disclosed the tool/model below and reviewed its output; I did not attribute commits to AI and will answer maintainer questions and review comments myself without AI/LLM.

I wrote tests for skip_ineligible_package! to cover the existing logic and then used Claude Code (Opus 5, high) to write some additional tests to finish what I started (bringing the method to 100% coverage in the end). I reviewed and tested the changes (confirming it's what I would have written myself) and tweaked some small things. I could have done it manually but I wanted to see how the AI-produced version would compare.


brew bump will skip formulae if they're disabled or HEAD-only and skip casks if they're disabled or use version :latest. However, if a package is set to be autobumped, the autobump skip message will overwrite any previously-set skip message. bump (and thusly autobump) skips disabled packages, so giving an error message about autobump is less relevant and can be confusing to users. The aforementioned skip conditions should take precedence over autobump status, so this modifies the autobump skip logic to ensure it won't overwrite an existing skip.

Output on main with a disabled cask that was previously autobumped:

$ brew bump olympus
==> olympus
Cask is autobumped so will have bump PRs opened by BrewTestBot every ~3 hours.

Output using this PR:

$ brew bump olympus
==> olympus
Cask is disabled so not accepting updates.

In the process, I renamed the skip_ineligible_formulae! method to skip_ineligible_package!, which makes it more clear that it's not a formula-only method. I also expanded the test coverage to 100% for the method, as the existing test only covered one of the possible skip scenarios (a version :latest cask).

The `skip_ineligible_formulae!` method in `brew bump` accepts both
formulae or casks, so the name may be misleading on the surface. This
renames the method to `skip_ineligible_package!`, to improve clarity.
`brew bump` will skip formulae if they're disabled or HEAD-only and
skip casks if they're disabled or use `version :latest`. However, if a
package is set to be autobumped, the autobump skip message will
overwrite any previously-set skip message. bump (and thusly autobump)
skips disabled packages, so giving an error message about autobump is
less relevant and can be confusing to users. The aforementioned skip
conditions should take precedence over autobump status, so this
modifies the autobump skip logic to ensure it won't overwrite an
existing skip.
Copilot AI balanced review requested due to automatic review settings September 1, 2026 23:22
@samford samford changed the title Bump avoid overwriting skip message bump: avoid overwriting skip message with autobump Sep 1, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

馃煝 Approval recommended

The focused logic change is correct, all references were updated, and relevant behavior is covered by tests.

Pull request overview

Prevents brew bump from replacing higher-priority ineligibility messages with autobump messages.

Changes:

  • Renames the package eligibility helper for formula and cask clarity.
  • Preserves disabled, HEAD-only, and version :latest skip reasons.
  • Expands unit coverage across eligibility scenarios.
File summaries
File Description
Library/Homebrew/dev-cmd/bump.rb Preserves existing skip reasons and renames the helper.
Library/Homebrew/test/dev-cmd/bump_spec.rb Adds comprehensive eligibility and message tests.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 0
  • Review effort level: Balanced

馃挕 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

@samford
samford force-pushed the bump-avoid-overwriting-skip-message branch from f5947ed to 35a552b Compare September 2, 2026 00:01
@krehel

krehel commented Sep 2, 2026

Copy link
Copy Markdown
Member

Thank you @samford!

@krehel
krehel added this pull request to the merge queue Sep 2, 2026
Merged via the queue into main with commit d28e785 Sep 2, 2026
58 checks passed
@krehel
krehel deleted the bump-avoid-overwriting-skip-message branch September 2, 2026 01:20

@MikeMcQuaid MikeMcQuaid left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks!

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.

5 participants