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

Byron · 2021-09-01T14:05:22Z

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.

test/src/test/integration/link_git_protocol.rs

kim · 2021-09-02T10:00:49Z

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 · 2021-09-02T11:44:48Z

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 · 2021-09-06T15:59:38Z

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 · 2021-09-07T01:10:38Z

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 · 2021-09-07T04:42:05Z

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:

pin all gitoxide crate versions at the cost of not getting patches automatically and upgrading periodically.
consume all gitoxide code through git-repository without using the unstable flag. That's not feasible today as the API surface doesn't cover what's needed to interact with low-level git-protocol facilities. This would be my preferred choice as it would mean that git-repository indeed fulfils its promise of delivering highest performance with negligible overhead for the convenience layer it provides.
gitoxide pre-release plumbing crates bump their dependents (those workspace crates who use it) minor version once their minor version changes to prevent auto-updates on the consumers side. This is what's (much better) described in the smart-release issue I mentioned earlier.

test/src/test/integration/link_git_protocol.rs

kim · 2021-09-07T07:14:48Z

test/src/test/integration/link_git_protocol.rs

        )
-        .unwrap();
+        .unwrap()
+        .detach();
    repo.reference(


I always found it a bit odd that reference creates a reference, but well names

Me too! And it was aptly called create_reference before. But as I worked with this it dawned on me how libgit2 (or git2) is thinking and I started liking it and thought it was beneficial to not unnecessarily change names.

This also means that reference**s** now creates an intermediate object for iterators, and you guessed it, that was called iter_references() before I took the U-turn there as well.

I don't know if it's good to be so git2 inspired there, and already thought providing an alternative name for the same functionality potentially even behind feature

[2000 words later]… names are hard.

test/src/test/integration/link_git_protocol.rs

kim · 2021-09-07T07:23:30Z

test/src/test/integration/link_git_protocol.rs

@@ -317,13 +335,17 @@ where
        assert!(out.pack.is_some());
    }

-    let remote_repo = git2::Repository::open(&remote).unwrap();
+    let mut remote_repo = git::open(remote.clone()).unwrap().into_easy_arc_exclusive(); // without GATs we need the arc version


GATs would be needed on gitoxide's side?

Yes, just there. Here is the code for that and the reason mutable RefMut<'argh_a_lifetime_that_cannot_be_expressed, Repository> can't be done yet.

kim · 2021-09-07T07:38:42Z

test/src/test/integration/link_git_protocol.rs

@@ -341,6 +363,8 @@ where
        update_tips(&local_repo, &out.wanted_refs).unwrap();
    }

+    // Need to refresh it as it didn't notice the new pack


Interesting.. what does it exactly mean to open a repository, in terms of I/O, file handles, etc?

I'm asking because git2 does a lot actually (including resolving and parsing config files), so we ended up using git2::Repository from a (database) pool to avoid fd exhaustion and other issues. I suspect that we'd not use easy mode in a lot of places in order to be able to carefully manage resource consumption, but I think documentation about what the cost of an easy repo is would help.

Indeed, documentation is non-existing in places and I plan to rectify that with a quick-pass of doc-writing before I move on.

Right now open(…) doesn't do much except for mapping all packs in full and reading the local repository config file once. Everything else is lazy.

On the server you would do the same, but instead of holding a git::Repository you would hold a git::EasyArcExclusive. That way, access to the persistent pack memory maps is shared, but caches and on-demand maps (like packed-refs) are not.

Edit: An Easy repo is nothing more than a shared Repository and an cache, cloning the Easy shares the repo and creates a new cache.

Even though it's not implemented, I would like to have a refresh() to reload the object database. And thinking about it, it's easy to demo this API here but would then force the local repo to be an EasyArcExclusive as well.

Probably worth sketching this out while we are here.

Note that git-config will be fully loaded into memory so no handles will be left open.

test/src/test/integration/link_git_protocol.rs

FintanH · 2021-09-07T08:53:46Z

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 😬

kim · 2021-09-07T10:13:52Z

* 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 · 2021-09-07T14:42:05Z

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 · 2021-09-07T14:50:58Z

Pending Changes

Show how packs can be refreshed after pull/fetch
See if writing ObjectRefs is as easy as it sounds for consistency and unbound performance

Byron · 2021-09-08T14:32:58Z

test/src/test/integration/link_git_protocol.rs

+        repo.commit(
+            "refs/namespaces/foo/refs/heads/main",
+            &auth.to_ref(),
+            &auth.to_ref(),


While I was doing breaking changes, I unified parameter order 😅

Byron · 2021-09-08T14:34:57Z

test/src/test/integration/link_git_protocol.rs

+    let base_id = {
+        repo.commit(
+            "refs/namespaces/foo/refs/heads/main",
+            &auth.to_ref(),


Unfortunately the to_ref() is necessary as we can't have real refs. The *Ref is borrowed, too, for maximum flexibility, or else those who can't move these have to clone them. That's cheap, but I guess it's the question of who has to make the call.

I could imagine introducing a trait for ToRef which eliminates the to_ref() call.

Oh, and before I forget: the refs are a lie, internally it still creates an owned Commit as it's easier to work with. Namely, object ids are in hex but nobody really has these, so they would need internal conversion and memory to hold that. A possible optimization, but one I skipped 😅.

Byron · 2021-09-08T14:35:44Z

test/src/test/integration/link_git_protocol.rs

+    let auth = git::actor::Signature::now_local_or_utc("apollo", "[email protected]");
+
+    let empty_tree_id = repo
+        .write_object(&git::objs::Tree::empty())


Now one can write object refs as well as owned objects, and there is a trait for that so calls look as good as they can.

Byron · 2021-09-08T14:39:11Z

test/src/test/integration/link_git_protocol.rs

@@ -341,6 +360,8 @@ where
        update_tips(&local_repo, &out.wanted_refs).unwrap();
    }

+    // Need to refresh it as it didn't notice the new pack
+    local_repo.refresh_object_database().unwrap();


This is exactly what would have to be done on the server as well once it causes new packs to be written. Internally it currently replaces itself, which is certainly something that could be improved one day.

Signed-off-by: Sebastian Thiel <[email protected]>

Byron · 2021-09-10T02:25:41Z

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

FintanH

LGTM for the tests, just have the one question :)

FintanH · 2021-09-10T07:00:38Z

link-git-protocol/Cargo.toml

+default-features = false # leave out max-performance as it doesn't build on msvc win due to sha1-asm. See https://github.com/RustCrypto/asm-hashes/issues/17 for details
+features = [ "one-stop-shop", "async-network-client", "unstable" ]
+
+[dependencies.git-features]


Why are we adding these back rather than accessing them through git-repository?

It adds all of the default features back except for max-performance, which unfortunately breaks the build.

I was thinking that it might be better to remove max-performance from the default feature set for now, but felt bad about it.

However, now that I wrote this I revisited the issue and fixed it on git-repository side as the superior choice :).

I'm confused, too. It seems like we need git-repository and git-packetline, but not the rest. Maybe a rebase went wrong or something?

Great catch! And even though I don't know how that happened, removing the superfluous packages did indeed not cause any trouble.

That way no special feature configuration is necessary.

…default Related to radicle-dev/radicle-link#780

FintanH

I approve this message 👍

kim · 2021-09-13T06:38:10Z

Closed via 07013fc

Thanks!

Byron · 2021-09-13T23:54:12Z

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.

Byron requested a review from a team as a code owner September 1, 2021 14:05

Byron commented Sep 1, 2021

View reviewed changes

test/src/test/integration/link_git_protocol.rs Outdated Show resolved Hide resolved

xla marked this pull request as draft September 2, 2021 02:50

xla removed the request for review from a team September 2, 2021 02:50

Byron marked this pull request as ready for review September 7, 2021 01:07

Byron commented Sep 7, 2021

View reviewed changes

test/src/test/integration/link_git_protocol.rs Show resolved Hide resolved

Byron commented Sep 7, 2021

View reviewed changes

test/src/test/integration/link_git_protocol.rs Outdated Show resolved Hide resolved

kim reviewed Sep 7, 2021

View reviewed changes

FintanH mentioned this pull request Sep 8, 2021

git-repository: feature flag usage GitoxideLabs/gitoxide#193

Closed

Byron mentioned this pull request Sep 8, 2021

Transform radicle-link integration test setup (as much as possible) to gitoxide GitoxideLabs/gitoxide#164

Closed

9 tasks

Byron commented Sep 8, 2021

View reviewed changes

Use gitoxide for link-git-protocol tests

c053dc7

Signed-off-by: Sebastian Thiel <[email protected]>

FintanH reviewed Sep 10, 2021

View reviewed changes

Bump minimal supported git-repository version

e4942f7

That way no special feature configuration is necessary.

Byron added a commit to GitoxideLabs/gitoxide that referenced this pull request Sep 10, 2021

[repository] don't enforce feature flags that may fail on windows by …

afdec2e

…default Related to radicle-dev/radicle-link#780

Remove now unused plumbing dependencies

825e0d9

FintanH approved these changes Sep 10, 2021

View reviewed changes

kim closed this Sep 13, 2021

Byron deleted the git-protocol-integration-oxidized branch September 13, 2021 23:48

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

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

Uh oh!

Conversation

Byron commented Sep 1, 2021

Uh oh!

Uh oh!

kim commented Sep 2, 2021

Uh oh!

Byron commented Sep 2, 2021

Uh oh!

Byron commented Sep 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Byron commented Sep 7, 2021

Uh oh!

Byron commented Sep 7, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Byron Sep 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Byron Sep 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

FintanH commented Sep 7, 2021

Uh oh!

kim commented Sep 7, 2021 via email

Uh oh!

Byron commented Sep 7, 2021

Uh oh!

Byron commented Sep 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pending Changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Byron commented Sep 10, 2021

Uh oh!

FintanH left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FintanH left a comment

Choose a reason for hiding this comment

Uh oh!

kim commented Sep 13, 2021

Uh oh!

Byron commented Sep 13, 2021

Uh oh!

Uh oh!

Byron commented Sep 6, 2021 •

edited

Loading

Byron Sep 7, 2021 •

edited

Loading

Byron Sep 7, 2021 •

edited

Loading

Byron commented Sep 7, 2021 •

edited

Loading