Avoid needlessly spawning SSH connections with git archive#5309
Merged
bk2204 merged 2 commits intogit-lfs:mainfrom Mar 28, 2023
Merged
Avoid needlessly spawning SSH connections with git archive#5309bk2204 merged 2 commits intogit-lfs:mainfrom
git archive#5309bk2204 merged 2 commits intogit-lfs:mainfrom
Conversation
fe989b5 to
863994d
Compare
Right now, any time we instantiate a Manifest object, we create an API client, and when we create the API client, if we're using SSH, we try to make a connection to the server. However, we often instantiate a Manifest object when performing various functionality such as smudging data, which means that when a user creates an archive locally, they can be prompted for an SSH password, which is undesirable. Let's take a first step to fixing this by making Manifest an interface. Right now, it has one concrete version, a concreteManifest, which can be used to access the internals, and we provide methods to upgrade it from the interface to the concrete type and determine whether it's upgraded or not. We attempt to upgrade it any time we need to access its internals. In the future, we'll also offer a lazyManifest, which is lazy and will only instantiate the concreteManifest inside when we attempt to upgrade it to the latter. But for now, only implement the concreteManifest to make it clearer what's changing. Similarly, we make our TransferQueue upgradable so that we don't upgrade its Manifest right away. In both cases, we'll want to use the lazyManifest to delay the instantiation of the API client (and hence the starting of the SSH connection) in a future commit.
863994d to
4d8036d
Compare
chrisd8088
approved these changes
Mar 25, 2023
When a user invokes `git archive` with LFS files, `git lfs filter-process` is invoked to smudge the LFS files. However, currently when we instantiate the manifest object as part of that, an attempt is made to connect to the remote using SSH, which we don't want to do unless necessary. For example, if the user already has all the files locally, the network connection is needless and serves only to waste resources. In the previous commit, we made our manifest an abstract interface with a single implementing type: a concrete manifest. Now, introduce a lazy manifest, which can upgrade to a concrete manifest but doesn't instantiate one until that happens. This allows us to instantiate a manifest without making the SSH connection, and we can delay the SSH connection until it's really needed, if at all. Add a test for this case as well.
4d8036d to
a0065c0
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When a user invokes
git archivewith LFS files,git lfs filter-processis invoked to smudge the LFS files. However, currently when we instantiate the manifest object as part of that, an attempt is made to connect to the remote using SSH, which we don't want to do unless necessary. For example, if the user already has all the files locally, the network connection is needless and serves only to waste resources.Adjust our manifest types to introduce a lazy manifest that only spawns the SSH connection on demand. Further description is available in the commit messages.
Fixes #5307