client::Transport impl

[kim]

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]

Thanks so much for sharing, here is my thoughts.

I need to pass Extra Parameters in the header

Great catch, I also think that should be possible. This should be an adequate fix:

    fn handshake(&mut self, service: Service, params: impl Iterator<Item = &'a str>) -> Result<SetServiceResponse<'_>, Error>;

I want to abort the handshake if the server doesn’t support v2

If you operate on transport level you would be driving it and thus can already abort at will by closing the connection. Thus I assume this should be possible when using the Delegate for the fetch protocol. In that case, the Delegate already has a prepare_fetch(…) method which can abort the operation early as it will see the actual protocol version. It can also know that V2 is used as in that case, prepare_ls_refs(…) is called before.

When using git-protocol and the Delegate the only disadvantage I see right now is that the fetch implementation will always read and parse all V1 refs before giving the delegate the chance to abort. Is this something you want to do?

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

I would love to allow git::Connection to be used by everyone implementing custom transports as well as it's already bringing some standard logic that everyone would need while staying compatible to git.
Thus I suggest to make the responses configurable upon instantiation. What about this?

        Ok(git::Connection::new(
            read,
            write,
            desired_version,
            path,
            None::<(String, _)>,
            git::ConnectMode::Daemon,
        )
        .set_stateful(false)
        .set_url("foobar"))

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

If you think my suggestions could work I am happy to implement them, these are a quick-fixes to me. I couldn't find the code you were writing but seeing it might help to make even better APIs :).

[kim]

the fetch implementation will always read and parse all V1 refs before giving the delegate the chance to abort

Yes that’s the thing. My fetches are non-interactive, the remote is not trusted, and the repository layout is such that there can legitimately be thousands of refs. So I want to be a bit paranoid and at least discard all packets should the other end downgrade without being asked to.

Perhaps desired_version could instead be supported_versions() -> &[Version]? That’d be defining a version negotiation protocol where git doesn’t have one, so a bit iffy to present as a stable interface. So maybe that’s indeed specific to radicle-link, or should be brought up on the git mailing list.

Thus I suggest to make the responses configurable upon instantiation.

Yes that looks like it would cut it for me (modulo the version negotiation).

Can you point me to where the url and statefulness are actually used?

I couldn't find the code

Yeah I’m still using path dependencies, so haven’t pushed.

[Byron]

Perhaps desired_version could instead be supported_versions() -> &[Version]? That’d be defining a version negotiation protocol where git doesn’t have one, so a bit iffy to present as a stable interface. So maybe that’s indeed specific to radicle-link, or should be brought up on the git mailing list.

I see, and agree that not being forced to fetch possibly a million refs via V1 is in the benefit of a client who wants to stay safe. Ultimately timeouts would always have to be implemented but that isn't done in the current protocol implementation either.

To me it sounds reasonable to add a supported_versions() method as suggested above and have the protocol layer validate it before consuming V1 refs or proceeding to ls_refs in the case of V2 closing the connection early if there is a mismatch.

Can you point me to where the url and statefulness are actually used?

It's used to support V1 which is stateful in all the cases except when it's used in the HTTP transport. That controls how Arguments are used. This might be an opportunity for improved documentation to let users know it's really just for this case :D. Maybe the daemon won't even care if it receives arguments multiple lines if we assume statelessness for all protocols, I will give it a try.