Skip to content

utils/git: only mutate describe-cache when .git is user-owned - #23517

Merged
MikeMcQuaid merged 2 commits into
mainfrom
git-describe-cache-ownership
Aug 14, 2026
Merged

utils/git: only mutate describe-cache when .git is user-owned#23517
MikeMcQuaid merged 2 commits into
mainfrom
git-describe-cache-ownership

Conversation

@p-linnane

Copy link
Copy Markdown
Contributor

set-homebrew-version-from-git deletes and recreates .git/describe-cache whenever it has to fall back to git describe. It did so unconditionally, so brew would rm -rf and mkdir -p inside a Git directory the invoking user does not own, e.g. an installation owned by another user or a .git that has been replaced by a symlink pointing elsewhere.

Only mutate the cache when .git is owned by the current user. Reads are unaffected, so a repository owned by someone else still uses an already populated cache and otherwise just falls back to git describe each run, which is all the cache was ever saving.


  • 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 Hyperfine benchmarks.
  • 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.

Claude Code (Opus 5) drafted the implementation and tests; I reviewed the diff, verified the new ownership test fails without the fix and passes with it, and ran brew lgtm + targeted specs.


Signed-off-by: Patrick Linnane <patrick@linnane.io>
Copilot AI lite review requested due to automatic review settings August 14, 2026 01:46

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.

Pull request overview

This PR hardens set-homebrew-version-from-git’s .git/describe-cache handling so brew only deletes/recreates the cache when the repository’s .git directory is owned by the invoking user, avoiding destructive writes inside Git directories owned by other users or reached via symlinked .git.

Changes:

  • Add an ownership gate (-O) around all .git/describe-cache mutations (cleanup + rewrite) in set-homebrew-version-from-git.
  • Refactor the function to use a shared git_directory variable for .git path accesses.
  • Add bash-level RSpec coverage to assert cache writes occur only when .git is user-owned.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
Library/Homebrew/utils/git.sh Gate .git/describe-cache deletion/creation behind a “.git is user-owned” check to prevent mutating non-owned Git dirs.
Library/Homebrew/test/bash_spec.rb Add tests validating cache writes when .git is owned and no cache mutation when .git is not owned (via symlink to /).

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

Comment thread Library/Homebrew/utils/git.sh Outdated

@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!

Co-authored-by: Carlo Cabrera <github@carlo.cab>
@MikeMcQuaid
MikeMcQuaid enabled auto-merge August 14, 2026 07:22
@MikeMcQuaid
MikeMcQuaid added this pull request to the merge queue Aug 14, 2026
Merged via the queue into main with commit 433288d Aug 14, 2026
45 checks passed
@MikeMcQuaid
MikeMcQuaid deleted the git-describe-cache-ownership branch August 14, 2026 08:00
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