Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

pr-pull: implement autosquash option #8724

Merged
merged 3 commits into from
Sep 17, 2020
Merged

pr-pull: implement autosquash option #8724

merged 3 commits into from
Sep 17, 2020
+231 −40

Conversation

jonchang
Copy link
Contributor

  • 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 tests with your changes locally?

Step 1 of #8093.

Maintainers should be able to run e.g. brew pr-pull --autosquash 1234 and automagically get a reasonable-looking commit message. Still have some testing to do with pull requests with multiple authors and other edge cases but I'd like to get this feature out for review before I neglect it even more.

Simple version bump:

% brew pr-pull --autosquash --no-upload https://github.com/Homebrew/homebrew-core/pull/61130
==> Fetching homebrew/core pull request #61130
==> Using 3 commits from #61130
HEAD is now at 6ab7a1fda2 benthos: update 3.28.0 bottle.
==> openfast 2.4.0
==> Downloading https://api.github.com/repos/Homebrew/homebrew-core/actions/artifacts/17574672/zip
% git log -1
commit 8a9aa14ed99c30937fa964bb5a309d986189b761 (HEAD -> master)
Author: Rafael M Mudafort <rafmudaf@gmail.com>
Date:   Mon Sep 14 18:04:27 2020 -0500

    openfast 2.4.0

    * Update to v2.4.0
    * Update test case input file
    * Remove 'revision' line - brew test feedback

    Closes #61130.

New formula:

% brew pr-pull --autosquash --no-upload https://github.com/Homebrew/homebrew-core/pull/61125
==> Fetching homebrew/core pull request #61125
==> Using 4 commits from #61125
HEAD is now at 6ab7a1fda2 benthos: update 3.28.0 bottle.
==> youtube-dlc 2020.09.13 (new formula)
Error: No artifact with the name `bottles` was found!
  https://github.com/Homebrew/homebrew-core/actions/runs/254441461
% git log -1
commit 68c7c77b92d993e240fa8791a6a86ac46a16769b (HEAD -> master)
Author: Christopher Nethercott <ccnethercott@gmail.com>
Date:   Mon Sep 14 20:25:43 2020 +0100

    youtube-dlc 2020.09.13 (new formula)

    * Added youtube-dlc
    * Removed unusued comments for youtube-dlc
    * Removed erroronous tabs
    * Removed redundent comment and used version as variable for the URL

    Closes #61125.

Rewording a commit:

% brew pr-pull --autosquash --no-upload --message "for FLAC support" https://github.com/Homebrew/homebrew-core/pull/60740
==> Fetching homebrew/core pull request #60740
==> Using 2 commits from #60740
HEAD is now at 6ab7a1fda2 benthos: update 3.28.0 bottle.
==> sdl_mixer: revision for FLAC support
==> Downloading https://api.github.com/repos/Homebrew/homebrew-core/actions/artifacts/16631782/zip
% git log -1
commit 4ad0537331a490af5aaf471ce89713e29e0c39d2 (HEAD -> master)
Author: Kreeblah <kreeblah@gmail.com>
Date:   Sun Sep 6 18:36:34 2020 -0700

    sdl_mixer: revision for FLAC support

    * Added FLAC music support to sdl_mixer formula
    * Added license

    Closes #60740.

Sorry, something went wrong.

MikeMcQuaid
MikeMcQuaid approved these changes Sep 15, 2020
View reviewed changes
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Looks pretty good and a nice idea. Might be nice to split out some logic to make it more testable and add some tests.

Library/Homebrew/dev-cmd/pr-pull.rb Outdated
Comment on lines 128 to 131
Formulary.from_contents(formula_name, full_path, old_file, :stable)
rescue FormulaUnavailableError
nil
end
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Formulary.from_contents(formula_name, full_path, old_file, :stable)
rescue FormulaUnavailableError
nil
end
Formulary.from_contents(formula_name, full_path, old_file, :stable)
rescue FormulaUnavailableError
nil
end

and same below

Library/Homebrew/dev-cmd/pr-pull.rb Outdated
return "#{formula_name} #{new_formula.stable.version}" if old_formula.stable.version != new_formula.stable.version
return "#{formula_name}: revision #{reason}" if old_formula.revision != new_formula.revision

"#{formula_name}: #{reason || "rebuild"}"
Copy link
Member

Choose a reason for hiding this comment

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

might be nice to split the reason or rebuild case into an early return like the above.

@jonchang jonchang mentioned this pull request Sep 15, 2020
Closed
5 tasks
MikeMcQuaid
MikeMcQuaid reviewed Sep 15, 2020
View reviewed changes
Library/Homebrew/dev-cmd/pr-pull.rb
Comment on lines 418 to 396
cherry_pick_pr!(pr, path: tap.path, args: args)
autosquash!(original_commit, path: tap.path, args: args) if args.autosquash?
signoff!(pr, tap: tap, args: args) unless args.clean?
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if these three functions could all split off into other files with unit tests. Just aware that this file is getting pretty long, only likely to get longer and has essentially no testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'm thinking of maybe moving into utils/git.rb. Have split some stuff off into their own functions and thinking about how I can do tests.

@jonchang
Copy link
Contributor Author

I've split another branch off with the refactoring and test improvements, so gonna merge this in for now and work on the refactor so this pull request doesn't get huge.

@jonchang jonchang merged commit 164f0f0 into Homebrew:master Sep 17, 2020
@jonchang jonchang deleted the pr-pull-autosquash branch September 17, 2020 09:01
@jonchang jonchang mentioned this pull request Sep 17, 2020
Merged
6 tasks
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Dec 11, 2020
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Dec 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@MikeMcQuaid MikeMcQuaid

Assignees
No one assigned
Labels
outdated PR was locked due to age
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jonchang @MikeMcQuaid @BrewTestBot