-
Notifications
You must be signed in to change notification settings - Fork 399
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add shallow cloning capability #979
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding this!
Can you also include a test for it?
src/build.rs
Outdated
/// Specify fetch depth, a value less than zero is interpreted as pull everything (effectively the same as no declaring a limit depth) | ||
pub fn depth(&mut self, depth: i32) -> &mut RepoBuilder<'cb> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of adding a new method here, would it be possible to leave this as a setting that required calling .fetch_options(...)
instead? I am reluctant to try to repeat low-level option settings, since that would require deciding which settings are or are not replicated, and possibly causing confusion when something could be specified in multiple places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So should this be removed? I find the depth
parameter to be kinda (very) important imo, but it can be removed without any problems.
src/build.rs
Outdated
// if let Some(ref fetch_opts) = &self.fetch_opts { | ||
// fetch_opts.depth(depth); | ||
// } else { | ||
// let mut fo = FetchOptions::new(); | ||
// fo.depth(depth); | ||
// self.fetch_options(fo); | ||
// } | ||
// self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks to be some vestigial work?
src/repo.rs
Outdated
/// Clone a remote repository with a depth of one (shallow cloning) | ||
/// | ||
/// Similar to `git clone --depth 1` | ||
pub fn shallow_clone<P: AsRef<Path>>(url: &str, into: P) -> Result<Repository, Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the comment on RepoBuilder
, I think I would like to avoid having too many convenience functions, since there is a somewhat combinatorial explosion of all the different options someone might want. Using a builder pattern can avoid adding all sorts of variants of different methods.
I'm not sure how to add a test for this, it seems like there aren't any tests for cloning features. |
At the bottom of |
This new commit addresses the comments and contains a first attempt to test it. It should work but it doesn't, I need some guidance here. The test creates a repo and commits two times to that new repo. Then creates a |
Any work on this? |
Unfortunately it looks like libgit2 does not do shallow clones on local repositories. I think it requires the smart protocol and relies on the server to figure out what to fetch. I don't think we have any testing infrastructure for the smart protocol, so I think we'll just need to not include a test for this for now. I have filed libgit2/libgit2#6634 on that issue. |
So for now, it can go without a test? (I'll add a fixme) |
Yep. |
4680cd1
to
722b7d0
Compare
Why it has not been merged yet? I mean is there nay issue preventing it? |
a901c4c
to
1441a8d
Compare
1441a8d
to
7a80cc2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Add shallow cloning (as well as customizable depth), introduced in
libgit2 v1.7.0