Fix gitattributes filter snapshot edge cases by joshka · Pull Request #9676 · jj-vcs/jj

joshka · 2026-06-21T07:37:08Z

This splits the follow-up to #9635 into focused review commits. The first commits characterize the wrong behavior found during review, and the following commits update those same tests while fixing the corresponding path.

Review guide:

test: characterize filtered snapshot paths / git-lfs: fix filtered snapshot paths cover stale .gitattributes fallback and tracked files under ignored directories.
test: characterize gitattributes lookup errors / git-lfs: propagate gitattributes lookup errors cover parse and read errors from attributes lookup. The fix commit also keeps optional .gitattributes probes from failing when an ancestor path is a file, with a separate regression test for the NotADirectory case found after CI exercised rename detection.
test: characterize run temp snapshot filters / git-lfs: pass ignore filters to run snapshots cover temporary snapshots created by jj run.
test: characterize diffedit temp snapshot filters / git-lfs: pass ignore filters to diffedit snapshots cover temporary snapshots created by diff editing.
The remaining commits add deletion-path coverage, update gix-attributes, keep no-default-features builds clean, document the snapshot-only behavior, and group jj run worker inputs in a small context struct so clippy's argument-count lint stays green.

The rebuilt stack was compared against the previous flat version of this PR. The intentionally new follow-up delta is limited to the NotADirectory .gitattributes regression fix/test and the RunContext clippy cleanup.

Validation run locally:

cargo test -p jj-lib test_disk_file_loader_ignores_file_in_attributes_path -- --nocapture
cargo test -p jj-cli test_diff_renamed_file_and_dir -- --nocapture
cargo clippy -p jj-cli --all-targets --all-features -- -D warnings
cargo +nightly fmt --all -- --check

Checklist

If applicable:

I have updated CHANGELOG.md
I have updated the documentation (README.md, docs/, demos/)
I have updated the config schema (cli/src/config-schema.json)
I have added/updated tests to cover my changes
I fully understand the code I'm changing, even if it was initially suggested/generated by an AI assistant
For any prose generated by an AI assistant, I have proof-read and copy-edited it for correctness, clarity, and voice

github-actions

The following commits do not follow our format for subject lines:

311503a: Fix gitattributes snapshot review issues

Commits should have a subject line following the format <topic>: <description>. Please review the commit guidelines for more information.

All commits are now correctly formatted. Thank you for your contribution!

martinvonz · 2026-06-21T08:08:13Z

Reminder to fill out the PR template. I'm mentioning this especially because the PR description sounds quite LLM-generated.

martinvonz · 2026-06-21T08:10:17Z

Problems Found And Fixed

This is a long list. Would it make sense to create one commit per item (roughly)?

Add regression tests for snapshot paths where filter-matched files are handled incorrectly. The expectations document pre-fix behavior, so this commit passes before the implementation changes.

Create gitattributes lookup state from the tree being snapshotted, not from the empty initial tree. Also apply filter ignores while scanning tracked files through ignored-directory shortcuts, so snapshot preserves filtered tree contents consistently.

Document that filter matching currently treats a failed .gitattributes load as a non-match. That lets snapshot continue with content that may have been protected by a configured filter, so the next commit changes this to a visible error.

Return an error from filter matching when .gitattributes cannot load. Snapshot should fail instead of treating unreadable attributes as non-matches and recording filtered content. Still ignore optional attribute probes when an ancestor path is a file. Those NotADirectory lookups mean no .gitattributes file exists there, not that attribute loading failed.

Document that jj run snapshots its temporary working copy without the configured gitattributes filter ignores. The test records the pre-fix rewrite so the next commit can change only implementation and expected result.

Use configured git.ignore-filters when jj run snapshots its temporary working copies. Without this, commands can rewrite tracked files even though normal snapshots would leave them untouched.

Document that external diffedit currently snapshots its temporary output working copy without configured gitattributes filter ignores. The test records the pre-fix edited output before the implementation is fixed.

Use git.ignore-filters for external diffedit output snapshots. Otherwise an editor can rewrite tracked files even though normal snapshots would preserve tree contents.

Add regression coverage for the deletion snapshot path. The original PR already preserves tracked filter-matched files when they disappear from disk; this keeps that separate path covered alongside the update paths fixed earlier in the stack.

Carry the reviewed gix-attributes version and update the lockfile. This keeps dependency churn easy to review apart from behavior changes.

Move print_error_sources into the git-only transaction path so no-git builds do not keep an unused import. This is build hygiene exposed while validating the temporary snapshot fixes with --no-default-features.

Document that git.ignore-filters applies only while jj snapshots local working-copy contents. Imports from Git commits still preserve committed tree contents, so the setting is not part of Git import semantics. Keep the schema docs aligned with the new configuration key.

Collect the shared jj run worker inputs in a small context struct so the producer entry point stays under clippy's argument-count limit.

joshka · 2026-06-21T17:58:47Z

I did a deeper validation pass to make the review easier. The issues in this stack are real code-path problems, not just theoretical edge cases, and the tests are meant to document the failure modes separately.

Snapshot fallback for .gitattributes could use stale tree state. If disk .gitattributes is missing after checkout, snapshot can fall back to the wrong tree and record disk contents for a filtered file that should preserve the existing tree content. The problem is characterized in test: characterize filtered snapshot paths, and fixed in git-lfs: fix filtered snapshot paths.
Tracked files under .gitignore-ignored directories use a shortcut traversal path. That path also needs .gitattributes filtering, or a tracked LFS-filtered file under an ignored directory can be rewritten from disk. This is covered by the same characterization/fix pair above.
Deleted tracked files are handled by a separate snapshot path. If that path skips filter checks, deleting a tracked filtered file can record a deletion instead of preserving tree content. This is covered explicitly in test: cover filtered tracked file deletions.
Real .gitattributes load failures should be visible. Treating load errors as “no match” can silently snapshot content that may have been intentionally excluded. The problem is characterized in test: characterize gitattributes lookup errors, and fixed in git-lfs: propagate gitattributes lookup errors.
NotADirectory while probing optional .gitattributes files is different from a real load failure. A probe like file/.gitattributes just means no attributes file exists there. Treating it as fatal caused rename detection to fail with an internal snapshot error. The focused regression test and fix are in git-lfs: propagate gitattributes lookup errors.
jj run snapshots temporary working copies after running the command. Those snapshots need configured git.ignore-filters, or jj run can rewrite tracked filtered files that normal snapshots would preserve. The problem is characterized in test: characterize run temp snapshot filters, and fixed in git-lfs: pass ignore filters to run snapshots.
External diffedit has the same temporary-working-copy shape. The editor output snapshot also needs configured filters, or an editor can rewrite tracked filtered files through the temp output tree. The problem is characterized in test: characterize diffedit temp snapshot filters, and fixed in git-lfs: pass ignore filters to diffedit snapshots.
The no-default-features build exposed a real cfg hygiene issue: print_error_sources was imported outside the git-only path that uses it. That is fixed in cli: gate git-only error import.
The clippy failure was also real: run_inner() exceeded the argument-count lint after filter plumbing. The fix groups the shared worker inputs in RunContext without changing behavior in cli: group run worker context.

I re-ran the targeted validation locally:

cargo test -p jj-lib test_gitattributes_ -- --nocapture
cargo test -p jj-cli test_run_gitattributes_filter_in_temp_snapshot -- --nocapture
cargo test -p jj-cli test_diffedit_gitattributes_filter_in_temp_snapshot -- --nocapture
cargo test -p jj-cli test_diff_renamed_file_and_dir -- --nocapture
cargo check -p jj-cli --no-default-features
cargo clippy -p jj-cli --all-targets --all-features -- -D warnings
cargo +nightly fmt --all -- --check

sbarfurth

I tried this and it (still) works well. In terms of Rust code quality I can't promise the best review, but to my eye it looks perfectly fine. LGTM.

PhilipMetzger · 2026-06-22T15:41:23Z

imo, this belongs into @kejadlen/@sbarfurth's PR adding the feature

I think the goal of this PR is to merge into mine (or the one from @kejadlen), right? At least that was my understanding.

yes - this is an intentionally stacked PR on that. The intent here was to review, but rather than just give vague fix this problem reviews coming from codex, this more properly tests that the problems listed are problems and then fixes them

joshka · 2026-06-22T19:04:32Z

@sbarfurth I've rebased and merged these into your PR.

joshka requested a review from a team as a code owner June 21, 2026 07:37

github-actions Bot previously requested changes Jun 21, 2026

View reviewed changes

joshka requested a review from sbarfurth June 21, 2026 07:39

joshka force-pushed the joshka/fix-gitattributes-snapshot-review branch from 311503a to fe54182 Compare June 21, 2026 07:40

joshka mentioned this pull request Jun 21, 2026

git-lfs: add ability to ignore files based on git attributes #9635

Open

6 tasks

joshka force-pushed the joshka/fix-gitattributes-snapshot-review branch from fe54182 to 65fd11f Compare June 21, 2026 07:44

joshka force-pushed the joshka/fix-gitattributes-snapshot-review branch 4 times, most recently from 10a1b7b to fa94c4e Compare June 21, 2026 17:06

joshka added 3 commits June 21, 2026 10:09

test: characterize filtered snapshot paths

ea7f3de

Add regression tests for snapshot paths where filter-matched files are handled incorrectly. The expectations document pre-fix behavior, so this commit passes before the implementation changes.

test: characterize gitattributes lookup errors

6fb3558

Document that filter matching currently treats a failed .gitattributes load as a non-match. That lets snapshot continue with content that may have been protected by a configured filter, so the next commit changes this to a visible error.

joshka force-pushed the joshka/fix-gitattributes-snapshot-review branch from fa94c4e to b087429 Compare June 21, 2026 17:10

joshka added 10 commits June 21, 2026 10:43

test: characterize run temp snapshot filters

98d1a80

Document that jj run snapshots its temporary working copy without the configured gitattributes filter ignores. The test records the pre-fix rewrite so the next commit can change only implementation and expected result.

git-lfs: pass ignore filters to run snapshots

fdb05b4

Use configured git.ignore-filters when jj run snapshots its temporary working copies. Without this, commands can rewrite tracked files even though normal snapshots would leave them untouched.

test: characterize diffedit temp snapshot filters

9e7ca90

Document that external diffedit currently snapshots its temporary output working copy without configured gitattributes filter ignores. The test records the pre-fix edited output before the implementation is fixed.

git-lfs: pass ignore filters to diffedit snapshots

27dece1

Use git.ignore-filters for external diffedit output snapshots. Otherwise an editor can rewrite tracked files even though normal snapshots would preserve tree contents.

test: cover filtered tracked file deletions

26096fa

Add regression coverage for the deletion snapshot path. The original PR already preserves tracked filter-matched files when they disappear from disk; this keeps that separate path covered alongside the update paths fixed earlier in the stack.

cargo: update gix-attributes dependency

84d446b

Carry the reviewed gix-attributes version and update the lockfile. This keeps dependency churn easy to review apart from behavior changes.

cli: gate git-only error import

20c936e

Move print_error_sources into the git-only transaction path so no-git builds do not keep an unused import. This is build hygiene exposed while validating the temporary snapshot fixes with --no-default-features.

cli: group run worker context

b5d9f45

Collect the shared jj run worker inputs in a small context struct so the producer entry point stays under clippy's argument-count limit.

joshka force-pushed the joshka/fix-gitattributes-snapshot-review branch from b087429 to b5d9f45 Compare June 21, 2026 17:43

sbarfurth approved these changes Jun 22, 2026

View reviewed changes

sbarfurth reviewed Jun 22, 2026

View reviewed changes

PhilipMetzger reviewed Jun 22, 2026

View reviewed changes

joshka merged commit cf6c9db into jj-vcs:sbarfurth/push-uulvmqxnpmzk Jun 22, 2026
3 checks passed

Navigation Menu

Uh oh!

Fix gitattributes filter snapshot edge cases - #9676

Fix gitattributes filter snapshot edge cases#9676
joshka merged 13 commits into
jj-vcs:sbarfurth/push-uulvmqxnpmzkfrom
joshka:joshka/fix-gitattributes-snapshot-review

joshka commented Jun 21, 2026 •

edited

Loading

Uh oh!

github-actions Bot left a comment

Uh oh!

martinvonz commented Jun 21, 2026

Uh oh!

martinvonz commented Jun 21, 2026

Problems Found And Fixed

Uh oh!

joshka commented Jun 21, 2026

Uh oh!

sbarfurth left a comment

Uh oh!

PhilipMetzger Jun 22, 2026

Uh oh!

sbarfurth Jun 22, 2026

Uh oh!

joshka Jun 22, 2026

Uh oh!

Uh oh!

joshka commented Jun 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

joshka commented Jun 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

martinvonz commented Jun 21, 2026

Uh oh!

martinvonz commented Jun 21, 2026

Problems Found And Fixed

Uh oh!

joshka commented Jun 21, 2026

Uh oh!

sbarfurth left a comment

Choose a reason for hiding this comment

Uh oh!

PhilipMetzger Jun 22, 2026

Choose a reason for hiding this comment

Uh oh!

sbarfurth Jun 22, 2026

Choose a reason for hiding this comment

Uh oh!

joshka Jun 22, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

joshka commented Jun 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

joshka commented Jun 21, 2026 •

edited

Loading