Skip to content

Conversation

@daavoo
Copy link
Contributor

@daavoo daavoo commented Feb 27, 2023

Using pygit2 was causing treeverse/dvc#9080

@daavoo daavoo self-assigned this Feb 27, 2023
@daavoo daavoo requested a review from a team February 27, 2023 11:34
@daavoo daavoo marked this pull request as draft February 27, 2023 11:35
@daavoo daavoo removed the request for review from a team February 27, 2023 11:35
@daavoo daavoo marked this pull request as ready for review February 27, 2023 11:45
@daavoo daavoo requested a review from a team February 27, 2023 11:45
get_matching_helper_commands(url, self.dulwich.repo.get_config_stack())
)
parsed = urlparse(url)
ssh = parsed.scheme in ("git", "git+ssh", "ssh") or url.startswith("git@")
Copy link
Contributor

Choose a reason for hiding this comment

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

This should maybe be handled here: https://github.com/iterative/scmrepo/blob/e8de01e07edeacae33a9552a1005152b5a153b2a/src/scmrepo/git/backend/pygit2.py#L463-L464

But this may be a different problem entirely if pygit2+SSH is not working on linux/mac. libgit2/pygit2 are supposed to support SSH everywhere except windows.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the issue is that while it supports SSH, libgit2 doesn't parse openssh configs

https://libgit2.org/docs/guides/authentication/

So we probably need to be using pygit2.RemoteCallbacks and loading the keyfile or user/pass ourselves (we can use asyncssh for this)

Copy link
Contributor

@pmrowla pmrowla left a comment

Choose a reason for hiding this comment

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

This is ok as a workaround until we handle libgit2/pygit2 auth callbacks properly

kwargs["backends"] = ["dulwich"]
if "dulwich" in kwargs.get("backends", self.backends.backends):
credentials_helper = any(
get_matching_helper_commands(url, self.dulwich.repo.get_config_stack())
Copy link
Collaborator

Choose a reason for hiding this comment

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

IIRC credential-helper is not supposed to work for ssh, only for http urls. So this should be fixed in get_matching_helper_commands() or in upstream dulwich.

Regarding ssh not working in pygit2, let's investigate/fix it separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIRC credential-helper is not supposed to work for ssh, only for http urls. So this should be fixed in get_matching_helper_commands() or in upstream dulwich.

Not sure I understand what you mean. The previous logic is preserved but the change is to also force dulwich if the URL is ssh

Copy link
Collaborator

@skshetry skshetry Feb 27, 2023

Choose a reason for hiding this comment

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

get_matching_helper_commands should not match anything for ssh urls, so it should not enter this block.

Please try to address comments before merging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

get_matching_helper_commands should not match anything for ssh urls, so it should not enter this block.

Sorry, I am still not sure I follow. You mean it should be something like:

if ssh or any(get_matching_helper_commands(url, self.dulwich.repo.get_config_stack()):

@daavoo
Copy link
Contributor Author

daavoo commented Feb 27, 2023

This is ok as a workaround until we handle libgit2/pygit2 auth callbacks properly

Opened #197

@daavoo daavoo merged commit dd2ffa9 into main Feb 27, 2023
@daavoo daavoo deleted the dulwich-ssh branch February 27, 2023 15:40
@ReveStobinson
Copy link

Just manually installed the 0.1.13 release with dvc=2.45.1, and these changes appear to resolve treeverse/dvc#9080 for me! Thank you, all, for the quick response on this!

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.

4 participants