Make git::Connection so versatile that it can fit more use-cases

[Byron]

Discussed in #110

^{Originally posted by kim June 25, 2021}
It turns out that I needed to define my own Transport impl, because:

• I need to pass Extra Parameters in the header
• I want to abort the handshake if the server doesn’t support v2
• I wasn’t sure how the to_url and is_stateful methods are used, but morally they need to return different things than the default Connection for my case

To do that, I needed to make the capabilities parsing public. While that is probably always needed for custom transports, I’m wondering if the interface could be generalised such that more use cases can just reuse a parametric Connection. For example, the delegate pattern could be employed for the handshake, and extra parameters could be stored in Connection.

I’d be happy to propose a patch, but wanted to gauge first if there’s interest, or maybe different plans already.

[Byron]

Progress

• I need to pass Extra Parameters in the header
  • extra parameters can now be passed as handshake() parameter on the transport layer and the Delegate on the protocol layer has a new handshake_extra_parameters() method to do the same, default implemented to return no extra parameters at all.
• I want to abort the handshake if the server doesn’t support v2
  • git-transports can specify supported protocol versions. In case of the git::Connection this is implemented so that V1 can always be upgraded to any other protocol version, but if anything else is requested, only the given version is supported. Essentially it's an 'upgrade-only' policy. If the version requirement is not met the operation will now return with a clearly identifiable error right after the handshake without closing the connection or parsing the refs. As an interesting chunk of information I may add that I tried to close the connection but am unable to due to the borrowchecker not letting me. Within that construct I already worked around a borrow-check issue but couldn't find a way to work around this one.
• I wasn’t sure how the to_url and is_stateful methods are used, but morally they need to return different things than the default Connection for my case
  • The git::Connection now has a custom_url(Some(url)) builder kind of method which allows to easily override the default url that would otherwise be queried. It's only relevant for authenticated transports as its passed to the respective authentication helper. is_stateful() is not overridable just yet but isn't important for V2 onward either. It's something that can be added for completeness I suppose once the style of custom_url(…) was proven to feel 'right' :D.

All the changes above should make it possible to use the standard git::Connection in the transport layer, please let me know what else might be missing because I'd really want that one to be the one-stop-shop for custom transports in the vast majority of cases at least.

Looking forward to your feedback.

[kim]

This all looks good, will give it a spin shortly!

As an interesting chunk of information I may add that I tried to close the connection but am unable to due to the borrowchecker not letting me.

I'm not sure what you mean by "close" -- there isn't currently much choice but to pass the Transport by value to the fetch function. If it returns an Err result, the transport is dropped, which I suppose is equivalent to closing the connection for most implementations. I could imagine that some users would actually want to re-use the connection regardless, or reset it with a custom error code (which obviously is specific to the underlying Async{Read,Write}). I think this may require an impl Transport for &mut Connection, so the caller can retain ownership.

[Byron]

there isn't currently much choice but to pass the Transport by value to the fetch function. If it returns an Err result, the transport is dropped, which I suppose is equivalent to closing the connection for most implementations.

I would expect the connection to be closed on drop as well, right now it's explicitly something that the implementation can and should do when the delegate demands it to be closed, also resulting in the whole operation to stop. There is no other reason for that than me trying to be explicit.

Now that I have this borrowchecker issue, again, that prevents me from calling this method maybe that's the hint to remove the close() method from Transport and document the expected behaviour somewhere.

Once Transport is implemented for &mut Connection/T: Transport it would of course be nice if the connection would always be in the correct state to prevent misuse. In the latter case, the caller would have to assure the connection is not used anymore or closed explicitly once fetch returned an error.

Let me see what I can do to improve that.

[Byron]

Great news: Transport::close() was actually a misnomer, as it really wanted to indicate that no further requests are going to made and there is no pack to be sent. This of course is better handled by fetch(…) itself, exactly in one place.

This made obvious that Action::Close rather wants to be Action::Cancel.

[kim]

Transport::close() was actually a misnomer

Are you sure though that the inner writer doesn't need to be flushed?

[Byron]

A valid point! Previously flushing wasn't done consistently which didn't change. On the bright side, the underlying implementation is always driven by a packet line writer which could be taught to flush on drop for good measure.

My intuition is to wait-and-see if that's necessary, thus far things seemed to have worked as expected, and extra flush calls aren't free. However, it might not be me having to hunt down some weird flush related bug so I would also be OK to simply not take the risk and add a drop impl there.

What's your thoughts?

[kim]

Relying on Drop seems a bit hairy in the async variants, unless you can keep track of a waker (and even then...). Maybe better to leave it to the caller.

[Byron]

True that!

I will be most grateful for learning from your experience while you are making it, gitoxide should just work without footguns even on plumbing level.

For now it seems to work, but if anything starts getting fishy I will make all efforts to put flushes in the right place, here maybe for blocking.