Skip to content

Avoid generated rmdir for shared file lists#22940

Merged
bevanjkay merged 2 commits into
Homebrew:mainfrom
loganrosen:generate-zap-sharedfilelist-rmdir
Jul 8, 2026
Merged

Avoid generated rmdir for shared file lists#22940
bevanjkay merged 2 commits into
Homebrew:mainfrom
loganrosen:generate-zap-sharedfilelist-rmdir

Conversation

@loganrosen

@loganrosen loganrosen commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

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

  • AI was used to generate or assist with generating this PR.

GitHub Copilot CLI helped make this small change. I verified it with the new regression test, ./bin/brew lgtm --online, and ./bin/brew generate-zap transmission.


This prevents brew generate-zap from suggesting rmdir for Apple's shared recent documents directory when it finds an app-specific .sfl* file under that directory.

For example, before this change, running:

brew generate-zap transmission

could suggest removing:

rmdir: "~/Library/Application Support/com.apple.sharedfilelist/com.apple.LSSharedFileList.ApplicationRecentDocuments"

zap rmdir only removes directories if they are empty, so this would not remove
other applications' shared file list entries.
However, this parent directory is macOS-owned and not app-specific, so including
it in generated cask-specific zap stanzas is misleading noise.
The matching app-specific shared file list file is already included under
trash.

This adds that directory to the generated rmdir exclusions and adds a regression test for the derived rmdir candidates.

./bin/brew lgtm --online passes locally.

Do not derive rmdir candidates for Apple's application recent documents directory when generate-zap finds matching shared file list files.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 4, 2026 03:24

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 adjusts brew generate-zap to avoid generating rmdir directives for macOS-owned Shared File List “ApplicationRecentDocuments” directories, which can contain state for multiple applications and should not be removed as part of an app-specific zap stanza.

Changes:

  • Add the Shared File List “ApplicationRecentDocuments” directory to the rmdir exclusion list used when deriving rmdir candidates.
  • Add a regression spec ensuring derive_rmdir_candidates does not suggest rmdir for app-specific .sfl* files under that shared directory.
Show a summary per file
File Description
Library/Homebrew/dev-cmd/generate-zap.rb Excludes the macOS-owned Shared File List recent-documents directory from derived rmdir candidates.
Library/Homebrew/test/dev-cmd/generate-zap_spec.rb Adds a regression test to ensure no rmdir is suggested for that shared recent-documents directory.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 2/2 changed files
  • Comments generated: 0
  • Review effort level: Low

@bevanjkay

bevanjkay commented Jul 4, 2026

Copy link
Copy Markdown
Member

Thanks for the PR @loganrosen - I don't disagree with making this change, because it doesn't make sense for this to be included in the zap stanza for every cask, however the logic behind the reason for the change is not quite right. rmdir would actually only remove the directory if the folder is empty, so there's no risk of it removing additional paths.

From a philosophical point of view, it it is worth noting the zap is expected to remove shared application files at time, but if the entire folder in question were to be removed by a zap stanza (with zap trash or zap delete) that would be an overreach in my opinion.

I have a specific comment on the test itself, but I'll leave that against the actual code for context.

Comment thread Library/Homebrew/test/dev-cmd/generate-zap_spec.rb
Assert the generated rmdir candidates exclude the shared recent documents directory itself, rather than only checking the enclosed app-specific file path.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bevanjkay bevanjkay added this pull request to the merge queue Jul 8, 2026
Merged via the queue into Homebrew:main with commit a34c4ca Jul 8, 2026
41 checks passed
@loganrosen loganrosen deleted the generate-zap-sharedfilelist-rmdir branch July 9, 2026 20:20
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.

3 participants