Repository navigation
Autominsize support - #6284
Autominsize support#6284gnkow wants to merge 7 commits into
Conversation
chrisd8088
left a comment
There was a problem hiding this comment.
Hey, thanks very much for this draft PR, and for trying to tackle a long-standing issue!
I typically avoid reviewing on PRs in a draft state, as I assume they are still a work in progress, but from your comment I think you are hoping for some discussion, so I thought I would at least make some initial comments.
As I understand it, the key design principle of this PR is to pass all files through Git LFS; that's the effect of a .gitattributes pattern which matches *.
This idea has been considered before; I think our primary concern is going to be the performance cost of running every file through the Git LFS filters. We'd definitely want to see some comparative timing measurements done on each of the platforms we support (Linux, macOS, and Windows) using a reasonably large Git repository as test data.
Git LFS was originally developed several large Git hosting services (Atlassian, GitHub, GitLab) as a kind of workaround which allowed them to avoid storing large binary files in their own platforms. By using .gitattributes patterns to invoke the Git LFS filters, though, they could limit the impact of this additional program and system to just a specific set of files.
As a high-level concern, if we were to now start recommending that users process every file through Git LFS, that seems like we are trending away from direction I think we likely would prefer, where Git itself can do more of the work necessary to store and transfer large files efficiently.
A few other notes I made while thinking about this approach:
- If the minimum size is relatively small, files such as
.gitattributesand.gitignoremay be converted to Git LFS objects, which is not appropriate as Git expects these to have a specific format that it can read. All such Git special files would need to be excluded automatically. - Since files managed with Git LFS can be quite large, using
os.ReadAll()may consume a considerable amount of memory. It would likely be necessary to spool the incoming data to a temporary file; ourtools.Spool()function does this already and is used in oursmudgefilter code to avoid deadlocking the pipe with Git (see PR #1932). - If a file below the threshold is removed from the working tree and then
git checkoutis run to recreate it, Git will invoke Git LFS as asmudgefilter, which assumes the file should contain a pointer and will report it as malformed when it does not. So thesmudgefilter would also need to be aware of the file size threshold and ignore non-pointers below that limit. - I happen to have spent the last week working to deal with the ramifications of the bug fix proposed in PR #6011, and so I know that once it's merged, our
clean()function will rarely know the size of a file in advance. Specifically, thefileSizeparameter is passed as-1duringcleanfilter operations because Git doesn't provide the size of the data it's sending, and although we currently try to work around this by checking the size of the corresponding file in the working tree, this introduces a race condition because the file might change after Git starts streaming it to our filter. Ultimately, we have to remove this check and so only by reading the data arriving from Git would be know if its size exceeds a given threshold. - Specifying the threshold value in our
.lfsconfigfile is, I think, going to be a bit fragile, because it means the overall mechanism depends on both a global-match pattern in.gitattributesand a Git configuration option. Instead, if we were going to try to implement a threshold like this at all, I would want to see if we could specify it directly in the.gitattributesentry as a custom key/value attribute along withfilter=lfs. Of course, this does mean Git LFS would have to read and parse the.gitattributesfile to learn what the current threshold is, if any.
Those are just some quick initial thoughts. Whether or not we pursue the approach taken in this PR, it's definitely worth considering the implications of such a design, so thank you very much for opening the PR and starting the discussion!

This PR adds a --min-size flag to git lfs track and fixes a build failure when no git tags are present.
The new --min-size flag automatically tracks files exceeding a given size threshold: