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

utils/popen: add safe argument to popen_read and popen_write #10305

Merged
merged 3 commits into from
Jan 13, 2021

Conversation

SeekingMeaning
Copy link
Contributor

@SeekingMeaning SeekingMeaning commented Jan 12, 2021

  • 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?
  • Have you successfully run brew man locally and committed any changes?

Follow-up to #10245

This PR adds a safe argument to popen_read and popen_write that can then be used in Utils.git_head

The reason for adding this is that formulae use safe_popen_read to get the git head, while Utils.git_head currently uses popen_read. Since Utils.git_head should primarily be used by formulae, this PR adds a safe argument to git_head that has a default value of true (whereas the default value is false for popen_read)

I also added tests for safe_popen_read and safe_popen_write which passed with the original methods as seen in the test runs for commit 40d85ec: https://github.com/Homebrew/brew/runs/1689977863

@BrewTestBot
Copy link
Member

Review period will end on 2021-01-13 at 17:55:14 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Jan 12, 2021
@SeekingMeaning SeekingMeaning changed the title popen_spec: add tests for safe_popen_read and safe_popen_write utils/popen: add safe argument to popen_read and popen_write Jan 12, 2021
@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Jan 13, 2021
@BrewTestBot
Copy link
Member

Review period ended.

@SeekingMeaning SeekingMeaning merged commit be6dd72 into Homebrew:master Jan 13, 2021
mistydemeo added a commit that referenced this pull request Jan 13, 2021
These were introduced in #10305 but only started going red
after that PR was merged.
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Feb 13, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Feb 13, 2021
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