Comments (8)
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.
from gitoxide.
Progress
- I need to pass Extra Parameters in the header
- extra parameters can now be passed as
handshake()
parameter on the transport layer and theDelegate
on the protocol layer has a newhandshake_extra_parameters()
method to do the same, default implemented to return no extra parameters at all.
- extra parameters can now be passed as
- 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.
- git-transports can specify supported protocol versions. In case of the
- 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 acustom_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 ofcustom_url(…)
was proven to feel 'right' :D.
- The
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.
from gitoxide.
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.
from gitoxide.
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
.
from gitoxide.
from gitoxide.
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?
from gitoxide.
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.
from gitoxide.
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.
from gitoxide.
Related Issues (20)
- building on windows fails due to symlinks in working tree HOT 9
- How to track gitoxide releases HOT 1
- Virtualised gix fails with No such device because of MAP_SHARED mmap. HOT 7
- Oxidize `gitu`
- Oxidize Radicle-Git HOT 7
- Oxidize comtrya HOT 1
- Oxidize `comtrya` HOT 4
- another parsing failure of malformed author/committer timestamp HOT 2
- gix-transport 0.41.3 has issues HOT 6
- unread field warnings on nightly HOT 1
- Availability: crates.io vs releases HOT 5
- gix_submodule::File::from_bytes is a catch22 HOT 4
- some way to refresh in-memory packed refs cache without relying on mtime HOT 2
- Panic receiving pack if fetch interrupted HOT 2
- `gix clone` sets `core.symlinks` to `false` on Windows even if globally `true` HOT 1
- Checking out a dangling symlink on Windows is treated as a hard error HOT 3
- CI install-action now fails on Windows, can't find .cargo/bin
- 16 tests fail on Windows with GIX_TEST_IGNORE_ARCHIVES=1
- Tests on Windows require Git Bash or a similar environment HOT 1
- Assertion failure crash in `gix_date::time::write::<impl gix_date::Time>::write_to` HOT 3
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from gitoxide.