Skip to content

Conversation

@efiop
Copy link
Contributor

@efiop efiop commented Mar 17, 2023

clone/push/pull/fetch/push_refspecs/fetch_refspecs all rely on auth heavily and git CLI, unfortunately, handles it the best across the board. So we will prefer using it for those operations but if CLI git is not available - we'll fall back to dulwich/pygit2.

Note that push/fetch_refspecs are not implemented in gitpython backend yet, so this won't work for them until those are implemented.

Fixes #208

clone/push/pull/fetch/push_refspecs/fetch_refspecs all rely on auth heavily
and git CLI, unfortunately, handles it the best across the board.

Note that push/fetch_refspecs are not implemented in gitpython backend yet,
so this won't work for them until those are implemented.

Fixes #208
@efiop efiop requested review from pmrowla and skshetry March 17, 2023 00:30
@codecov-commenter
Copy link

Codecov Report

Patch coverage: 90.00% and project coverage change: -1.46 ⚠️

Comparison is base (18dae0f) 82.29% compared to head (b0467ba) 80.84%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #210      +/-   ##
==========================================
- Coverage   82.29%   80.84%   -1.46%     
==========================================
  Files          25       25              
  Lines        3497     3493       -4     
  Branches      608      607       -1     
==========================================
- Hits         2878     2824      -54     
- Misses        527      568      +41     
- Partials       92      101       +9     
Impacted Files Coverage Δ
src/scmrepo/git/__init__.py 84.74% <90.00%> (+0.44%) ⬆️

... and 8 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@efiop
Copy link
Contributor Author

efiop commented Mar 17, 2023

@pmrowla @skshetry Folks, really need your opinion and review here. 🙏

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.

I would still prefer that we just spend the time fixing these auth problems in pygit/dulwich instead of re-introducing default use of gitpython

@efiop
Copy link
Contributor Author

efiop commented Mar 18, 2023

@pmrowla I agree with you. Do you think it is doable short term? I could jump into it. Things like treeverse/dvc-ssh#20 look concerning though.

Really hate to prefer gitpython for these again, but the work you've done is not wasted, as it still makes dvc work without CLI git and overall scmrepo is very useful on itsown.

@pmrowla
Copy link
Contributor

pmrowla commented Mar 21, 2023

Porting the apple openssh to asyncssh is non-trivial and probably not what we want. What we should be doing in dulwich is checking for unsupported configuration options in our asyncssh vendor (like UseKeychain which specifies using the apple keychain behavior) and then fallback to the default dulwich SSH vendor instead (which just calls system ssh via subprocess, and would use apple's openssh build and work as expected).

We default to using asyncssh since it removes a dependency on ssh, which is not always available in windows (or in containers)

@pmrowla
Copy link
Contributor

pmrowla commented Mar 21, 2023

The other issues are mostly about implementing auth callbacks for pygit2, which should be straightforward (the callback support already exists in pygit, we just have to write the functions to pass the appropriate ssh key/http username+pw/etc back into pygit)

Copy link
Collaborator

@skshetry skshetry left a comment

Choose a reason for hiding this comment

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

We should implement push_refspecs/fetch_refspecs in GitPython too if we make this change. Using a different backend for cloning and a different backend for push_refspecs will be painful (which it already is). I'd prefer to have this change together.

@efiop
Copy link
Contributor Author

efiop commented Mar 21, 2023

@pmrowla Sounds reasonable. Let's discuss during planning today.

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.

clone: use gitpython's CLI git first and fallback to dulwich if CLI git is not available

4 participants