git-repository: feature flag usage

[FintanH]

Following the comment here[0], I was attempting to use git-repository for the one-stop-shop of import in link-git-protocol.

The first thing I noticed was that we were using fetch from the git-protocol crate, but this is only available if #[cfg(any(feature = "blocking-client", feature = "async-client"))]. git-repository doesn't use any of those feature flags, and I'm not sure how one might conditionally pass those down.

The second thing I noticed is that there are some feature flags mentioned, such as ["git-protocol"][1], however, none of them are listed in the features section[3].

[Byron]

The first thing I noticed was that we were using fetch from the git-protocol crate, but this is only available if #[cfg(any(feature = "blocking-client", feature = "async-client"))]. git-repository doesn't use any of those feature flags, and I'm not sure how one might conditionally pass those down.

The idea here is that the consuming applications makes that decision. Probably it's odd there is no flags to pass down, right now the idea to pull in the crate to configure its feature flags, relying on cargo's compile model to add these flags for you transitively.

https://github.com/Byron/gitoxide/blob/5ae584c65cc6e9ad306f077abb609159a15c6375/gitoxide-core/Cargo.toml#L31

Maybe that's just me trying not to have to transitively make every feature toggle from low-level crates available in the ones higher up to make them configurable, but in case of git-repository I see how it would be beneficial to have configuration flags for that at least. Say yay if you think flags for this are indeed the way to go and it will happen very soon, a new git-repository release is inbound.

The second thing I noticed is that there are some feature flags mentioned, such as ["git-protocol"][1], however, none of them are listed in the features section[3].

It's a cargo feature that I learned about quite late. Optional dependencies are feature toggles automatically. Quite neat I think.

Thanks a lot for the feature-toggle feedback in general, these are 'quite an experiment' and where put together just to make things work with the workspace consumers gitoxide-core and transitively gitoxide, so there's certainly a lot of room for improvement.

[FintanH]

The idea here is that the consuming applications makes that decision. Probably it's odd there is no flags to pass down, right now the idea to pull in the crate to configure its feature flags, relying on cargo's compile model to add these flags for you transitively.

Couldn't this lead to having inconsistent types as well, since we could end up with two versions of git-protocol, one that is re-exported from git-repository and one that is from the top-level git-protocol? I could foresee this as being a bit of a time sink if someone accidentally uses something via git-repository when they should have been using it from git-protocol, and suddenly they're getting compiler errors about no implementation for X 🙃 Sure it's easy to fix that by changing the import but can be frustrating.

I'm not sure what the right solution is here though. It would be nice if feature flags were composable like, git-protocol:async-client in git-repository which enables async-client transitively. But I imagine I'd need to propose a rustc RFC for that 😅

It's a cargo feature that I learned about quite late. Optional dependencies are feature toggles automatically. Quite neat I think.

Well TIL :)

[Byron]

Couldn't this lead to having inconsistent types as well, since we could end up with two versions of git-protocol, one that is re-exported from git-repository and one that is from the top-level git-protocol.

Yes, absolutely. Let's not promote this usage for non-workspace users and instead put the async/blocking features out as feature toggles.

I'm not sure what the right solution is here though. It would be nice if feature flags were composable like, git-protocol:async-client in git-repository which enables async-client transitively. But I imagine I'd need to propose a rustc RFC for that 😅

And while that is on the way and incubating… ;), important toggles will have to be re-exported at least in the case of git-repository.

Please note that the git-features crate is absolutely meant to be used like this, and thus must be kept in sync with the crate version as used in the whole dependency graph. This definitely means it has to be stability tier 1 to make that feasible.

Right now it seems there is no perfect solution to this, but decent decisions can be made on a case by case basis.

I will let you know here when the new release with these toggles is ready, I want to get it all ready within the next 4 hours.

[FintanH]

I will let you know here when the new release with these toggles is ready, I want to get it all ready within the next 4 hours.

Appreciate it! FYI: we fixed the build issues by bumping some versions[0]

[Byron]

git-repository v0.9 was just released which provides the respective feature flags. Please let me know if this works for you.

Since I manually tried to do what #192 will do automatically, I think that one should be done next to assure git-repository gets its minor version bumped when the plumbing crates have breaking changes, and prevent surprising breakages on CI.

[FintanH]

git-repository v0.9 was just released which provides the respective feature flags. Please let me know if this works for you.

I'll whip it up now and see if I can get it going.

FYI your latest release broke radicle-link builds again[0]. Not sure what went wrong but wanted to flag it with you.

[FintanH]

I'll whip it up now and see if I can get it going.

I wasn't able to build. I have the following entry in link-git-protocol's Cargo.toml:

[dependencies.git-repository]
version = "0.9.0"
features = [ "async-network-client", "unstable" ]

and got the following error:

    Updating crates.io index
  Downloaded git-repository v0.9.0
  Downloaded git-ref v0.7.1
  Downloaded 2 crates (83.4 KB) in 1.36s
    Checking git-transport v0.11.1
    Checking git-object v0.14.0
    Checking git-pack v0.10.0
error[E0599]: no method named `write_to` found for reference `&SignatureRef<'_>` in the current scope
  --> /home/haptop/.cargo/registry/src/github.spider-man.dpdns.org-1ecc6299db9ec823/git-object-0.14.0/src/encode.rs:56:11
   |
56 |     value.write_to(&mut out)?;
   |           ^^^^^^^^ method not found in `&SignatureRef<'_>`
   |
   = help: items from traits can only be used if the trait is implemented and in scope
note: `WriteTo` defines an item `write_to`, perhaps you need to implement it
  --> /home/haptop/.cargo/registry/src/github.spider-man.dpdns.org-1ecc6299db9ec823/git-object-0.14.0/src/traits.rs:6:1
   |
6  | pub trait WriteTo {
   | ^^^^^^^^^^^^^^^^^

error: aborting due to previous error

For more information about this error, try `rustc --explain E0599`.
error: could not compile `git-object`

To learn more, run the command again with --verbose.
warning: build failed, waiting for other jobs to finish...
error: build failed
[Finished running. Exit status: 101]

[Byron]

FYI your latest release broke radicle-link builds again[0]. Not sure what went wrong but wanted to flag it with you.

Argh, this problem definitely needs [super-human][#192] help :D.

I wasn't able to build. I have the following entry in link-git-protocol's Cargo.toml: […]

This needs a cargo update run to work, as some crates only had to bump their patch level as the change wasn't supposed to be breaking 🤦‍♂️. This is also a case where #192 wouldn't have helped as patch releases would be assumed to actually add features. Being pre-release is a pity here. To fix this, cargo smart-release should unconditionally set the versions of pre-release crates in depending crates. #194 is there to remedy this and should be an easy fix.

Sorry for all the hassle, I am happy that fixes for this are on the horizon so that soon enough we won't remember that it once was quite different 😅.

PS: We all agree that spurious CI failures are a pain to be cured once and for all. Recently my thus far perfect CI started to show signs of such a sickness too :/, but without a fix.

[FintanH]

This needs a cargo update run to work

Ah yes, my bad 🤦 I didn't even think of that at the end of my day 😅 I've got a working patch now :) https://lists.sr.ht/~radicle-link/dev/patches/25082

Sorry for all the hassle

No worries, we're learning together 😄

PS: We all agree that spurious CI failures are a pain to be cured once and for all.

Oh damn 😞 It's always the nitty gritty that gets in the way of the work we want to do haha

[Byron]

You are so fast in adopting this 😅.

It's probably a good thing to move ahead in all areas, that is working with git repositories via git-repository and using more Rust in the server via git-protocol and maybe even git-serve.

Let me close this issue as I think the feature flags are OK for now.