From 1f77b537f60ce31e3c9373f1cd7a309698c2f482 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Fri, 8 Nov 2013 07:16:31 +0100 Subject: [PATCH 1/5] Document 'Force Edit' flag of 'Edit Topic Name' access right Change-Id: Ibe87c60788780a845b62f4389200d9e1859a60af Signed-off-by: Edwin Kempin --- Documentation/access-control.txt | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Documentation/access-control.txt b/Documentation/access-control.txt index db806ccac7..189316cc56 100644 --- a/Documentation/access-control.txt +++ b/Documentation/access-control.txt @@ -822,6 +822,10 @@ The change owner, branch owners, project owners, and site administrators can always edit the topic name (even without having the `Edit Topic Name` access right assigned). +Whether the topic can be edited on closed changes can be controlled +by the 'Force Edit' flag. If this flag is not set the topic can only be +edited on open changes. + Examples of typical roles in a project -------------------------------------- From 7afa73ccba58113cbca3e14066630bdfc31e92b7 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Fri, 8 Nov 2013 07:48:47 +0100 Subject: [PATCH 2/5] Minor cleanups in plugin documentation Change-Id: I50e41618f2fdfa7fe8bd84b7050f697d2d4b87e7 Signed-off-by: Edwin Kempin --- Documentation/dev-plugins.txt | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/Documentation/dev-plugins.txt b/Documentation/dev-plugins.txt index d0e361aded..717547b88c 100644 --- a/Documentation/dev-plugins.txt +++ b/Documentation/dev-plugins.txt @@ -650,16 +650,17 @@ this can be specified by setting `scope = CapabilityScope.CORE`: UI Extension ------------ -Plugins can contribute their own UI actions on core Gerrit pages. -This is useful for workflow customization or exposing plugin functionality -through the UI in addition to SSH commands and the REST API. +Plugins can contribute UI actions on core Gerrit pages. This is useful +for workflow customization or exposing plugin functionality through the +UI in addition to SSH commands and the REST API. -For instance a plugin to integrate Jira with Gerrit changes may contribute its -own "File bug" button to allow filing a bug from the change page or plugins to -integrate continuous integration systems may contribute a "Schedule" button to -allow a CI build to be scheduled manually from the patch set panel. +For instance a plugin to integrate Jira with Gerrit changes may +contribute a "File bug" button to allow filing a bug from the change +page or plugins to integrate continuous integration systems may +contribute a "Schedule" button to allow a CI build to be scheduled +manually from the patch set panel. -Two different places on core Gerrit pages are currently supported: +Two different places on core Gerrit pages are supported: * Change screen * Project info screen @@ -768,7 +769,8 @@ public class Module extends AbstractModule { } ---- -The module above must be declared in pom.xml for Maven driven plugins: +The module above must be declared in the `pom.xml` for Maven driven +plugins: [source,xml] ---- @@ -777,7 +779,7 @@ The module above must be declared in pom.xml for Maven driven plugins: ---- -or in the BUCK configuration file for Buck driven plugins: +or in the `BUCK` configuration file for Buck driven plugins: [source,python] ---- @@ -832,7 +834,8 @@ public class HttpModule extends HttpPluginModule { } ---- -The HTTP module above must be declared in pom.xml for Maven driven plugins: +The HTTP module above must be declared in the `pom.xml` for Maven +driven plugins: [source,xml] ---- @@ -841,7 +844,7 @@ The HTTP module above must be declared in pom.xml for Maven driven plugins: ---- -or in the BUCK configuration file for Buck driven plugins +or in the `BUCK` configuration file for Buck driven plugins [source,python] ---- @@ -858,7 +861,7 @@ case. The following prerequisities must be met, to satisfy the capability check: * user is authenticated -* user is a member of the Administrators group, or +* user is a member of a group which has the `Administrate Server` capability, or * user is a member of a group which has the required capability The `apply` method is called when the button is clicked. If `UiAction` is @@ -878,7 +881,7 @@ can be accessed from any REST client, i. e.: ==== A special case is to bind an endpoint without a view name. This is -particularly useful for DELETE requests: +particularly useful for `DELETE` requests: [source,java] ---- From b2261635cc90935af1b11f58062415021d6af144 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Fri, 8 Nov 2013 07:30:45 +0100 Subject: [PATCH 3/5] Documentation: Link from 'Server administration -> Plugins' to plugins At the moment the 'Plugins' entry in the 'Server administration' section in 'index.txt' is a dead label which is a placeholder for documentation which is not written yet. For the time this documentation is not available yet, link to the list of plugins that are hosted on gerrit-review. This is more useful than having a dead label. Change-Id: If39872809703a6df9ac784b62615b5021dd76c35 Signed-off-by: Edwin Kempin --- Documentation/index.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/index.txt b/Documentation/index.txt index 9b8acc4b49..58dae6e8ee 100644 --- a/Documentation/index.txt +++ b/Documentation/index.txt @@ -60,7 +60,7 @@ Index ... How to read stats from the JVM .. High availability .. Replication -.. Plugins +.. link:https://gerrit-review.googlesource.com/#/admin/projects/?filter=plugins%252F[Plugins] .. link:dev-design.html[System Design] .. link:config-contact.html[User Contact Information] .. link:config-reverseproxy.html[Reverse Proxy] From fd195e7b95f7ea3284dff5bf687f181682245af6 Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Fri, 8 Nov 2013 13:01:28 -0800 Subject: [PATCH 4/5] Refactor ChangesCollection to be injectable Remove the user specific control factory, allowing the collection to be injected from the system module rather than a higher level request scoped module. With this refactoring in place it is now possible to reuse ChangesCollection in other REST APIs, such as to call its parse() method to lookup a change as a child member. Change-Id: I8d14b3b9c2dab3cc9f29d968c6a15d0ef6835a04 --- .../gerrit/server/change/ChangeJson.java | 46 +++++++++++-------- .../server/change/ChangesCollection.java | 10 ++-- .../google/gerrit/server/change/Module.java | 1 + .../gerrit/server/project/ChangeControl.java | 9 ++++ .../server/query/change/QueryChanges.java | 7 +-- .../server/query/change/QueryProcessor.java | 15 ++++-- 6 files changed, 56 insertions(+), 32 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java index f64e424bd9..5a3ab3b79e 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java @@ -31,6 +31,9 @@ import static com.google.gerrit.common.changes.ListChangesOption.REVIEWED; import com.google.common.base.Joiner; import com.google.common.base.Objects; +import com.google.common.cache.CacheBuilder; +import com.google.common.cache.CacheLoader; +import com.google.common.cache.LoadingCache; import com.google.common.collect.HashBasedTable; import com.google.common.collect.HashMultimap; import com.google.common.collect.ImmutableList; @@ -64,6 +67,7 @@ import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.PatchSetApproval; import com.google.gerrit.reviewdb.client.PatchSetInfo; import com.google.gerrit.reviewdb.client.PatchSetInfo.ParentInfo; +import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.UserIdentity; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.AnonymousUser; @@ -77,7 +81,8 @@ import com.google.gerrit.server.patch.PatchListNotAvailableException; import com.google.gerrit.server.patch.PatchSetInfoFactory; import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException; import com.google.gerrit.server.project.ChangeControl; -import com.google.gerrit.server.project.NoSuchChangeException; +import com.google.gerrit.server.project.NoSuchProjectException; +import com.google.gerrit.server.project.ProjectControl; import com.google.gerrit.server.query.change.ChangeData; import com.google.gwtorm.server.OrmException; import com.google.gwtorm.server.ResultSet; @@ -87,6 +92,7 @@ import com.google.inject.Provider; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import java.io.IOException; import java.sql.Timestamp; import java.util.Collection; import java.util.Collections; @@ -97,6 +103,7 @@ import java.util.List; import java.util.Map; import java.util.Set; import java.util.TreeMap; +import java.util.concurrent.ExecutionException; public class ChangeJson { private static final Logger log = LoggerFactory.getLogger(ChangeJson.class); @@ -122,7 +129,7 @@ public class ChangeJson { private final Provider userProvider; private final AnonymousUser anonymous; private final IdentifiedUser.GenericFactory userFactory; - private final ChangeControl.GenericFactory changeControlGenericFactory; + private final ProjectControl.GenericFactory projectControlFactory; private final PatchSetInfoFactory patchSetInfoFactory; private final FileInfoJson fileInfoJson; private final AccountInfo.Loader.Factory accountLoaderFactory; @@ -131,20 +138,20 @@ public class ChangeJson { private final DynamicMap> changes; private final Revisions revisions; - private ChangeControl.Factory changeControlUserFactory; private EnumSet options; private AccountInfo.Loader accountLoader; private ChangeControl lastControl; private Set reviewed; + private LoadingCache projectControls; @Inject ChangeJson( Provider db, LabelNormalizer ln, - Provider userProvider, + Provider user, AnonymousUser au, IdentifiedUser.GenericFactory uf, - ChangeControl.GenericFactory ccf, + ProjectControl.GenericFactory pcf, PatchSetInfoFactory psi, FileInfoJson fileInfoJson, AccountInfo.Loader.Factory ailf, @@ -154,10 +161,10 @@ public class ChangeJson { Revisions revisions) { this.db = db; this.labelNormalizer = ln; - this.userProvider = userProvider; + this.userProvider = user; this.anonymous = au; this.userFactory = uf; - this.changeControlGenericFactory = ccf; + this.projectControlFactory = pcf; this.patchSetInfoFactory = psi; this.fileInfoJson = fileInfoJson; this.accountLoaderFactory = ailf; @@ -167,6 +174,15 @@ public class ChangeJson { this.revisions = revisions; options = EnumSet.noneOf(ListChangesOption.class); + projectControls = CacheBuilder.newBuilder() + .concurrencyLevel(1) + .build(new CacheLoader() { + @Override + public ProjectControl load(Project.NameKey key) + throws NoSuchProjectException, IOException { + return projectControlFactory.controlFor(key, userProvider.get()); + } + }); } public ChangeJson addOption(ListChangesOption o) { @@ -179,11 +195,6 @@ public class ChangeJson { return this; } - public ChangeJson setChangeControlFactory(ChangeControl.Factory cf) { - changeControlUserFactory = cf; - return this; - } - public ChangeInfo format(ChangeResource rsrc) throws OrmException { return format(new ChangeData(rsrc.getControl())); } @@ -321,13 +332,12 @@ public class ChangeJson { } try { - if (changeControlUserFactory != null) { - ctrl = changeControlUserFactory.controlFor(cd.change(db)); - } else { - ctrl = changeControlGenericFactory.controlFor(cd.change(db), - userProvider.get()); + Change change = cd.change(db); + if (change == null) { + return null; } - } catch (NoSuchChangeException e) { + ctrl = projectControls.get(change.getProject()).controlFor(change); + } catch (ExecutionException e) { return null; } lastControl = ctrl; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangesCollection.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangesCollection.java index 45328b18fb..ecb83b48ed 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangesCollection.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangesCollection.java @@ -23,6 +23,7 @@ import com.google.gerrit.extensions.restapi.RestView; import com.google.gerrit.extensions.restapi.TopLevelResource; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.server.ReviewDb; +import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.NoSuchChangeException; import com.google.gerrit.server.query.change.QueryChanges; @@ -37,17 +38,20 @@ import java.util.List; public class ChangesCollection implements RestCollection { private final Provider db; - private final ChangeControl.Factory changeControlFactory; + private final Provider user; + private final ChangeControl.GenericFactory changeControlFactory; private final Provider queryFactory; private final DynamicMap> views; @Inject ChangesCollection( Provider dbProvider, - ChangeControl.Factory changeControlFactory, + Provider user, + ChangeControl.GenericFactory changeControlFactory, Provider queryFactory, DynamicMap> views) { this.db = dbProvider; + this.user = user; this.changeControlFactory = changeControlFactory; this.queryFactory = queryFactory; this.views = views; @@ -74,7 +78,7 @@ public class ChangesCollection implements ChangeControl control; try { - control = changeControlFactory.validateFor(changes.get(0)); + control = changeControlFactory.validateFor(changes.get(0), user.get()); } catch (NoSuchChangeException e) { throw new ResourceNotFoundException(id); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java index 7fb09b5d0f..9ed2beed78 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java @@ -31,6 +31,7 @@ import com.google.gerrit.server.config.FactoryModule; public class Module extends RestApiModule { @Override protected void configure() { + bind(ChangesCollection.class); bind(Revisions.class); bind(Reviewers.class); bind(Drafts.class); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java index d87db2ff09..698a0ac12a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java @@ -94,6 +94,15 @@ public class ChangeControl { return controlFor(change, user); } + public ChangeControl validateFor(Change change, CurrentUser user) + throws NoSuchChangeException, OrmException { + ChangeControl c = controlFor(change, user); + if (!c.isVisible(db.get())) { + throw new NoSuchChangeException(c.getChange().getId()); + } + return c; + } + public ChangeControl validateFor(Change.Id id, CurrentUser user) throws NoSuchChangeException, OrmException { ChangeControl c = controlFor(id, user); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryChanges.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryChanges.java index 44c71a95bc..4b6a5a6fc2 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryChanges.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryChanges.java @@ -24,7 +24,6 @@ import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.change.ChangeJson; import com.google.gerrit.server.change.ChangeJson.ChangeInfo; -import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.query.QueryParseException; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; @@ -82,16 +81,12 @@ public class QueryChanges implements RestReadView { } @Inject - QueryChanges(ChangeJson json, - QueryProcessor qp, - ChangeControl.Factory cf, - Provider user) { + QueryChanges(ChangeJson json, QueryProcessor qp, Provider user) { this.json = json; this.imp = qp; this.user = user; options = EnumSet.noneOf(ListChangesOption.class); - json.setChangeControlFactory(cf); } public void addQuery(String query) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryProcessor.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryProcessor.java index 5624f458ee..2f3fe540e8 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryProcessor.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryProcessor.java @@ -98,7 +98,8 @@ public class QueryProcessor { private final ChangeQueryRewriter queryRewriter; private final Provider db; private final GitRepositoryManager repoManager; - private final ChangeControl.Factory changeControlFactory; + private final ChangeControl.GenericFactory changeControlFactory; + private final CurrentUser user; private final int maxLimit; private OutputFormat outputFormat = OutputFormat.TEXT; @@ -123,13 +124,14 @@ public class QueryProcessor { ChangeQueryBuilder.Factory queryBuilder, CurrentUser currentUser, ChangeQueryRewriter queryRewriter, Provider db, GitRepositoryManager repoManager, - ChangeControl.Factory changeControlFactory) { + ChangeControl.GenericFactory changeControlFactory) { this.eventFactory = eventFactory; this.queryBuilder = queryBuilder.create(currentUser); this.queryRewriter = queryRewriter; this.db = db; this.repoManager = repoManager; this.changeControlFactory = changeControlFactory; + this.user = currentUser; this.maxLimit = currentUser.getCapabilities() .getRange(GlobalCapability.QUERY_LIMIT) .getMax(); @@ -298,8 +300,11 @@ public class QueryProcessor { List results = queryChanges(queryString); ChangeAttribute c = null; for (ChangeData d : results) { - LabelTypes labelTypes = changeControlFactory.controlFor(d.getChange()) - .getLabelTypes(); + ChangeControl cc = d.changeControl(); + if (cc == null || cc.getCurrentUser() != user) { + cc = changeControlFactory.controlFor(d.change(db), user); + } + LabelTypes labelTypes = cc.getLabelTypes(); c = eventFactory.asChangeAttribute(d.getChange()); eventFactory.extend(c, d.getChange()); eventFactory.addTrackingIds(c, d.trackingIds(db)); @@ -307,7 +312,7 @@ public class QueryProcessor { if (includeSubmitRecords) { PatchSet.Id psId = d.getChange().currentPatchSetId(); PatchSet patchSet = db.get().patchSets().get(psId); - List submitResult = d.changeControl().canSubmit( // + List submitResult = cc.canSubmit( // db.get(), patchSet, null, false, true, true); eventFactory.addSubmitRecords(c, submitResult); } From a7ef8dadbc01f77dc088cbd5aded417f0d4c5c44 Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Fri, 8 Nov 2013 15:35:37 -0800 Subject: [PATCH 5/5] Copy reviewed flag for deleted files If a file is deleted and a user marks it as reviewed, copy the reviewed mark onto newer revisions provided the same content has been deleted. Checking the content of the deleted file allows reviewers to re-examine a delete after a rebase, in case there is content in the newer deleted revision that needs to be considered to be moved elsewhere. Rewrite the copying process to iterate the 2 or 4 trees exactly once, relying on a PathFilterGroup to identify the relevant files. This should be faster on larger changes as it avoids revisting trees. It is also easier to handle the 2 additional trees for the ancestor content check on deleted files. Change-Id: Ic710ca2971e66ea919ee1c9f51eb42538174e7b3 --- .../google/gerrit/server/change/Files.java | 63 +++++++++++++------ 1 file changed, 44 insertions(+), 19 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Files.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Files.java index ec8234ffc7..98e2ee99e5 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Files.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Files.java @@ -37,7 +37,6 @@ import com.google.gerrit.server.change.FileInfoJson.FileInfo; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.patch.PatchList; import com.google.gerrit.server.patch.PatchListCache; -import com.google.gerrit.server.patch.PatchListEntry; import com.google.gerrit.server.patch.PatchListNotAvailableException; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; @@ -45,9 +44,9 @@ import com.google.inject.Provider; import org.eclipse.jgit.lib.ObjectReader; import org.eclipse.jgit.lib.Repository; -import org.eclipse.jgit.revwalk.RevTree; import org.eclipse.jgit.revwalk.RevWalk; import org.eclipse.jgit.treewalk.TreeWalk; +import org.eclipse.jgit.treewalk.filter.PathFilterGroup; import org.kohsuke.args4j.Option; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -219,23 +218,49 @@ class Files implements ChildCollection { List pathList = Lists.newArrayListWithCapacity(sz); RevWalk rw = new RevWalk(reader); - RevTree o = rw.parseCommit(oldList.getNewId()).getTree(); - RevTree c = rw.parseCommit(curList.getNewId()).getTree(); - for (PatchListEntry p : curList.getPatches()) { - String path = p.getNewName(); - if (!Patch.COMMIT_MSG.equals(path) && paths.contains(path)) { - TreeWalk tw = TreeWalk.forPath(reader, path, o, c); - if (tw != null - && tw.getRawMode(0) != 0 - && tw.getRawMode(1) != 0 - && tw.idEqual(0, 1)) { - inserts.add(new AccountPatchReview( - new Patch.Key( - resource.getPatchSet().getId(), - path), - userId)); - pathList.add(path); - } + TreeWalk tw = new TreeWalk(reader); + tw.setFilter(PathFilterGroup.createFromStrings(paths)); + tw.setRecursive(true); + int o = tw.addTree(rw.parseCommit(oldList.getNewId()).getTree()); + int c = tw.addTree(rw.parseCommit(curList.getNewId()).getTree()); + + int op = -1; + if (oldList.getOldId() != null) { + op = tw.addTree(rw.parseCommit(oldList.getOldId()).getTree()); + } + + int cp = -1; + if (curList.getOldId() != null) { + cp = tw.addTree(rw.parseCommit(curList.getOldId()).getTree()); + } + + while (tw.next()) { + String path = tw.getPathString(); + if (tw.getRawMode(o) != 0 && tw.getRawMode(c) != 0 + && tw.idEqual(o, c) + && paths.contains(path)) { + // File exists in previously reviewed oldList and in curList. + // File content is identical. + inserts.add(new AccountPatchReview( + new Patch.Key( + resource.getPatchSet().getId(), + path), + userId)); + pathList.add(path); + } else if (op >= 0 && cp >= 0 + && tw.getRawMode(o) == 0 && tw.getRawMode(c) == 0 + && tw.getRawMode(op) != 0 && tw.getRawMode(cp) != 0 + && tw.idEqual(op, cp) + && paths.contains(path)) { + // File was deleted in previously reviewed oldList and curList. + // File exists in ancestor of oldList and curList. + // File content is identical in ancestors. + inserts.add(new AccountPatchReview( + new Patch.Key( + resource.getPatchSet().getId(), + path), + userId)); + pathList.add(path); } } db.get().accountPatchReviews().insert(inserts);