[JENKINS-21248] Support shallow submodule update #303

fujii · 2018-03-09T05:31:10Z

Fix for https://issues.jenkins-ci.org/browse/JENKINS-21248

fujii · 2018-03-09T05:41:27Z

This patch is the git-plugin part of this fix.
fujii/git-plugin@035090e

MarkEWaite

Please provide tests for the shallow setting on submodule clone. The existing submodule tests should be a good starting point to add further tests.

fujii · 2018-03-12T05:56:40Z

Thank you for your feedback, @MarkEWaite.
I created a test, and it passes on my env. But it fails in CI env.
What makes the difference? CI server can access github.com during tests?

MarkEWaite · 2018-03-12T14:55:01Z

I see the same failure when I execute:

mkdir tmp
cd tmp
git clone https://github.com/jenkinsci/git-client-plugin
cd git-client-plugin
git checkout -b fujii-shallow-submodule-update master
git pull https://github.com/fujii/git-client-plugin.git shallow-submodule-update
mvn clean -Dtest=CliGitAPIImplTest test

I assumed it was the missing submoduleInit() call in the new test compared to the test which precedes it in the file. Unfortunately, that was not enough to fix the test. I think you'll need to investigate further.

fujii · 2018-03-13T07:10:05Z

Thank you for testing my patch, @MarkEWaite.
Unfortunately, I can't reproduce the error with your procedure.
I guess it might depend on the region.
So, I created a new test fetching a submodule from the local mirror target/clone.git.
This patch will fail in the CI auto test because https://github.com/jenkinsci/git-client-plugin.git doesn't have tests/getRelativeSubmodule branch yet.
It is in https://github.com/fujii/git-client-plugin/tree/tests/getRelativeSubmodule
How do you think this idea?

darxriggs · 2018-07-04T20:17:06Z

Hi @fujii, could you rebase this pull request on the current master branch?

darxriggs · 2018-07-04T21:04:06Z

src/main/java/org/jenkinsci/plugins/gitclient/CliGitAPIImpl.java

+ return this;
+ }
+
+ public SubmoduleUpdateCommand depth(Integer depth) {


If possible I would go for int instead of Integer for the method parameter and instance variable. (I also see boolean and no Boolean).

Ok. I have seen now that this is done like this to be consisent with the methods for clone/fetch.

darxriggs · 2018-07-04T21:12:49Z

src/test/java/org/jenkinsci/plugins/gitclient/GitAPITestCase.java

@@ -2570,6 +2570,18 @@ public void test_submodule_update() throws Exception {
 assertFixSubmoduleUrlsThrows();
 }

+ @NotImplementedInJGit
+ public void test_submodule_update_shallow() throws Exception {


Other test cases would also be useful:

no shallow(...) call

shallow(false)

depth(...)

fujii · 2018-07-05T02:21:10Z

@darxriggs: Thank you. I will do it. But, I'm pretty busy now. Feel free to take over.

darxriggs · 2018-07-06T15:30:00Z

Hi @fujii, the error you mentioned, is it about referencing shas on github when cloning? Github does not support this (see isaacs/github#436). The respective git option would be uploadpack.allowReachableSHA1InWant. The error output I get is as follows:

hudson.plugins.git.GitException: Command "git submodule update --depth=1 modules/sshkeys" returned status code 1:
stdout: 
stderr: Cloning into '/tmp/jenkinsTests.tmp/jenkins3443046290638298345test/modules/sshkeys'...
error: Server does not allow request for unadvertised object 689c45ed57f0829735f9a2b16760c14236fe21d9
Fetched in submodule path 'modules/sshkeys', but it did not contain 689c45ed57f0829735f9a2b16760c14236fe21d9. Direct fetching of that commit failed.

Your proposed solution with an extra branch that contains a submodule referencing a local git repository is a clever trick. But I find it a bit confusing and an extra branch has to be maintained.

Another idea is to put a repository with submodules into src/test/resources (there are already others) that contains all relevant properties to test submodules. This would also reduce the need to clone submodules from github which implies some risk and constraints.

What are your thoughts @MarkEWaite?

MarkEWaite · 2018-07-06T16:12:57Z

Adding a binary copy of a specific repository into src/test/resources would be fine. I have no problem with extra branches in the git-client-plugin repository for specific tests. I hesitate to have those branches depend on something outside the repository, unless the tests which use those branches are entirely optional.

darxriggs · 2018-07-12T19:56:14Z

src/main/java/org/jenkinsci/plugins/gitclient/JGitAPIImpl.java

@@ -2234,6 +2246,10 @@ public void execute() throws GitException, InterruptedException {
 listener.getLogger().println("[ERROR] JGit doesn't support submodule update --reference yet.");
 throw new UnsupportedOperationException("not implemented yet");
 }
+ if (shallow) {
+ listener.getLogger().println("[ERROR] JGit doesn't support shallow submodules yet.");
+ throw new UnsupportedOperationException("not implemented yet");


Hi @MarkEWaite, should using shallow for JGit throw this UnsupportedOperationException here or just log a WARNING when calling shallow(true) at lines 2224-2227?

As a reference FetchCommand and CloneCommand just perform logging. That's not consistent in this class.

You're right @darxriggs , it would be good enough to log the implementation gap rather than fail the checkout. Shallow clone is a performance optimization that isn't critical to the typical use of the repository.

darxriggs · 2018-07-29T17:33:57Z

As I cannot update this one, I prepared a new pull request #344 with improvements.

fujii · 2018-07-30T02:01:28Z

Thanks. Closed this ticket.

MarkEWaite requested changes Mar 10, 2018

View reviewed changes

fujii force-pushed the shallow-submodule-update branch from 9680c41 to 6af910a Compare March 12, 2018 04:47

[JENKINS-21248] Support shallow submodule update

89ba2d3

fujii force-pushed the shallow-submodule-update branch from 6af910a to 89ba2d3 Compare March 13, 2018 07:03

darxriggs reviewed Jul 4, 2018

View reviewed changes

Merge branch 'master' into shallow-submodule-update

e483818

darxriggs reviewed Jul 12, 2018

View reviewed changes

darxriggs mentioned this pull request Jul 29, 2018

[JENKINS-21248] shallow submodule update #344

Merged

fujii closed this Jul 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-21248] Support shallow submodule update #303

[JENKINS-21248] Support shallow submodule update #303

fujii commented Mar 9, 2018

fujii commented Mar 9, 2018

MarkEWaite left a comment

fujii commented Mar 12, 2018

MarkEWaite commented Mar 12, 2018

fujii commented Mar 13, 2018

darxriggs commented Jul 4, 2018

darxriggs Jul 4, 2018

darxriggs Jul 6, 2018

darxriggs Jul 4, 2018

fujii commented Jul 5, 2018

darxriggs commented Jul 6, 2018

MarkEWaite commented Jul 6, 2018

darxriggs Jul 12, 2018

MarkEWaite Jul 12, 2018

darxriggs commented Jul 29, 2018

fujii commented Jul 30, 2018

[JENKINS-21248] Support shallow submodule update #303

[JENKINS-21248] Support shallow submodule update #303

Conversation

fujii commented Mar 9, 2018

fujii commented Mar 9, 2018

MarkEWaite left a comment

Choose a reason for hiding this comment

fujii commented Mar 12, 2018

MarkEWaite commented Mar 12, 2018

fujii commented Mar 13, 2018

darxriggs commented Jul 4, 2018

darxriggs Jul 4, 2018

Choose a reason for hiding this comment

darxriggs Jul 6, 2018

Choose a reason for hiding this comment

darxriggs Jul 4, 2018

Choose a reason for hiding this comment

fujii commented Jul 5, 2018

darxriggs commented Jul 6, 2018

MarkEWaite commented Jul 6, 2018

darxriggs Jul 12, 2018

Choose a reason for hiding this comment

MarkEWaite Jul 12, 2018

Choose a reason for hiding this comment

darxriggs commented Jul 29, 2018

fujii commented Jul 30, 2018