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.

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

audit: compare current version to last committed version when seeing if revision should be reset #8576

Merged
merged 1 commit into from
Sep 3, 2020

Conversation

dtrodrigues
Copy link
Member

@dtrodrigues dtrodrigues commented Sep 2, 2020

  • 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?

#7684 refactored the logic to audit if formula version/revision numbers made sense. It compared the previous version with the newest committed version to see if the revision should have been reset. However, in the case where the last commit is a was a different version than the current formula, previous_version and newest_committed_version will be equal, even though they both differ from the current version.

This change now compares the current formula version with the previous committed version to see if the revision should have be reset.

See Homebrew/homebrew-core#60536 (comment)

cc @MikeMcQuaid @SMillerDev

Sorry, something went wrong.

@dtrodrigues
Copy link
Member Author

This isn't quite right either -- if you're already up to date with origin/master, then current_version and newest_committed_version will point to the same commit.

@dtrodrigues dtrodrigues force-pushed the revision-audit branch 2 times, most recently from 5a85a13 to f01d2b1 Compare September 2, 2020 21:29
@dtrodrigues
Copy link
Member Author

Looks like we did need to check for both the case that was already tested for as well as this new case.

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.

Good catch and thanks for the new test. One suggestion, can merge with or without, your call.

@@ -896,6 +896,11 @@ def audit_revision_and_version_scheme
current_revision == newest_committed_revision &&
current_revision == previous_revision
problem "'revision #{current_revision}' should be removed"
elsif current_version != newest_committed_version &&
Copy link
Member

Choose a reason for hiding this comment

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

(current_version != newest_committed_version || previous_version != newest_committed_version) would allow this to be combined with the section above.

@dtrodrigues dtrodrigues merged commit 5ce013b into Homebrew:master Sep 3, 2020
@dtrodrigues dtrodrigues deleted the revision-audit branch September 3, 2020 14:33
@Rylan12 Rylan12 mentioned this pull request Sep 19, 2020
5 tasks
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Dec 14, 2020
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Dec 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants