fix: only expand attribute macros when they are set#2640
Closed
Nils Andre (NilsIrl) wants to merge 1 commit into
Closed
fix: only expand attribute macros when they are set#2640Nils Andre (NilsIrl) wants to merge 1 commit into
Nils Andre (NilsIrl) wants to merge 1 commit into
Conversation
Git expands a macro's sub-assignments only when the matching rule sets the macro - `attr.c`'s `macroexpand_one()` acts only `if (item->macro && item->value == ATTR__TRUE)`. We expanded macros in any matched state, so `-macro`, `!macro`, and `macro=value` also applied the expansion, making attributes like `filter` appear set where `git check-attr` reports them as unspecified. The new `macro-states` baseline fixture records git's answers for all four assignment states of a macro and verifies parity.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds coverage and implementation for Git-compatible macro expansion behavior in attribute search outcomes (macros expand only when explicitly set).
Changes:
- Add a regression test and new fixture baseline for macro state handling (set/unset/unspecified/value).
- Update attribute outcome processing to expand macros only when the macro attribute is in the
Setstate. - Extend fixture generation script to produce baselines for the new macro-state scenarios.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| gix-attributes/tests/search/mod.rs | Adds a new test asserting macros expand only when set. |
| gix-attributes/tests/fixtures/make_attributes_baseline.sh | Generates a new fixture repo and baseline outputs for macro state cases. |
| gix-attributes/src/search/outcome.rs | Changes macro expansion condition to match Git behavior (expand only on Set). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+147
to
+150
| for (rela_path, expected) in (baseline::Expectations { lines: input.lines() }) { | ||
| actual.reset(); | ||
| group.pattern_matching_relative_path(rela_path, Case::Sensitive, None, &mut actual); | ||
| assert_references(&actual); |
Comment on lines
+102
to
+103
| git init | ||
| : > user.attributes |
2 tasks
Member
|
Thanks a lot. This PR was superseded by an alternative implementation that also compares this behavior against a git baseline. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2639.
What
Outcome::fill_attributes()expanded a macro's sub-assignments whenever a pattern referencing the macro matched, regardless of the assignment's state. Git expands a macro only when the matching rule sets it —attr.c'smacroexpand_one()acts onlyif (item->macro && item->value == ATTR__TRUE)— so-macro,!macro, andmacro=valuemust not apply the expansion.The fix gates the expansion on
State::Setwhile still recording the macro's own match (git also reports e.g.mymacro: unsetfor a-mymacrorule; only the expansion is suppressed).Tests
A new
macro-statesrepository inmake_attributes_baseline.shrecordsgit check-attr -aoutput for one path per assignment state (macro,-macro,!macro,macro=value), and a newmacros_are_only_expanded_when_settest asserts parity, following the existing baseline-test pattern. Before the fix it fails with the full expansion showing up for the three non-set states; the existinglookup-ordermacro-cycle/recursion tests still pass since those macros are matched as set.Ran
cargo test -p gix-attributes,GIX_TEST_IGNORE_ARCHIVES=1 cargo test -p gix-attributes --test attributes,cargo test -p gix-worktree,cargo clippy -p gix-attributes --all-targets, andcargo fmt --check.AI disclosure
This change was researched, written, and tested by an AI agent (Claude Fable 5, via Claude Code) directed and reviewed by Nils Andre (@NilsIrl); the commit is authored accordingly. The underlying issue (#2639) includes the standalone reproduction and the
attr.ccomparison.