Skip to content
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

Initial support for git submodules #103

Closed
wants to merge 4 commits into from
Closed

Initial support for git submodules #103

wants to merge 4 commits into from

Conversation

@rtobar
Copy link

@rtobar rtobar commented Aug 3, 2017

This initial support is based on the work done by @jonkeane for devtools, plus minor additions from my side. It includes submodules support for git2r, xgit and github remotes. It is not fully recursive yet, and the code is still not all cleanly separated, but it works as a first approach.

This pull request addresses #82.

jonkeane and others added 4 commits Aug 3, 2017
This was originally committed to the devtools package, but now ported to
remotes. The original work is from Jonathan Keane, but porting (which
required little effort) is mine.

Signed-off-by: Rodrigo Tobar <rtobar@icrar.org>
For example, https://github.com/ropensci/rzmq has a .gitmodules without a url

Signed-off-by: Rodrigo Tobar <rtobar@icrar.org>
Signed-off-by: Rodrigo Tobar <rtobar@icrar.org>
I left the warning message still there because testing has been very
manual and restricted to a few cases; better safe than sorry.

Signed-off-by: Rodrigo Tobar <rtobar@icrar.org>
@ronammar
Copy link

@ronammar ronammar commented Nov 13, 2017

Thanks for this useful feature @rtobar and @jonkeane! Any idea when this code will be merged into master?

Loading

@rtobar
Copy link
Author

@rtobar rtobar commented Nov 14, 2017

No idea from my side. The pull request has been hanging out for a bit, but nothing much has happened since. Hopefully your comment will bring the attention back to this pull request.

Loading

@ronammar
Copy link

@ronammar ronammar commented Jan 18, 2018

Bump?

Loading

@gaborcsardi
Copy link
Contributor

@gaborcsardi gaborcsardi commented Jan 18, 2018

Well, you said that:

not fully recursive yet, and the code is still not all cleanly separated,

so I was wondering if you wanted to finish it.

Loading

@ronammar
Copy link

@ronammar ronammar commented Jan 18, 2018

@rtobar - thoughts? I've been using your fork and it works recursively for me, so what functionality is absent?

Loading

@rtobar
Copy link
Author

@rtobar rtobar commented Jan 19, 2018

Hi @ronammar, @gaborcsardi

When I say that it's not fully recursive I mean that submodules are not checked for submodules (e.g., repository A references repository B, which in turn references repository C), which IIRC hasn't been implemented yet. I didn't even try that scenario, so if it works it's pure chance. When you say that it works recursively for you, do you mean exactly that, or only that one level of submodule references works fine?

By the code not being cleanly separated I mean that the specifics about "recursiveness" are not generic enough at the moment since {{install-remotes.R}} has code that is specific to {{git}}-related remotes, instead of generalizing this concept and coding it into each of the supported remotes. As a first iteration it works though, which is why I though this request was maybe already good enough to be present as a first iteration.

If @gaborcsardi thinks this needs a bit more of work before being accepted I could try to find some time to address the comments, but can't promise any speedy delivery.

Loading

@ronammar
Copy link

@ronammar ronammar commented Jan 19, 2018

@rtobar Admittedly, I've only tested with a single submodule level, so it behaved the same as a git clone recursive call for the same repo.

Loading

jimhester added a commit to jimhester/remotes that referenced this issue Aug 10, 2018
jimhester added a commit to jimhester/remotes that referenced this issue Aug 16, 2018
jimhester added a commit to jimhester/remotes that referenced this issue Aug 16, 2018
@jimhester jimhester closed this in 0927172 Aug 20, 2018
@ronammar
Copy link

@ronammar ronammar commented Aug 30, 2018

Tested and it works for me. Thanks!

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants