Block selected Git filters before staging patch paths#30850
Block selected Git filters before staging patch paths#30850bookholt-oai wants to merge 15 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db8a0aeadf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| git_root: &Path, | ||
| paths: &[String], | ||
| ) -> io::Result<()> { | ||
| ensure_no_selected_executable_git_filters(git, git_root, paths, &[])?; |
There was a problem hiding this comment.
For paths whose selected driver has only filter.<driver>.smudge configured, this new staging guard now returns Unsupported before git add, even though staging does not execute smudge helpers. Git's gitattributes docs describe smudge as the checkout path and clean as the check-in/staging path, with process taking precedence for filtering, so direct staging or revert pre-staging now rejects repositories that Git could stage safely; use a staging-specific guard that ignores smudge-only entries while still blocking clean and process.
Useful? React with 👍 / 👎.
| git_root: &Path, | ||
| paths: &[String], | ||
| ) -> io::Result<()> { | ||
| ensure_no_selected_executable_git_filters(git, git_root, paths, &[])?; |
There was a problem hiding this comment.
When the diff names a deleted or otherwise missing file that is selected by a clean/process filter, this guard now rejects before the later symlink_metadata pass drops non-existent leaves and returns without running git add. That makes direct staging fail for cases where no executable helper could run and the index would have remained untouched, such as a missing *.txt filter=lfs path; limit the guard to the existing leaves that will actually be passed to git add.
Useful? React with 👍 / 👎.
| git_root: &Path, | ||
| paths: &[String], | ||
| ) -> io::Result<()> { | ||
| ensure_no_selected_executable_git_filters(git, git_root, paths, &[])?; |
There was a problem hiding this comment.
When a user happens to have a filter driver named unspecified (or unset) configured, this new staging guard rejects ordinary paths that do not select any filter because git check-attr filter reports the sentinel value unspecified for those paths and the guard treats it as a real driver name. In that configuration git add would not run the helper for an unfiltered file, so direct staging now fails spuriously; filter out Git's sentinel attribute values before comparing them with configured drivers.
Useful? React with 👍 / 👎.
…4394-git-05b-stage-filters
…ec-4394-git-05b-stage-filters
# Conflicts: # codex-rs/git-utils/src/safe_git.rs # codex-rs/git-utils/src/safe_git_tests.rs
…4394-git-05b-stage-filters
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e674ac9a52
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let (_code, _, _) = run_git(git, git_root, git_config_args, &args)?; | ||
| // We do not hard fail staging; best-effort is OK. Return Ok even on non-zero. | ||
| let (existing, content_filter_paths) = classify_exact_staging_leaves(config, paths)?; | ||
| let _result = update_index_exact_paths_from_apply(config, &existing, &content_filter_paths)?; |
There was a problem hiding this comment.
When a reverse apply touches an ignored or sparse-excluded path together with a modified tracked file, update_index_exact_paths_from_apply now returns a non-zero policy result before staging any of the eligible paths, and this wrapper discards that result. apply_git_patch then continues to git apply -R with the tracked files still unstaged, so they fail with an index mismatch; the previous git add ignored tracked best-effort path still staged the tracked file even though Git returned non-zero for the skipped path. Consider filtering out paths Git would skip for pre-staging, or otherwise preserving best-effort staging for the eligible paths before continuing.
Useful? React with 👍 / 👎.
| let git_config_args = safe_git_config_parts(); | ||
| ensure_no_worktree_config_sources(&git, git_root, &git_config_args)?; | ||
| git.ensure_repository_root_route(git_root)?; | ||
| let canonical_root = std::fs::canonicalize(git_root)?; |
There was a problem hiding this comment.
When stage_paths is called with a relative repository route such as linked-route from inside a primary worktree, this guard passes that relative spelling through to ensure_repository_root_route. The new policy only rejects crossings when it can also prove the route's parent is a related worktree, but a single-component relative path has no parent (and multi-component relative parents are not the absolute worktree ancestors), so a symlink/junction into a linked worktree that would be rejected when passed as an absolute path can be allowed. Convert the caller's lexical route to an absolute path before this check while still preserving the symlink spelling.
Useful? React with 👍 / 👎.
1 participant
Why
Staging is a separate Git execution boundary from applying a patch. A path that was validated as a file can become a directory before
git add, causing Git to recurse into unchecked files and run a filter selected for one of them.Approach
Stage only the exact normalized file paths with a literal, non-recursive
update-indexcommand.Reuse the parent operation's configuration-source authorization, but make a fresh decision about filters, attributes, the index, the filesystem, ignore rules, and sparse checkout immediately before staging. Reject selected clean and process filters, while allowing an optional smudge-only filter when it is not required.
Preserve normal behavior for deletions, file symlinks, executable bits, ignored files, and sparse paths. Refuse unsafe index flags, path aliases, file/directory collisions, Windows path ambiguity, and crossings between related worktrees.
This PR is stacked on #30848 and should land after it.
Testing
codex-git-utilstests passed.