Git Lfs - #7098
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
@weiznich is missing CLA 😔 |
a7bda24 to
17485ec
Compare
There was a problem hiding this comment.
some minor nits, I am also cc'ing @06393993 since he's actively working on the Gitattributes design.
|
While none of the following questions should block the merge, they are probably worth answering:
|
I haven't considered it. My initial goal is to ignore all git-lfs files so we can handle them properly using normal git lfs commands. Like in gitignore, if you want to start tracking using gitlfs, you need first to I'm open/available to do any requested change.
That is a great question. If we don't parse .gitiattributes, the snapshot engine will interpret that gitattributes filtered files are part of the change. It's already how we are doing for gitignore. If we can answer this question for gitattributes we can also proabably do the same for gitignore.
Thanks for pointing this out. That part of the code was taken from
Probably for next steps, is to add commands under In any case, I haven't planned for that and I'm open to suggestions or at least make the code more prepared for future work. |
|
I am asking in details just because this CL is very different from my design, that I plan to propose early this August, and am nervous about the unfortunate conflict: either my design will be rejected, or we need to revert this PR to adopt my design. If you are not hurry with this PR, I am wondering if you could read my design if you haven't already?
Sorry for not being clear, but I was asking about the behavior of the For example, if one runs And when it comes to implement the checkout path, merge conflicts on Applogize again that I didn't make myself clear what I mean on my original question. I personally don't think this potentially inconsistency should block this PR, but I generally am not sure about the standard of the jj project, so just raise the question so that the core maintainers won't overlook them.
That's not necessarily the case. We can still ignore those files in
I don't think we should do the same to
From the gitattributes documentation. While it is also possible to read and parse For example, in a Rust project, people usually put
Nice catch on the inconsistent gitignore behavior with git. As of the next step, I am not sure. However, if I were you I would:
But, changes to gitignore behaviors will be in another commit if not another PR.
Sorry I don't understand why would we need to introduce a command to update
My simplisitic thought is that, the current interface doesn't make it easy to integrate with future tasks. For both the filter feature(which should solve the git lfs support once and for all) and the EOL conversion feature, we need to know the exact state of an attribute. And the EOL conversion feature needs to query the state of the
I would argue that my design is more ergonomic, because with my design the diff tool code doesn't need to build up the |
Thanks for sending me the design proposal. I was out of the loop but now I'm gonna take a look at it. I'm not in a hurry with this PR, it's just that I've been using outdated versions of jj and alternatives PR/branches for months now and I'd like to have some support soon. In any case, I'm gonna review this proposal and try to make this PR compatible with it or give feedback about it (if I find something that doesn't fit). |
|
Thanks. The plan for my design is to go through the formal design process to avoid surprises on code review. I was somewhat actively discussing the design in the dicord channel mentioned in the readme file: https://github.com/jj-vcs/jj/blob/main/README.md?plain=1#L11 |
|
Regarding |
|
I built this PR locally with I added the following to my [git]
ignore-filters = ["git-crypt"]And I have the following in my Yet when I run What am I doing wrong? |
You probably need to do |
|
@gusinacio @06393993 do you folks think this PR is ready? We'd really like to make jj become usable in our org as our repo has a lot of LFS files. I've been trying this off of this branch, works fairly well for our use case. But it's harder to share with the rest of the org as we'd prefer a pre-installed binary in the VSCode Devcontainer. Any further progress in this PR would be helpful for us. Thanks in advance |
|
Re udaya2899, I am not a maintainer, so I can't make the decision. But I personally am not against this PR, because we can always introduce fixes afterwards. However, this PR does have some problems that I don't think we have a good answers for:
|
|
Thanks 06393993 for clarifying. I request @yuja or @martinvonz or any other maintainer to pitch-in and help resolve the questions and merge this. This PR makes |
|
I've also been manually compiling this branch locally in order to make It could even be flagged as an experimental feature, as has been done with other configuration options (like auto-advance branches), just so users can start playing with it without having to rebase and compile custom branches, and most importantly allow the average |
| return Ok(None); | ||
| }; | ||
| // try to resolve the file | ||
| let id = match &file_to_merge.resolve_trivial(SameChange::Accept) { |
There was a problem hiding this comment.
I am not very familiar with the purpose of adding the same_change parameter, but most old call sites just use SameChange::Accept, so I assume SameChange::Accept is correct.
06393993
left a comment
06393993
left a comment
There was a problem hiding this comment.
- Can we squash the "git-attributes: use .gitattributes file for current folder" commit into the "git-attributes: add git attributes file support" commit?
- We are missing some tests but that can be another PR from me if you don't want to do the job:
- Tests for the 2
FileLoaderimplementations.TreeFileLoaderis especially complicated and worth testing. - Stress test on the depth of the folder to ensure that the recursion implementation won't result in stack overflow under reasonable workload.
- Simple integration tests to make sure that everything is glue-ed correctly.
- Tests for the 2
- I may change the design to specify the
FileLoaderpriority atGitAttributescreation time, and we allow aribtrary number ofFileLoaderto initializeGitAttributes. This could simplify the implementation. But that's another issue to solve, and will be a different PR.
15e0ea5 to
f26a7cf
Compare
06393993
left a comment
06393993
left a comment
There was a problem hiding this comment.
The current design handles only snapshot(disk -> store) but doesn't handle update(store -> disk) at all. While I understand why it's implemented such way, I am wondering if other maintainers will accept such confusing implementation. So we should definitely need an approval on this question from one of maintainers.
I am especially not quite sure how this would influence the file_states cache in the TreeState proto(in lib\src\protos\local_working_copy.proto). Such implementation can result a weird TreeState::file_states: after snapshot, the ignored files disappear in TreeState::file_states, but after update(or checkout), the ignored files will be added back to TreeState::file_states. Not sure how bad the consequence can be. Definitely need another eye on this.
In addition, I think it's worth explaining the "ignore only on snapshot" behavior in details in docs, including some possible weird behavior1, both in cli/src/config-schema.json where jj generates it's CLI help document and the online document(under the docs/config.md document).
Footnotes
-
e.g. if the user removes a file ignored in a revision, jumps to a different revision, and jumps back to the revision, they will see that the file appears again and is not removed ↩
| executable_path: settings.get("git.executable-path")?, | ||
| write_change_id_header: settings.get("git.write-change-id-header")?, | ||
| colocate: settings.get("git.colocate")?, | ||
| ignore_filters: settings |
There was a problem hiding this comment.
Should we also change cli/src/config-schema.json and relevant tests?
There was a problem hiding this comment.
I added to config-schema.json. Any examples on which tests to add for it?
There was a problem hiding this comment.
I added to config-schema.json. Any examples on which tests to add for it?
Check the test failure and you can tell what you missed. Currently, you fail the test_config_get_yields_values_consistent_with_schema_defaults test. But it's possible that you fail some other tests once you fix this one. So just rerun the entire test suites locally to know what you miss.
06393993
left a comment
06393993
left a comment
There was a problem hiding this comment.
In case you miss the message from discord channel:
Can you also take a look at #7098 (review) and reply to it? The current behavior is understandably weird in that we don't handle update(store -> disk), but only handle snapshot(disk -> store). I think we need to document in details why we need such weird behavior to avoid future maintainers from confusing or even incorrectly "fixing" this weird behavior. Besides, we should make sure maintainers know that this design is deliberate, and they accept such design.
PhilipMetzger
left a comment
PhilipMetzger
left a comment
There was a problem hiding this comment.
I think this would look good if we didn't choose another approach in the Design doc.
@PhilipMetzger which design doc are you referring to? I am looking at https://github.com/jj-vcs/jj/tree/1e521f564dcec8f93ba3b0a494259d38ef0d8341/docs/design and cannot see any mention of LFS. But maybe what you are referring to is some indirect effect of another design doc on attribute handling? It would be nice to understand if it would be doable to go with this "ignore lfs" approach and the work done by @gusinacio in this PR for the time being until the time when jujutsu will have implemented the "better-LFS" plans! |
Please refer to #8144 for the WIP filter gitattributes design. Thanks. |
martinvonz
left a comment
martinvonz
left a comment
There was a problem hiding this comment.
Thanks! Sorry about the very long delay in reviewing this. I hope you haven't given up on this PR.
| #[cfg(not(feature = "git"))] | ||
| pub fn base_attributes(&self) -> Result<Arc<GitAttributesFile>, ConfigGetError> { | ||
| Ok(Arc::new(GitAttributesFile::default())) | ||
| } | ||
|
|
||
| #[cfg(feature = "git")] | ||
| #[instrument(skip_all)] | ||
| pub fn base_attributes(&self) -> Result<Arc<GitAttributesFile>, ConfigGetError> { | ||
| Ok(Arc::new(GitAttributesFile::new( | ||
| &self.settings().git_settings()?.ignore_filters, | ||
| ))) | ||
| } | ||
|
|
There was a problem hiding this comment.
nit: use cfg! instead:
#[instrument(skip_all)]
pub fn base_attributes(&self) -> Result<Arc<GitAttributesFile>, ConfigGetError> {
if cfg!(feature = "git") {
Ok(Arc::new(GitAttributesFile::new(
&self.settings().git_settings()?.ignore_filters,
)))
} else {
Ok(Arc::new(GitAttributesFile::default()))
}
}That way we always compile-test both branches and IDEs can better understand both branches too.
| impl std::fmt::Debug for GitAttributes { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| f.debug_struct("GitAttributes").finish() | ||
| } | ||
| } |
| conflict_marker_style, | ||
| eol_conversion_mode: EolConversionMode::None, | ||
| fsmonitor_settings: FsmonitorSettings::None, | ||
| ignore_filters: Vec::new(), |
There was a problem hiding this comment.
Should not be needed. The user shouldn't be editing LFS files with jj, so they shouldn't need to edit diffs containing changes to LFS files. And even if they did, it would be fine to let them edit the tracked file (the one with a hash in it).
| /// The fsmonitor (e.g. Watchman) to use, if any. | ||
| pub fsmonitor_settings: FsmonitorSettings, | ||
|
|
||
| /// Ignore filters in .gitattributes |
There was a problem hiding this comment.
Can you add a bit more detail? What are the entries here? Here's a possibly better comment (not sure if correct):
/// Names of .gitattributes filters whose matching files should be ignored in the working copy
|
|
||
| // we use std::fs::symlink_metadata to not follow symlinks to follow Git's | ||
| // behavior. | ||
| let metadata = match std::fs::symlink_metadata(&path) { |
There was a problem hiding this comment.
nit: We usually write this as path.symlink_metadata()
| &self, | ||
| priority: SearchPriority, | ||
| ) -> Result<Arc<SearchAndCollection>, GitAttributesError> { | ||
| // we use pin because this is a recursive call |
There was a problem hiding this comment.
nit: move this a few lines down so it's just before the Box::pin() call?
| return false; | ||
| }; | ||
| let value = value.as_ref().as_bstr(); | ||
| ignore_filters.iter().any(|state| value == state) |
There was a problem hiding this comment.
nit: Should we use a HashSet<BString> instead so we don't have to scan the list? Or is the list more or less guaranteed to be very short?
| path: &RepoPath, | ||
| ignore_filters: &[String], | ||
| priority: SearchPriority, | ||
| ) -> impl Future<Output = bool>; |
There was a problem hiding this comment.
nit: Make the function async instead of (explicitly) returning a future
|
I'm closing this, there's too much uncertainty on how to handle lfs. |
|
If we're serious about getting this merged, I don't mind picking it back up to get it over the line. |
|
@kejadlen: Thanks! |
|
@kejadlen go ahead! |
|
New PR here: #9068 |
11 participants
Partially fixes #80. There's no handling of Git LFS inside jj, it just ignores so you can handle with
git lfs <command>.Thanks to @bcspragu with #6036, @weiznich #5480, and @kejadlen (where I forked the current branch).
Checklist
CHANGELOG.mdREADME.md,docs/,demos/)cli/src/config-schema.json)