git-lfs: ignore filtered files in .gitattributes during snapshot - #9068
git-lfs: ignore filtered files in .gitattributes during snapshot#9068kejadlen wants to merge 3 commits into
Conversation
8babab8 to
1d5d597
Compare
| ignore_filters: &HashSet<String>, | ||
| priority: SearchPriority, | ||
| ) -> bool { | ||
| let Ok(result) = self.search(path, ["filter"], priority).await else { |
There was a problem hiding this comment.
Dropping the Err path here, not sure if this should propagate an error up?
…s#8719 fixes (gitattr filter)
…s#8719 fixes (gitattr filter)
…s#8719 fixes (gitattr filter)
…s#8719 fixes (gitattr filter)
…s#8719 fixes (gitattr filter)
…s#8719 fixes (gitattr filter)
…s#8719 fixes (gitattr filter)
…s#8719 fixes (gitattr filter)
|
I believe I addressed Martin's comments, but I missed this if that's what you're referring to? |
|
I don't know if that's what Philip was referring to, but what's your answer to it anyway? Is it correct that this implementation doesn't handle the update case? What does that even mean? I thought we already handle the update case. Maybe @06393993 can explain what it is that doesn't work. |
From #9068 (comment):
I am not sure what "already handle the update case" means. Given that the update path is unaffected, the original questions in #7098 (review) are still valid:
|
Never mind, I was just confused. Sorry. It sounds like many users are fine with the current PR even though it's limited, so I think it seems fine to get it merged as is. Sorry that I still haven't taken the time to review it. |
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| #![expect(missing_docs)] |
There was a problem hiding this comment.
Please document it instead. We use these attributes only for old code that hasn't been documented yet.
There was a problem hiding this comment.
git-attributes: add git attributes file support
Could you add some more detail? Reading this, it's not at all obvious what to expect from the patch. I see that there's a FileLoader trait and bunch of switching between loading from disk or tree. It would be good to explain the rationale behind that.
There was a problem hiding this comment.
From description:
git-lfs: access git attributes to ignore filtered files
Can you elaborate? What did we do in the previous patch if we didn't do this?
yes, this is pretty much it. |
Link: https://stackoverflow.com/a/41961459 ("Move Git LFS tracked files under regular Git") Link: jj-vcs/jj#80 ("Add support for Git LFS") Link: jj-vcs/jj#9068 ("git-lfs: ignore filtered files in .gitattributes during snapshot")
|
one-line fix for the compilation error: raylu@5eb8aad |
|
I also uploaded a working branch to this repo that's based on the latest non-working snap of this PR:
|
33216b7 to
b31efce
Compare
|
I wanted to mention I'm really excited to see this feature implemented! LFS is one of the biggest sticking points in my mind when I try to evangelize jj 😁. Keep up the good work! |
…s#8719 fixes (gitattr filter)
…s#8719 fixes (gitattr filter)
…s#8719 fixes (gitattr filter)
…s#8719 fixes (gitattr filter)
jj-vcs#9068 LFS ignore-filters)
|
As a backup and given the willingness of maintainers, I have adapted this PR into #9635. It squashes the multiple commits in this PR. I've also attempted to address the comments from @martinvonz around documentation. I will happily drive my PR forward as many people in my organization are relying on this patched version and I want to lower the friction. I'm also happy to drop my PR and support here if that's better, but it seems this has been dormant for over a month now. |
jj-vcs#9068 was written against jj 0.40. Adapt to 0.42: - FileLoader uses futures::AsyncRead (Pin<Box<...>>) to match Store::read_file; drop the removed file_util::BlockingAsyncReader (read .gitattributes fully into a futures::io::Cursor). - make tokio a non-optional jj-lib dep (gitattributes uses tokio::sync::OnceCell unconditionally). - drop an unused dag_walk_async import.
jj-vcs#9068 LFS ignore-filters)
jj-vcs#9068 LFS ignore-filters)
jj-vcs#9068 LFS ignore-filters)
jj-vcs#9068 LFS ignore-filters)
8 participants
Partially addresses #80 by ignoring files matched by configurable
.gitattributesfilter attributes during snapshot. This lets users work with Git LFS (and git-crypt, etc.) repos in jj by excluding filtered files from tracking, deferring actual file handling to the external tool (e.g.git lfs pull).History
This work builds on several prior attempts to bring LFS support to jj:
gix-attributes-based parsing. Recently closed by the author due to uncertainty about the design direction.This PR picks up where #7098 left off, per #7098 (comment).
Approach
A new
git.ignore-filterssetting (defaulting to["lfs"]) names.gitattributesfilter attributes whose matching files should be excluded from snapshots. During snapshot, the working copy reads.gitattributesfiles from both disk and the tree store, matches paths againstgix-attributes, and skips files whosefilterattribute matches a configured ignore filter. Skipped files are also omitted from the deleted-files check so they don't appear as spuriously removed.This only affects the snapshot path (disk → store). Checkout (store → disk) is unaffected, which is intentional: files are still checked out normally so that
git lfs pullworks afterward.Known limitations
.gitattributesfiles in the repo are parsed on every snapshot, regardless of which directories changed..gitattributesfiles are followed (matching jj's current.gitignorebehavior, but diverging from git).These are acceptable tradeoffs for an initial implementation and can be refined alongside the broader gitattributes design (#7164, #8144).
Checklist
CHANGELOG.mdI have updated the documentation (README.md,docs/,demos/)cli/src/config-schema.json)how it works, how it's organized), including any code drafted by an LLM.
an eye towards deleting anything that is irrelevant, clarifying anything
that is confusing, and adding details that are relevant. This includes,
for example, commit descriptions, PR descriptions, and code comments.
(PR summary assisted by Claude Opus 4.6 via pi)