fix(proxy): checkout peer #1010
Conversation
We set up checking out projects differing by where the project is coming from. If it's a owned by the user then we simply clone the repo calling the remote `rad`. If it's coming from another peer's branch then we set it up via their remote. We then have to create the `rad` remote to set that up as our upstream. This all culminates in testing that we can clone from a specific peer and that we don't end up getting changes from another peer that we know about.
|
Note that we get this in the log output:
But this isn't an error that breaks the tests. |
|
lol sorry |
|
Spooky action at a distance. |
|
@kim The error is still popping up 🤔 |
|
When cloning from a peer, do we setup an additional EDIT: I guess it's this: builder.remote_create(move |repo, _remote_name, url| {
Remote::new(url, name.clone()).create(repo)
}); |
| /// We're checking out a remote's version of the project. | ||
| Remote { | ||
| /// The handle of the remote. | ||
| handle: String, |
There was a problem hiding this comment.
Is this the handle as in hash(id.revs[0])?
There was a problem hiding this comment.
Nope, it's the handle of the peer. I'll update the docs 👍
There was a problem hiding this comment.
Ok I'm not sure what that means, waiting to see the doc update :)
There was a problem hiding this comment.
Ah I see, it's the non-unique (display) name of the user.
There was a problem hiding this comment.
💯 good to know the docs helped 😄
|
Aside: how did you come up with the |
It was originally specified here: radicle-dev/radicle-link#235 |
There was a problem hiding this comment.
Smol changes, dope! It's unclear what the behaviour is we expect, there is some default pushing going on. Wouldn't the expectation be that a checkout of another peer is more like an upstream set we sometimes fetch from? Like how the origin is tracked on a forked repo to get mainline changes in.
proxy/coco/src/project/checkout.rs
Outdated
|
|
||
| include::set_include_path(&repo, self.include_path)?; | ||
| // Create a rad remote and push the default branch so we can set it as the | ||
| // upstream. |
There was a problem hiding this comment.
Why are we pushing to the remote for another peer by default?
There was a problem hiding this comment.
We're not :) We're pushing for our peer so that they have a copy of the default branch under their rad remote. Similar to having the default branch under origin when you clone from GitHub. This then becomes your version of the default branch.
There was a problem hiding this comment.
I see, I guess. It's hard to follow which end is which in this code and what the to expected state at the end is for the working copy.
There was a problem hiding this comment.
Cool, that's good to know. I'll make it clearer by maybe breaking it out or more documentation. Tbh, I could only reason through it by looking at the outcomes locally so I'm not surprised it's not clear :)
There was a problem hiding this comment.
I also see:
WARN librad::git::local::transport > child process still running, sending SIGKILL
in the logs when creating projects and doing checkouts when manually testing. The functionality seems to work though.
Other than that, looks good from my end & can't wait to test it with real peers.
We move the clone functions as methods on Ownership to make things clearer and compartmentalised. We also document the checkout verbosely, because it's not very clear from the actions take but is clearer when described and shown what the final config looks like.
proxy/coco/src/state.rs
Outdated
| @@ -577,11 +581,29 @@ impl State { | |||
| where | |||
| P: Into<Option<PeerId>> + Clone + Send + 'static, | |||
| { | |||
| let proj = self.get_project(urn.clone(), peer_id).await?; | |||
| let include_path = self.update_include(urn).await?; | |||
| let proj = self.get_project(urn.clone(), None).await?; | |||
There was a problem hiding this comment.
What's the significance of not passing the peer_id here anymore?
There was a problem hiding this comment.
We can always get a project without a PeerId so I thought that would make sense to get the project metadata that way. But now that I'm thinking about it, it might be significant if the user's project data differs from the peers. If the default branch has changed it might mean that the user tries to checkout the wrong branch.
So, I think I'll change it back to passing the peer_id 👍
|
Converting back to a draft because it wasn't fully working when testing in the application itself. There's some weirdness to do with the local transport protocol that I'm not quite understanding. |
- remote should have been called peerId and is optional - passing the PeerId was stubbed so we pass down the currentPeerId now - the endpoint didn't have the full path since it was missing `/checkout`. It must have been a fluke that it was a match regardless.
It turns out we were using the refspecs and the URLs wrong. We create the correct refspecs here and clean up the documentation and parameter passing.
Since we need the guard for self peer id in checkout as well, we move them to the http module so that they can be reused. But they should eventually be removed once we figure out how to handle this better.
Closes #849
This PR ensures that we when we do a checkout of a project that it comes from the correct peer. There are two cases:
In the former, it looks similar to creating a new project. We clone the project and set
radas our default remote.In the latter, we need to set up the project based off of the peer's reference and then also create the rad remote for our default remote.
The test creates three peers. Bob clones from Alice, and Eve clones from Bob. Bob makes a commit to his working copy. Then Eve creates a working copy based off of Alice's view, and we ensure the Eve does not see Bob's commit.