Skip to content

docs: renaming formula requires two commits#20316

Closed
abitrolly wants to merge 2 commits intoHomebrew:mainfrom
abitrolly:patch-1
Closed

docs: renaming formula requires two commits#20316
abitrolly wants to merge 2 commits intoHomebrew:mainfrom
abitrolly:patch-1

Conversation

@abitrolly
Copy link
Contributor

@abitrolly abitrolly commented Jul 26, 2025

Using git mv to rename formula will break CI. Formula needs to be copied and old one removed in two separate commits.

See Homebrew/homebrew-core#231234 (comment) for details.

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

@abitrolly abitrolly changed the title docs: when renaming formula, old file needs to be copied docs: renaming formula requires two commits Jul 26, 2025
@Bo98
Copy link
Member

Bo98 commented Jul 26, 2025

That modified bottle block warning is very new so seems more likely a bug with that? Dropping the bottle block for renames is expected because we don't support renames on bottle level (so old bottles don't work).

So renames can work in a single commit, as long as git is able to detect it as a rename. If you've made significant changes to the formula that makes git not think that, you probably want to have a dedicated (new formula) commit anyway instead of trying to hide the changes in a rename.

@abitrolly
Copy link
Contributor Author

abitrolly commented Jul 26, 2025

The bottles are regenerated in a separate PR, right?

Perhaps the PR bot could detect renames from the PR title and don't block the PR in case the bottle section was removed from rename.

@carlocab
Copy link
Member

Technically it doesn't really block the PR, despite the red status.

But I agree that we should probably just fix the brew check-bottle-modification command so that it can handle renamed formulae in a single commit. A quick-and-dirty fix would make its failure not make CI red (but still post the comment, and acknowledge the possibility of false positives).

@carlocab
Copy link
Member

Oh, actually, maybe it's not the bottle block check?

See Homebrew/homebrew-core#230831.

The bottle block check doesn't complain about the rename being done in a single commit, but the commit style check does complain about the fact that a single commit modifies two formulae.

@Bo98
Copy link
Member

Bo98 commented Jul 26, 2025

the commit style check does complain about the fact that a single commit modifies two formulae.

That's an example of subtantial enough changes that git isn't detecting that as a rename. Renames work fine: Homebrew/homebrew-core#196439.

The increased size of our bottle block does confuse git more nowadays though. Though Homebrew/homebrew-core#231234 still is detected as a rename despite all that. You can see commit style passes fine: Homebrew/homebrew-core@7419276

The bottles are regenerated in a separate PR, right?

No, they're pushed to the same PR during merge

carlocab added a commit to Homebrew/homebrew-core that referenced this pull request Jul 27, 2025
This should help avoid some of the issues that motivated
Homebrew/brew#20316.
@carlocab
Copy link
Member

Ah, yes, I see. We can fix this for check-bottle-modification with Homebrew/homebrew-core#231336.

carlocab added a commit to Homebrew/homebrew-core that referenced this pull request Jul 27, 2025
Let's try a smaller test that doesn't confuse `git` here given the
discussion at Homebrew/brew#20316.
@abitrolly
Copy link
Contributor Author

Now I need to check if the fix works to remove double commit advice. "remove bottles" instruction is still needed.

@MikeMcQuaid
Copy link
Member

Passing on this, the advice is not desirable. We should instead fix it so this is not required.

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.

4 participants