From d5714d73d4dda1cb2ef39e0e328e328c36b525a2 Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Fri, 7 Dec 2012 11:17:31 -0800 Subject: [PATCH] Revert "Expand submodule subscription support" This reverts commit 333676bd1b6b24459c86def586ae9769b1fd2b33 Too many problems were identified during a secondary review. Change-Id: I6112e6594827a537ac57ccd283b55674506eef20 --- Documentation/user-submodules.txt | 52 ++++++++----------- .../java/com/google/gerrit/pgm/Daemon.java | 12 ----- .../server/config/SshdListenAddress.java | 29 ----------- .../config/SshdListenAddressModule.java | 30 ----------- .../config/SshdListenAddressProvider.java | 46 ---------------- .../google/gerrit/server/git/SubmoduleOp.java | 32 +----------- .../gerrit/server/git/SubmoduleOpTest.java | 48 ++++++++--------- .../gerrit/httpd/WebAppInitializer.java | 8 --- 8 files changed, 47 insertions(+), 210 deletions(-) delete mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/config/SshdListenAddress.java delete mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/config/SshdListenAddressModule.java delete mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/config/SshdListenAddressProvider.java diff --git a/Documentation/user-submodules.txt b/Documentation/user-submodules.txt index f965c27998..cfaf3e981d 100644 --- a/Documentation/user-submodules.txt +++ b/Documentation/user-submodules.txt @@ -1,5 +1,5 @@ -Gerrit Code Review - Superproject subscription to submodules updates -==================================================================== +Gerrit Code Review - Superprojects subscribed to submodules updates +=================================================================== Description ----------- @@ -25,7 +25,7 @@ commit having the updated gitlinks. Git Submodules Overview ----------------------- -Submodules are a git feature that allows an external repository to be +It is a git feature that allows an external repository to be attached inside a repository at a specific path. The objective here is to provide a brief overview, further details can be found in the official git submodule command documentation. @@ -37,7 +37,7 @@ at path 'a' by executing the following command when being inside 'super': ===== git submodule add ssh://server/a a -===== +==== Still considering the above example, after its execution notice that inside the local repository 'super' the 'a' folder is considered a @@ -63,11 +63,11 @@ Creating a New Subscription Defining the Submodule Branch ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -This is required because submodule subscription is actually the +This is required because Submodule subscription is actually the subscription of a submodule project and one of its branches for a branch of a super project. -Since Gerrit manages subscriptions in the branch scope, we could have +Since it manages subscriptions in the branch scope, we could have a scenario having a project called 'super' having a branch 'integration' subscribed to a project called 'a' in branch 'integration', and also having the same 'super' project but in branch 'dev' subscribed to the 'a' @@ -77,23 +77,22 @@ After adding the git submodule to a super project, one should edit the .gitmodules file to add a branch field to each submodule section which is supposed to be subscribed. -As the branch field is a Gerrit specific field it will not be filled -automatically by the git submodule command, so one needs to edit it -manually. Its value should indicate the branch of a submodule project -that when updated will trigger automatic update of its registered -gitlink. +The branch field is not filled by the git submodule command. Its value +should indicate the branch of a submodule project that when updated +will trigger automatic update of its registered gitlink. The branch value could be '.' if the submodule project branch has the same name as the destination branch of the commit having gitlinks/.gitmodules file. -If the intention is to make use of the Gerrit feature described -here, one should always be sure to update the .gitmodules file after -adding submodules to a super project. +The branch field of a submodule section is a custom git submodule +feature for Gerrit use. One should always be sure to fill it in +editing .gitmodules file after adding submodules to a super project, +if it is the intention to make use of the Gerrit feature introduced here. -If a git submodule is added but the branch field is not added to the -.gitmodules file Gerrit will not create a subscribtion for the -submodule and there will be no automatic updates to the superproject. +Any git submodules which are added and not have the branch field +available in the .gitmodules file will not be subscribed by Gerrit +to automatically update the superproject. Detecting and Subscribing Submodules ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ @@ -119,22 +118,17 @@ is merged in branch 'dev-of-a' of 'a' project, Gerrit automatically creates a new commit on branch 'dev' of 'super' updating the gitlink to point to the just merged commit. -Limitations in subscription -~~~~~~~~~~~~~~~~~~~~~~~~~~~ +Canonical Web Url +~~~~~~~~~~~~~~~~~ -Gerrit will only automatically update superprojects where the -submodules are hosted on the same Gerrit instance as the superproject. - -Gerrit determines this by checking the hostname of the submodule -specified in the .gitmodules file and comparing it with the Gerrit -instance's hostname. By default the hostname from the canonical web -url is used, but if SSH is turned on and the SSH server name is -different from the canonical hostname it will be used instead. +Gerrit will automatically update only the superprojects that added +the submodules of urls of the running server (the one described in +the canonical web url value in Gerrit configuration file). The Gerrit instance administrator group should always certify to provide the canonical web url value in its configuration file. Users -should certify to use the correct hostname of the running Gerrit -instance to add/subscribe submodules. +should certify to use the url value of the running Gerrit instance to +add/subscribe submodules. Removing Subscriptions ---------------------- diff --git a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Daemon.java b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Daemon.java index 3af7b29cb2..1cbaff2003 100644 --- a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Daemon.java +++ b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Daemon.java @@ -42,8 +42,6 @@ import com.google.gerrit.server.config.AuthConfig; import com.google.gerrit.server.config.AuthConfigModule; import com.google.gerrit.server.config.CanonicalWebUrlModule; import com.google.gerrit.server.config.CanonicalWebUrlProvider; -import com.google.gerrit.server.config.SshdListenAddressModule; -import com.google.gerrit.server.config.SshdListenAddressProvider; import com.google.gerrit.server.config.GerritGlobalModule; import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.config.MasterNodeStartup; @@ -297,16 +295,6 @@ public class Daemon extends SiteProgram { modules.add(new SmtpEmailSender.Module()); modules.add(new SignedTokenEmailTokenVerifier.Module()); modules.add(new PluginModule()); - - if (sshd) { - modules.add(new SshdListenAddressModule() { - @Override - protected Class> provider() { - return SshdListenAddressProvider.class; - } - }); - } - if (httpd) { modules.add(new CanonicalWebUrlModule() { @Override diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/SshdListenAddress.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/SshdListenAddress.java deleted file mode 100644 index 94c26b9cb5..0000000000 --- a/gerrit-server/src/main/java/com/google/gerrit/server/config/SshdListenAddress.java +++ /dev/null @@ -1,29 +0,0 @@ -// Copyright (C) 2012 The Android Open Source Project -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.gerrit.server.config; - -import static java.lang.annotation.RetentionPolicy.RUNTIME; - -import com.google.inject.BindingAnnotation; - -import java.lang.annotation.Retention; - -/** - * Marker on a {@link String} holding the SSH listen address for this server. - */ -@Retention(RUNTIME) -@BindingAnnotation -public @interface SshdListenAddress { -} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/SshdListenAddressModule.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/SshdListenAddressModule.java deleted file mode 100644 index 044fefb513..0000000000 --- a/gerrit-server/src/main/java/com/google/gerrit/server/config/SshdListenAddressModule.java +++ /dev/null @@ -1,30 +0,0 @@ -// Copyright (C) 2012 The Android Open Source Project -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.gerrit.server.config; - -import com.google.inject.AbstractModule; -import com.google.inject.Provider; - -/** Supports binding the {@link SshdListenAddress} annotation. */ -public abstract class SshdListenAddressModule extends AbstractModule { - @Override - protected void configure() { - final Class> provider = provider(); - bind(String.class).annotatedWith(SshdListenAddress.class) - .toProvider(provider); - } - - protected abstract Class> provider(); -} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/SshdListenAddressProvider.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/SshdListenAddressProvider.java deleted file mode 100644 index 2e56b3dfe4..0000000000 --- a/gerrit-server/src/main/java/com/google/gerrit/server/config/SshdListenAddressProvider.java +++ /dev/null @@ -1,46 +0,0 @@ -// Copyright (C) 2012 The Android Open Source Project -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.gerrit.server.config; - -import com.google.inject.Inject; -import com.google.inject.Provider; - -import org.eclipse.jgit.lib.Config; - -/** Provides {@link SshdListenAddressl} from {@code sshd.listenAddress}. */ -public class SshdListenAddressProvider implements Provider { - private final String sshAddress; - - @Inject - public SshdListenAddressProvider(@GerritServerConfig final Config config) { - String sshdListenAddress = config.getString("sshd", null, "listenAddress"); - String sshdAdvertisedAddress = config.getString("sshd", null, "advertisedAddress"); - - /* - * If advertised address is specified it should take precedence over the - * "normal" listening address. - */ - if (sshdAdvertisedAddress != null && ! sshdAdvertisedAddress.isEmpty()) { - sshAddress = sshdAdvertisedAddress; - } else { - sshAddress = sshdListenAddress; - } - } - - @Override - public String get() { - return sshAddress; - } -} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/SubmoduleOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/SubmoduleOp.java index 397631545d..7129b86e71 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/SubmoduleOp.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/SubmoduleOp.java @@ -21,7 +21,6 @@ import com.google.gerrit.reviewdb.client.SubmoduleSubscription; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.config.CanonicalWebUrl; -import com.google.gerrit.server.config.SshdListenAddress; import com.google.gerrit.server.extensions.events.GitReferenceUpdated; import com.google.gerrit.server.util.SubmoduleSectionParser; import com.google.gwtorm.server.OrmException; @@ -79,7 +78,6 @@ public class SubmoduleOp { private RevCommit mergeTip; private RevWalk rw; private final Provider urlProvider; - private final Provider sshProvider; private ReviewDb schema; private Repository db; private Project destProject; @@ -95,7 +93,6 @@ public class SubmoduleOp { public SubmoduleOp(@Assisted final Branch.NameKey destBranch, @Assisted RevCommit mergeTip, @Assisted RevWalk rw, @CanonicalWebUrl @Nullable final Provider urlProvider, - @SshdListenAddress @Nullable final Provider sshProvider, final SchemaFactory sf, @Assisted Repository db, @Assisted Project destProject, @Assisted List submitted, @Assisted final Map commits, @@ -105,7 +102,6 @@ public class SubmoduleOp { this.mergeTip = mergeTip; this.rw = rw; this.urlProvider = urlProvider; - this.sshProvider = sshProvider; this.schemaFactory = sf; this.db = db; this.destProject = destProject; @@ -141,32 +137,6 @@ public class SubmoduleOp { } try { - String thisServer = null; - String sshHostname = null; - String webHostname = new URI(urlProvider.get()).getHost(); - - // If SSH is disabled on the command line there will be no - // sshProvider to ask. Translate this to the gerrit.config - // option "off" inside the [sshd] section. - if (sshProvider == null) { - sshHostname = "off"; - } else { - // Make sure we only remove the port part and not a substring - // of an IPv6 address. - sshHostname = sshProvider.get().split(":\\d+$")[0]; - } - // Use the web host name (canonical web url) for the following - // conditions: - // - If the ssh hostname is the same as the web host - // - If we listen on all addresses - // - If sshd is turned off - if (webHostname.equals(sshHostname) || sshHostname.equals("*") || - sshHostname.equals("off")) { - thisServer = webHostname; - } else { - thisServer = sshHostname; - } - final TreeWalk tw = TreeWalk.forPath(db, GIT_MODULES, mergeTip.getTree()); if (tw != null && (FileMode.REGULAR_FILE.equals(tw.getRawMode(0)) || FileMode.EXECUTABLE_FILE @@ -175,6 +145,8 @@ public class SubmoduleOp { BlobBasedConfig bbc = new BlobBasedConfig(null, db, mergeTip, GIT_MODULES); + final String thisServer = new URI(urlProvider.get()).getHost(); + final Branch.NameKey target = new Branch.NameKey(new Project.NameKey(destProject.getName()), destBranch.get()); diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/git/SubmoduleOpTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/git/SubmoduleOpTest.java index e917e12d8f..67858aa4fc 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/git/SubmoduleOpTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/git/SubmoduleOpTest.java @@ -71,7 +71,6 @@ public class SubmoduleOpTest extends LocalDiskRepositoryTestCase { private SubmoduleSubscriptionAccess subscriptions; private ReviewDb schema; private Provider urlProvider; - private Provider sshProvider; private GitRepositoryManager repoManager; private GitReferenceUpdated replication; @@ -85,19 +84,18 @@ public class SubmoduleOpTest extends LocalDiskRepositoryTestCase { schema = createStrictMock(ReviewDb.class); subscriptions = createStrictMock(SubmoduleSubscriptionAccess.class); urlProvider = createStrictMock(Provider.class); - sshProvider = createStrictMock(Provider.class); repoManager = createStrictMock(GitRepositoryManager.class); replication = createStrictMock(GitReferenceUpdated.class); } private void doReplay() { - replay(schemaFactory, schema, subscriptions, urlProvider, sshProvider, - repoManager, replication); + replay(schemaFactory, schema, subscriptions, urlProvider, repoManager, + replication); } private void doVerify() { - verify(schemaFactory, schema, subscriptions, urlProvider, sshProvider, - repoManager, replication); + verify(schemaFactory, schema, subscriptions, urlProvider, repoManager, + replication); } /** @@ -119,8 +117,7 @@ public class SubmoduleOpTest extends LocalDiskRepositoryTestCase { final Branch.NameKey branchNameKey = new Branch.NameKey(new Project.NameKey("test-project"), "test-branch"); - expect(urlProvider.get()).andReturn("http://localhost:8080").atLeastOnce(); - expect(sshProvider.get()).andReturn("*:29418"); + expect(urlProvider.get()).andReturn("http://localhost:8080"); expect(schema.submoduleSubscriptions()).andReturn(subscriptions); final ResultSet emptySubscriptions = @@ -133,9 +130,9 @@ public class SubmoduleOpTest extends LocalDiskRepositoryTestCase { doReplay(); final SubmoduleOp submoduleOp = - new SubmoduleOp(branchNameKey, mergeTip, new RevWalk(realDb), - urlProvider, sshProvider, schemaFactory, realDb, null, - new ArrayList(), null, null, null, null); + new SubmoduleOp(branchNameKey, mergeTip, new RevWalk(realDb), urlProvider, + schemaFactory, realDb, null, new ArrayList(), null, null, + null, null); submoduleOp.update(); @@ -629,8 +626,7 @@ public class SubmoduleOpTest extends LocalDiskRepositoryTestCase { new Branch.NameKey(new Project.NameKey("target-project"), sourceBranchNameKey.get()); - expect(urlProvider.get()).andReturn("http://localhost:8080").atLeastOnce(); - expect(sshProvider.get()).andReturn("*:29418"); + expect(urlProvider.get()).andReturn("http://localhost:8080"); expect(schema.submoduleSubscriptions()).andReturn(subscriptions); final ResultSet subscribers = @@ -661,9 +657,10 @@ public class SubmoduleOpTest extends LocalDiskRepositoryTestCase { final SubmoduleOp submoduleOp = new SubmoduleOp(sourceBranchNameKey, sourceMergeTip, new RevWalk( - sourceRepository), urlProvider, sshProvider, schemaFactory, - sourceRepository, new Project(sourceBranchNameKey.getParentKey()), - submitted, mergedCommits, myIdent, repoManager, replication); + sourceRepository), urlProvider, schemaFactory, sourceRepository, + new Project(sourceBranchNameKey.getParentKey()), submitted, + mergedCommits, myIdent, repoManager, replication); + submoduleOp.update(); doVerify(); @@ -730,8 +727,7 @@ public class SubmoduleOpTest extends LocalDiskRepositoryTestCase { new Branch.NameKey(new Project.NameKey("target-project"), sourceBranchNameKey.get()); - expect(urlProvider.get()).andReturn("http://localhost:8080").atLeastOnce(); - expect(sshProvider.get()).andReturn("*:29418"); + expect(urlProvider.get()).andReturn("http://localhost:8080"); expect(schema.submoduleSubscriptions()).andReturn(subscriptions); final ResultSet subscribers = @@ -764,9 +760,10 @@ public class SubmoduleOpTest extends LocalDiskRepositoryTestCase { final SubmoduleOp submoduleOp = new SubmoduleOp(sourceBranchNameKey, sourceMergeTip, new RevWalk( - sourceRepository), urlProvider, sshProvider, schemaFactory, - sourceRepository, new Project(sourceBranchNameKey.getParentKey()), - submitted, mergedCommits, myIdent, repoManager, replication); + sourceRepository), urlProvider, schemaFactory, sourceRepository, + new Project(sourceBranchNameKey.getParentKey()), submitted, + mergedCommits, myIdent, repoManager, replication); + submoduleOp.update(); doVerify(); @@ -867,8 +864,7 @@ public class SubmoduleOpTest extends LocalDiskRepositoryTestCase { final RevCommit mergeTip = git.commit().setMessage("test").call(); - expect(urlProvider.get()).andReturn("http://localhost:8080").atLeastOnce(); - expect(sshProvider.get()).andReturn("*:29418"); + expect(urlProvider.get()).andReturn("http://localhost:8080").times(2); expect(schema.submoduleSubscriptions()).andReturn(subscriptions); expect(subscriptions.bySuperProject(mergedBranch)).andReturn( @@ -919,9 +915,9 @@ public class SubmoduleOpTest extends LocalDiskRepositoryTestCase { final SubmoduleOp submoduleOp = new SubmoduleOp(mergedBranch, mergeTip, new RevWalk(realDb), - urlProvider, sshProvider, schemaFactory, realDb, new Project( - mergedBranch.getParentKey()), new ArrayList(), null, - null, repoManager, null); + urlProvider, schemaFactory, realDb, new Project(mergedBranch + .getParentKey()), new ArrayList(), null, null, + repoManager, null); submoduleOp.update(); } diff --git a/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java b/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java index 07d38c8e31..6dab0d3c9c 100644 --- a/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java +++ b/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java @@ -27,8 +27,6 @@ import com.google.gerrit.server.cache.h2.DefaultCacheFactory; import com.google.gerrit.server.config.AuthConfig; import com.google.gerrit.server.config.AuthConfigModule; import com.google.gerrit.server.config.CanonicalWebUrlModule; -import com.google.gerrit.server.config.SshdListenAddressModule; -import com.google.gerrit.server.config.SshdListenAddressProvider; import com.google.gerrit.server.config.GerritGlobalModule; import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.config.GerritServerConfigModule; @@ -240,12 +238,6 @@ public class WebAppInitializer extends GuiceServletContextListener { return HttpCanonicalWebUrlProvider.class; } }); - modules.add(new SshdListenAddressModule() { - @Override - protected Class> provider() { - return SshdListenAddressProvider.class; - } - }); modules.add(new MasterNodeStartup()); return cfgInjector.createChildInjector(modules); }