feat(clone): turbo clone #9904
feat(clone): turbo clone #9904
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
…er finding bugs via tests
| @@ -1274,7 +1285,6 @@ pub async fn run( | |||
| }; | |||
|
|
|||
| cli_args.command = Some(command); | |||
| cli_args.cwd = Some(repo_root.as_path().to_owned()); | |||
There was a problem hiding this comment.
as far as I can tell, we don't actually use the cwd as repo root anywhere. can correct if this isn't true
There was a problem hiding this comment.
Is this a necessary change for this PR? repo_root looks like a bad name since it can get it's value from cli_args.cwd. This value will now no longer be made absolute if it is given as a relative value.
There was a problem hiding this comment.
Unfortunately it is necessary, although I can just save the old cwd instead. We don't want the repo root to be our cwd and in fact, we probably don't want to be in a repo in the first place for turbo clone. This is a pretty big departure from our other commands.
| Make sure we allow partial clones | ||
|
|
||
| Do a blobless clone | ||
| $ ${TURBO} clone file://$(pwd)/basic-monorepo.t basic-monorepo-blobless --local |
There was a problem hiding this comment.
I'm not convinced we're actually doing a real blobless clone here. If anybody has ideas for better testing that doesn't involve hitting a remote git repo (unless we really have to ig), I'm open to ideas.
There was a problem hiding this comment.
Like the config claims it's blobless but my suspicion is that git only does a blobless clone if you really need it. For trivial repos it just doesn't make sense. I tried adding a test that adds a file, then deletes in in a second commit, and then does a blobless clone, with the idea that the file should not be included in the clone since it's not a blob that is reachable from HEAD. But the file was indeed cloned, so who knows.
There was a problem hiding this comment.
If you're not convinced this test is actually testing the behavior we want than we shouldn't include it. Only suggestion for testing would be to set up a local git server and clone from that, but that feels a bit like overkill.
There was a problem hiding this comment.
We could try to clone against GitHub but that's a little sketchy for a test
| @@ -1274,7 +1285,6 @@ pub async fn run( | |||
| }; | |||
|
|
|||
| cli_args.command = Some(command); | |||
| cli_args.cwd = Some(repo_root.as_path().to_owned()); | |||
There was a problem hiding this comment.
Is this a necessary change for this PR? repo_root looks like a bad name since it can get it's value from cli_args.cwd. This value will now no longer be made absolute if it is given as a relative value.
| Make sure we allow partial clones | ||
|
|
||
| Do a blobless clone | ||
| $ ${TURBO} clone file://$(pwd)/basic-monorepo.t basic-monorepo-blobless --local |
There was a problem hiding this comment.
If you're not convinced this test is actually testing the behavior we want than we shouldn't include it. Only suggestion for testing would be to set up a local git server and clone from that, but that feels a bit like overkill.
Description
Implements
turbo clone, essentially a small wrapper aroundgit clonethat does simple optimizations like blobless clones for local development and treeless clones for CI.Testing Instructions
Wrote some tests but not convinced they're actually effective. See comments below