add support for git credential helpers

[dtrifiro]

• add CredentialHelper class to run credential helper commands and retrieve values
• add get_credentials_from_helper to retrieve credentials from helpers defined in provided config
• moved (unused) get_credentials_from_store to credentials.py, refactor it to use CredentialHelper with store helper.

Also see https://www.git-scm.com/docs/gitcredentials

fixes #873

[codecov-commenter]

Codecov Report

Merging #976 (ba0fa0c) into master (ba0fa0c) will not change coverage. The diff coverage is n/a.

:exclamation: Current head ba0fa0c differs from pull request most recent head 452bc9a. Consider uploading reports for the commit 452bc9a to get more accurate results

@@           Coverage Diff           @@
##           master     #976   +/-   ##
=======================================
  Coverage   85.05%   85.05%           
=======================================
  Files          93       93           
  Lines       22901    22901           
  Branches     3359     3359           
=======================================
  Hits        19479    19479           
  Misses       2957     2957           
  Partials      465      465           

:mega: Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

[dtrifiro]

Would appreciate if you could take a look @pmrowla

[dtrifiro]

Added some more tests with the latest push

[dtrifiro]

Hey @jelmer, this is ready for review btw 🙂

[jelmer]

Sorry for the delay, I'll try to have another look at it this week.

[dtrifiro]

Temporarily marking this as draft while I work on a patch for a bug in this implementation (as well as making this more closely match cli git's behaviour)

[dtrifiro]

While fixing a bug, I spent some time doing cleanups/refactors. I dropped GitCredentialsHttpClient and moved git credentials management in Urllib3HttpGitClient, as well as getting rid of of the retry logic and add some url-matching functions to more closely match cli git behaviour. Overall I feel like the code is a bit cleaner and behavior more predictable and consistent with cli git.

[bakaleks]

Hi @jelmer I would really appreciate if this PR gets merged. Thanks.

[d-miketa]

Same, without it I can't reach private Bitbucket repos. :(

[jelmer]

Sorry, I'm currently traveling but will be able to take a look in a couple of days.

[oslobowl]

Hi guys :) Do you know when this branch will be merged, approximately? Thanks a lot :)

[dtrifiro]

Sorry for sitting on this for so long

Hi @jelmer, thanks for taking the time to review this :slightly_smiling_face:

I'm a bit concerned about how this makes the interface for authentication more complicated. Ideally there should just be one pythonic interface for authentication, with perhaps one implementation of that that can interact with C git.

I'm not sure I understand what you're proposing here, but below you'll find my thoughts. Please let me know if I misunderstood and/or if there's any other parts you find complicated you can point me to, so I can have another look and take action.

With my last push I streamlined get_credentials_from_helpers to explicitly how the configs sections and values are being used. The only other method to get credentials is get_credentials_from_store which, as far as I can tell, is unused. We could either refactor this or get rid of it (as I proposed in another comment).

[dtrifiro]

Hey @jelmer, just a friendly ping: this is ready for review 🙂

[jelmer]

sorry about that, looking at it again now

[jelmer]

sorry about that, looking at it again now

Haven't forgotten about this, will probably put forward a PR with a subset of your changes later this week.

[jelmer]

Sorry again for the long delay. As a means of trying to move this forward, I've at least cherry-pick the config-related bits of this PR that are non-controversial.

[efiop]

Hey @jelmer , thanks for taking a look! 🙏 FYI: @dtrifiro broke his clavicle/collarbone and won't be able to reply for a few weeks. If there is anything he needs to address, he'll get back to it once he's recovered.

[jelmer]

Hey @jelmer , thanks for taking a look! 🙏 FYI: @dtrifiro broke his clavicle/collarbone and won't be able to reply for a few weeks. If there is anything he needs to address, he'll get back to it once he's recovered.

Ah, thanks - good to know. I hope his recovery goes well!

[dtrifiro]

Hey @jelmer, sorry for keeping this hanging so long.

I just rebased and added a new commit to update the get_credentials_from_store to use CredentialHelper.

If you have any other concerns and/or suggestions that would help this get merged, I'd greatly appreciate that.

Thanks!