[arthurschreiber]

This revamps the Rugged::Remote implementation that we currently have.

It removes the low-level #connect, #connected?, #disconnect and #update_tips methods in favor of "automatic" connection handling, which should be less error-prone and easier to use.

Remote#ls was modified to reflect this change.

Additionally, Remote#fetch was implemented with the same credentials support as Repository#clone_at, reusing most of the code that was there.

As part of the changes, support for the :progress and :update_tips callbacks was added to Repository#clone_at, and the options Hash structure was tweaked a bit.

Remote#push is still WIP, but should be added soon.

---

I'm not a 100% sure about the API - maybe #fetch and #push should be on the Repository instead of Remote? Or what about adding Repository#fetch and Repository#push convenience methods that take either a Remote or a url (and just forward to Remote#push/Remote#fetch)?

[carlosmn]

:transfer_progress

[carlosmn]

Looks pretty good!

I think that a Repository#fetch function would be useful, you can pass the name and optionally override the refspecs if you want (which wouldn't be as easy with Remote#fetch, as you would then have to restore the old ones

[carlosmn]

A note about #ls: the results from the latest connection (as long as the transport hasn't disappeared) are cached, so you don't necessarily need to create a brand new connection every time.

[arthurschreiber]

As soon as #307 gets merged, I'll probably rework parts of this to feature a similar API.

[albertsun]

Is this still the pull request that will implement push/pull support over ssh?

[arthurschreiber]

Yes. Now that #307 has landed, I'll spend some time over the next days to rework this to feature a similar API.

[arthurschreiber]

@vmg @cmn Can you take a look after the recent changes I pushed?

This PR has grown quite in size and scope, and I'm not sure how to feel about that. I could break this up into a bunch of smaller PRs that are more focused and would be easier to review.

Right now, this PR contains:

• Addition of Rugged::Credentials::SshKeyFromAgent.
• Refactoring of the existing credentials code.
• An example file showing how to emulate git-fetch.
• Dropping low-level remote actions like #connect and #disconnect.
• Addition of Remote#fetch with credentials support.
• Normalization of the options hash between Remote#fetch and Repository#clone_at.
• Move of Repository#push to Remote#push, addition of credentials support.

[arthurschreiber]

A note about #ls: the results from the latest connection (as long as the transport hasn't disappeared) are cached, so you don't necessarily need to create a brand new connection every time.

@cmn I think I still do. The problem is that I can't leave the connection open because I don't know which direction it was opened in (or I'd have to check that and then handle that correctly by reopening the connection). Or am I wrong?

[carlosmn]

If you're not exposing git_remote_download directly, I do agree that reconnecting is the simplest option and it would help with freeing the connection as well as getting a more up-to-date version of the remote refs.

[carlosmn]

Copy-paste error?

[carlosmn]

I wonder if someone might actually want access to the individual steps of a fetch or push, but that can be added whenever; it's good that there's a larger wrapper around it.

I'm not a fan of calling the user-pass cred simply Plaintext as we should be able to have other user-pass combinations that aren't necessarily just plaintext sent over an encrypted connection, and it doesn't quite give the impression that what you have there is simply a user/password combination.

[arthurschreiber]

I wonder if someone might actually want access to the individual steps of a fetch or push, but that can be added whenever; it's good that there's a larger wrapper around it.

Mmh. Now that we have git_remote_dup, I could actually keep the low-level methods like #connect, #download and so one, while always just duping the remote for a call to the higher level methods like #push and #fetch. Is that a good compromise?

I'm not a fan of calling the user-pass cred simply Plaintext

What about Rugged::Credentials::Password?

[carlosmn]

Yeah, keeping low-level commands and duping for everything else should work, if anybody actually wants to us it. I'd wait until someone actually does, though. No sense in trying to add it back in now.

Password sounds ok, I would have named it UserPassword but it's not really that big a difference.

[albertsun]

Just realized that the little pull request I sent #338 conflicts with the larger changes here.

This code is still all very unfamiliar to me, but is there anything I can do to help with this one?

[albertsun]

I've added a branch here https://github.com/albertsun/rugged/tree/merge-304 that has the commits from here and the commits from #338 merged together along with a test_clone_over_ssh_with_credentials_and_branch test.

[arthurschreiber]

@albertsun I'd rather you finish up the work on #338, and I can merge this seperately. This PR here needs some restructuring anyway, so you shouldn't have bothered. 😄

[antlypls]

Are there any plans to merge this PR soon?
I'd like to implement ls via ssh functionality based on these changes.

[bachue]

I'm also waiting for its merge. It's quite difficult for me to resolve conflicts each time when other commits merged

[arthurschreiber]

I reworked the whole patchset a bit, and cleaned up the code and commit history.

Remote#fetch, Remote#push and Remote#ls now all support credentials (or credential callbacks) through the options hash.

I played around with an alternative API that would've exposed the callbacks through properties on the Remote objects, but I was not really happy with that, because it made the implementation considerably more complex, and because it felt weird to have the Remote API work differently than all other parts of Rugged.

Overall, this should be ready to get merged soon.

[carlosmn]

AFAICT Remote#ls does not look into or use the options to set the credentials callback.

I've a couple of patches on top on this branch in my fork (under the same name), though the changes I made to libgit2 for accessing the remote's data doesn't get used, as you set the credentials on a cloned remote and the data in on the stack anyway.

What I tried to do was remove complexity and decisions from the code. So all the credentials get set in the remote, but it's decided during the callback whether to call it. This by itself doesn't help all that much, but this did lead me straight into the next change, which is to have a macro to check whether the callbacks are callable, which reduces the amount of code one has to look through, and makes the differences between the four if blocks clearer (IMO).

[arthurschreiber]

I like the change in carlosmn@0d14bc7, while I think the change in carlosmn@542d4c2 doesn't really help much.

AFAICT Remote#ls does not look into or use the options to set the credentials callback.

Yeah, you're right, I must haveve missed this somehow. I just pushed the missing functionality and some test cases.

[carlosmn]

That first commit removes changing code form the if-block. If we don't move it out of there, we need to make the macro more complex and somewhat uglier.

[arthurschreiber]

@carlosmn I cherry picked your changes, thank you very much! ❤️

I think the last tiny thing missing is support for the git_push transfer_progress and pack_progress callbacks, but this should not block this PR from getting merged.

[carlosmn]

Agreed. Those would be nice to have, but they can be added later.

We would need for my PR to libgit2 to get merged before, as we depend on the unmerged PASSTHROUGH behaviour.

[arthurschreiber]

@vmg Is there anything left to do before we can get this merged?

[arthurschreiber]

@vmg Done.

[bachue]

Great