Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Git LFS support to uv-git crate #10335

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

sydduckworth
Copy link

@sydduckworth sydduckworth commented Jan 6, 2025

Summary

Closes #3312.

This PR adds Git LFS support to the uv-git crate by using the git-lfs CLI to fetch required LFS objects for a revision following the call to git fetch.

The LFS fetch step is run for all repositories regardless of whether they have associated LFS objects. The step is skipped if the git-lfs CLI tool isn't installed.

As described in the linked issue, this may introduce overhead in the case where a user has Git LFS installed and is trying to clone a very large repository that doesn't use LFS, but my guess is that it shouldn't be an issue.

Test Plan

I verified that the minimal example in the linked issue passes, i.e. this command now succeeds:

uv pip install git+https://github.com/grebnetiew/lfs-py.git

I also verified that non-LFS repositories still work, with or without git-lfs installed.

Potential Changes / Improvements

With this change LFS objects in a given revision will always be downloaded if the user has Git LFS installed, which may not always be desired behavior. It might be helpful to add a field to the uv settings and/or an environment variable so that the LFS step can be disabled if needed.

- Added a `git lfs fetch` step after fetching the repository but before cloning it locally.
- Git LFS step is ignored if `git-lfs` is not installed.
- Errors while executing Git LFS are reported as normal.
/// We search for the Git LFS binary instead of using `git lfs` so that we can
/// distinguish Git LFS not being installed (which we can ignore) from
/// LFS errors (which should be returned).
static GIT_LFS: LazyLock<Option<PathBuf>> = LazyLock::new(|| which::which("git-lfs").ok());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not certain this detection method is portable to all end user systems. For example, I can have git lfs installed and working but I not have git-lfs binary on the path. I think probing git lfs directly is likely the most portable approach.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I've updated the logic so now instead of searching for git-lfs on the path, it just tries to run git lfs version the first time LFS is invoked and uses the success of that command as an indicator of LFS availability.

I'd definitely be interested in if anyone has a cleaner way of determining whether LFS is available. I was initially going to just check if the LFS fetch command returned 127 (command not found), but that return code isn't available in Powershell, so that approach isn't portable across shells.

Now instead of checking for a `git-lfs` binary on the path, we just try to run `git lfs version` to verify LFS is available.
@sydduckworth sydduckworth requested a review from samypr100 January 9, 2025 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

uv doesn't correctly checkout Git dependencies with Git LFS assets
2 participants