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

Fix issue where git archive fails on repositories with Git LFS #8186

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

Conversation

chadac
Copy link

@chadac chadac commented Apr 7, 2023

Motivation

Add a flag -c remote.origin.lfsurl=<actualUrl> to git.cc to support LFS calls end-to-end, unblocking issues where fetchGit fails on Git repositories using LFS.

Context

Resolves: #4623

Currently fetchGit is incapable of calling git-lfs. Fortunately, it almost works perfectly as git properly hands off necessary calls to a locally installed git-lfs executable, but there is a minor difference in expectations between what is available in the bare Git repository and what lfs needs. The error message from LFS is not very descriptive.

In particular, git-lfs attempts to discover the remote URL that it needs for fetching artifact URLs using the remote.origin.url Git config option, which is not present in the current bare Git repository.

While I think setting the remote.origin.url might have undesirable side-effects, LFS accepts an alternative URL via the LFS-specific configuration option remote.origin.lfsurl.

Since git.cc uses a bare checkout when pulling from repositories using submodules, Git LFS should work in those cases. It's only for repositories without submodules (i.e. when the archive command is invoked) that this configuration option is necessary. Thus, I added it directly as a configuration option on the archive command rather than updating the repository's config file.

One uncertainty that I have: I'm not exactly sure how easy this will be to test. Git LFS usually operates by making an HTTP request to an asset server and I'm not sure if it's feasible to run a test for LFS without needing a lot of extra infrastructure. It also sounds like such tests may be fundamentally changed with this new libgit approach. I can provide a sample repository with LFS files to manually verify this change, but I don't know if there is an easy way to test LFS going forward.

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

  • agreed on idea
  • agreed on implementation strategy
  • tests, as appropriate
    • functional tests - tests/**.sh
    • unit tests - src/*/tests
    • integration tests - tests/nixos/*
  • documentation in the manual
  • documentation in the internal API docs
  • code and comments are self-explanatory
  • commit message explains why the change was made
  • new feature or incompatible change: updated release notes

Priorities

Add 👍 to pull requests you find important.

@chadac chadac requested a review from edolstra as a code owner April 7, 2023 21:42
@edolstra
Copy link
Member

AFAIK, libgit2 does not have support for LFS, so that makes me reluctant to add LFS support right now.

If we do want to support LFS, it could be tested using the VM infrastructure, see e.g. tests/nixos/github-flakes.nix for a test of the github input type that simulates a GitHub server.

@chadac
Copy link
Author

chadac commented Apr 12, 2023

I think libgit2 does support custom filter hooks, although it would likely need to shell out to git-lfs separately which means custom code eventually. It would definitely still be helpful to our organization, which needs LFS to build a large number of our internal packages.

If y'all are open to adding LFS in, I'd be willing to adding integration tests in with this PR as a POC.

@bouk
Copy link
Contributor

bouk commented Sep 14, 2023

@chadac We could also just set remote.origin.url in the bare repo, even when we don't have submodules. This should fix it too, right?

@chadac
Copy link
Author

chadac commented Oct 2, 2023

@bouk It should be possible setting this as well. I used the more specific in case there were side effects y'all were avoiding. Can run a test to confirm.

@b-camacho
Copy link

This used to work if you have latest master git-lfs, since we changed git-lfs to look for remotes in the FETCH_HEAD file (git archive looks there by default)

Notwithstanding, git-lfs broke again in the git fetcher overhaul (specifically the move to libgit2). I'll try to fix it again though

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.

Support for Git LFS in private repositories
4 participants