-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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 support for sparse checkout #1317
Conversation
… to have the test scripts
…ult test behaviour
Same comment as I posted on #680: Shouldn't lfs.fetchinclude also be set or use "-I" (or "--include") parameter in GitCommandManager.lfsFetch to make sure LFS only fetches from included paths? |
1c46459
to
eea6d0e
Compare
Regarding this topic and after taking a look and making some test using "--include" option directly when making the
In order to have working
Let me know what you think about this! But my personal opinion is that the second solution is the best one. |
Disclaimer: I am not using Git LFS myself. Having said that, I guess that would allow Git LFS to batch-fetch, and the first option maybe would not? If so, I am definitely in favor of the second option. Even if both allow batch-fetching, the second option looks slightly cleaner to me. Probably something like this? diff --git a/__test__/git-auth-helper.test.ts b/__test__/git-auth-helper.test.ts
index fec6573..fa2d1db 100644
--- a/__test__/git-auth-helper.test.ts
+++ b/__test__/git-auth-helper.test.ts
@@ -759,6 +759,7 @@ async function setup(testName: string): Promise<void> {
init: jest.fn(),
isDetached: jest.fn(),
lfsFetch: jest.fn(),
+ lfsPull: jest.fn(),
lfsInstall: jest.fn(),
log1: jest.fn(),
remoteAdd: jest.fn(),
diff --git a/__test__/git-directory-helper.test.ts b/__test__/git-directory-helper.test.ts
index 362133f..9d9d8bd 100644
--- a/__test__/git-directory-helper.test.ts
+++ b/__test__/git-directory-helper.test.ts
@@ -474,6 +474,7 @@ async function setup(testName: string): Promise<void> {
init: jest.fn(),
isDetached: jest.fn(),
lfsFetch: jest.fn(),
+ lfsPull: jest.fn(),
lfsInstall: jest.fn(),
log1: jest.fn(),
remoteAdd: jest.fn(),
diff --git a/src/git-command-manager.ts b/src/git-command-manager.ts
index 4f6dc79..89b3a44 100644
--- a/src/git-command-manager.ts
+++ b/src/git-command-manager.ts
@@ -40,6 +40,7 @@ export interface IGitCommandManager {
init(): Promise<void>
isDetached(): Promise<boolean>
lfsFetch(ref: string): Promise<void>
+ lfsPull(): Promise<void>
lfsInstall(): Promise<void>
log1(format?: string): Promise<string>
remoteAdd(remoteName: string, remoteUrl: string): Promise<void>
@@ -328,6 +329,15 @@ class GitCommandManager {
})
}
+ async lfsPull(): Promise<void> {
+ const args = ['lfs', 'pull', '--include']
+
+ const that = this
+ await retryHelper.execute(async () => {
+ await that.execGit(args)
+ })
+ }
+
async lfsInstall(): Promise<void> {
await this.execGit(['lfs', 'install', '--local'])
}
diff --git a/src/git-source-provider.ts b/src/git-source-provider.ts
index 967097d..e8e3e44 100644
--- a/src/git-source-provider.ts
+++ b/src/git-source-provider.ts
@@ -188,7 +188,8 @@ export async function getSource(settings: IGitSourceSettings): Promise<void> {
// LFS fetch
// Explicit lfs-fetch to avoid slow checkout (fetches one lfs object at a time).
// Explicit lfs fetch will fetch lfs objects in parallel.
- if (settings.lfs) {
+ // For sparse checkouts, wait until after the checkout is done.
+ if (settings.lfs && !settings.sparseCheckout) {
core.startGroup('Fetching LFS objects')
await git.lfsFetch(checkoutInfo.startPoint || checkoutInfo.ref)
core.endGroup()
@@ -210,6 +211,13 @@ export async function getSource(settings: IGitSourceSettings): Promise<void> {
await git.checkout(checkoutInfo.ref, checkoutInfo.startPoint)
core.endGroup()
+ // Sparse checkout only: delayed LFS pull
+ if (settings.lfs && settings.sparseCheckout) {
+ core.startGroup('Pulling LFS objects')
+ await git.lfsPull()
+ core.endGroup()
+ }
+
// Submodules
if (settings.submodules) {
// Temporarily override global config |
Hmm. I cannot get that |
@dscho the "--include" pattern should work the same as the |
I am not sure that that's true. The manual page says that So yes, I think we need to use option 1. I pushed my proposed solution here: 25d6c12 |
You are right! Therefore better use option 1 🚀
That seems to be good the only thing that you might be missing is to don't set |
I'm confused... The |
Yo are right I got confused here 😅 All good then, I don't know why I thought it was in the other way! So for me 25d6c12 ✅ |
@dfdez @dscho Are you guys sure "git lfs -I" does not support patterns? This document seems to suggest it does: https://manpages.debian.org/testing/git-lfs/git-lfs-fetch.1.en.html. I also searched git-lfs repo and found this issue: git-lfs/git-lfs#912, which clearly suggests that patterns should be supported. If possible, we should use "lfs pull" for better performance. I don't use sparse checkout in dev environment, but our Jenkins build does, so I took a look at the build job log, and could confirm that it used lfs.fetchinlcude config and "lfs pull" to do so:
Note that somehow the log didn't show how it performed sparse checkout, but I confirmed that it used .git/info/sparse-checkout file to specify the sparse checkout path. Though, it doesn't matter since "git sparse-checkout" should be doing the same. |
The manual page talks about
With all of that, I actually really do not want to use this option.
Are you sure about that? From what I gather reading https://git-scm.com/docs/gitattributes#_delay, I could imagine that it works as intended (i.e. using batched LFS fetching) even when using sparse checkouts. As I mentioned earlier, I am by no means familiar with LFS. I don't have access to any meaningful repository using LFS. Maybe you do, and maybe you can test whether letting |
Seems you are right. I don't have the env at home so I didn't test but after some more digging, it appears that "git lfs pull has better performance" was just an old myth. According to git-lfs/git-lfs#2594 (comment) it shouldn't be a problem for Git anymore after Git 2.15. So your 25d6c12 should be the right solution. Thanks. |
After some digging I found out this on the git-lfs docs, but as @blu3mania menthioned git-lfs/git-lfs#2594 (comment) clarify everything! Therefore we can close the topic regarding |
@dfdez this was for the most part your work, culminating in https://github.com/actions/checkout/releases/tag/v3.5.3. Thank you so much! If I was able to assist, I am glad that I could. |
Yeah that's true and I would have appreciated a small mention in the release, @TingluoHuang I don't know if that is possible to add it, but I would really appreciate it! But the important thing is to have the functionality! Thanks for everything |
@dfdez you're absolutely correct, your contribution must be celebrated. Look at the latest revision of the release notes of https://github.com/actions/checkout/releases/tag/v3.5.3 😃 |
Thanks! 🚀 |
Context:
Monorepo sometimes can be challenging mainly when having a huge base of code. A lot of people have encounter situation where they don't need to fetch everything inside the repository, with this new option anyone will be able to make
sparse-checkout
inside github action quite simple!Features:
sparse-checkout
optionsparse-checkout
README.md
withsparse-checkout
option and examplesUsage:
Fetch only the root files
Fetch only the root files and
.github
andsrc
folderImplementation
In order to make
sparase-checkout
much useful I have added to thegit fetch
the possibility to add a filter in order to use--filter='blob:none'
.With this update we will make sure that when the fetch is done no blob of the repository will be downloaded (this filter will be done only when
sparse-checkout
is set), instead the blobs will be downloaded when making thegit checkout
after having thesparse-checkout
configured.Here you can find the article that I have used to guide me to make the implementation.
Related Issues