radicle-dev / radicle-link

The second iteration of the Radicle code collaboration protocol.
Other
423 stars 39 forks source link

Core-corrosion: Adding gitoxide to git-protocol integration tests #780

Closed Byron closed 3 years ago

Byron commented 3 years ago

A draft PR to get feedback early. I's goal is to add additional Rust to the core of git which is currently accessed through a thin corrosive layer provided by the git2 crate.

By the end of this PR I'd expect all usages of git2 to be replaced with at least equivalently readable substitutes provided by gitoxide except for where git2 is explicitly under test.

Doing so is extremely valuable to gitoxide as it gets to develop its high-level-yet-performant API based on actual usage of a proven and mature library.

kim commented 3 years ago

all usages of git2

When you say "all", do you mean all?

There are some uses in the tests which function like fixtures (but have random keys, so are not easy to generate upfront). But there are also other uses which are tied to the fact that librad is, well, tied to git2. I would suggest to scope this exercise the former.

Byron commented 3 years ago

When you say "all", do you mean all?

😅 All in this file except for the git2 specific tests. The strategy here might be to look at git2 usages in tests from time to time and see if it's feasible to replace those with gitoxide, potentially implementing the missing bits and pieces. Maybe that way one will end up with an API surface big enough to be feasible for places outside of tests as well.

I would suggest to scope this exercise the former.

Absolutely, I want it done yesterday to finally finish that git-fetch workblock I am still in, technically. I will keep that as MVP'ish as possible too to get the upload-pack building block more quickly.

Byron commented 3 years ago

It feels I have been working on this for an eternity, so I did hope that it will 'just work' with some success. Will see what's causing the issue the gitoxide can't find an object that is definitely there but only if the repository was handled by thin_pack_x, which doesn't do much with these repositories. It's very strange and I have the feeling an even stranger bug is to be found as cause of this.

On the bright side, one can already see how the git-repository API is shaping up, something like 2000 LOCs later.


Edit: The issue was caused by gitoxide filtering packs by their name and required them to start with pack-. Even though this now is common it's far from a requirement. Removing this constraint fixed the issue.

Byron commented 3 years ago

I think for the most part, using git-repository instead of git2 now is a pleasant experience API wise. gitoxide definitely was inspired by git2 in many places, while also trying to make the API more tree-like for chaining.

To make it nicer, I would love to remove some of the unwrap() calls in favor of returning results, which is done in some places but not everywhere. If that's agreeable, I can patch this.

Besides that, what do you think 🥺?

Byron commented 3 years ago

As a side-note, while preparing this PR with crates from crates.io I managed to break the build surprisingly as this workspace then used incompatible crates in the same build. The problem causing this is well-known and not easy to tackle, I started a discussion over at cargo smart-release to explore ways to make improve the situation.

There are ways to improve the situation already:

FintanH commented 3 years ago

As a side-note, while preparing this PR with crates from crates.io I managed to break the build surprisingly as this workspace then used incompatible crates in the same build.

One of my builds got bitten by this too :grimacing:

kim commented 3 years ago
  • consume all gitoxide code through git-repository

If I understand the problem correctly, then git-repository is depending on newer git-* crates, which we don't pull in because they are breaking as per semver. Going forward, however, I would expect git-repository to be more conservative than lower level crates, so we may not get functionality that is considered unstable until there is a release of git-repository.

So I'm not sure: either git-repository re-exports everything else, and we can control via an unstable feature whether we want the bleeding edge or not, or we would probably maintain our own prelude crate with pinned versions, to avoid having to manage those versions across multiple of our own crates. But I think the latter would preclude depending on git-repository, as we enter crate hell through the other door then.

Byron commented 3 years ago

I would expect git-repository to be more conservative than lower level crates, so we may not get functionality that is considered unstable until there is a release of git-repository.

That's correct, it's in stability tier 1. It re-exports all crates in the same stability tier and hides everything else.

o I'm not sure: either git-repository re-exports everything else, and we can control via an unstable feature […]

Such a feature toggle exists and is used to re-exports all lower tier crates. Along with a new version bumping scheme described in this cargo smart-release ticket, if any of the plumbing crates have breaking changes with a minor bump, git-repository would also have a minor bump. That way you would get patches automatically but not more, i.e. breaking changes. This would allow me to even post PRs for upgrades for you without your CI breaking surprisingly.

For a lack of a better idea, this would be my preference, as it seems more powerful than pinning all git-* crates to their version.

Thinking into the future of post-release crates, we would eventually stop using the unstable feature to get all the benefits of stability tier 1 while lower tier crates may still change, but you won't be using them directly anymore.

Byron commented 3 years ago

Pending Changes

Byron commented 3 years ago

I squashed everything into one commit and rebased against master. Is there anything else that would prevent merging this PR?

kim commented 3 years ago

Closed via 07013fc0e618bc112ad9019e34ed533b946eb0d5

Thanks!

Byron commented 3 years ago

Please note in order to provide actual protection from unexpected breakage coming up through the crates exposed by git-repository cargo-smart-release will have to learn a new trick. For now I have manually bumped git-repository's minor version to assure of that.