Skip to content

Conversation

@bk2204
Copy link
Member

@bk2204 bk2204 commented Mar 10, 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.

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

@bk2204 bk2204 force-pushed the ssh-transfer-manifest branch from fe989b5 to 863994d Compare March 10, 2023 16:58
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.
@bk2204 bk2204 force-pushed the ssh-transfer-manifest branch from 863994d to 4d8036d Compare March 23, 2023 16:56
@bk2204 bk2204 marked this pull request as ready for review March 23, 2023 17:21
@bk2204 bk2204 requested a review from a team as a code owner March 23, 2023 17:21
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.
@bk2204 bk2204 force-pushed the ssh-transfer-manifest branch from 4d8036d to a0065c0 Compare March 28, 2023 15:22
@bk2204 bk2204 merged commit 8434ced into git-lfs:main Mar 28, 2023
@bk2204 bk2204 deleted the ssh-transfer-manifest branch March 28, 2023 16:57
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.

git archive should not ask for password to ssh key

2 participants