diff --git a/Documentation/images/intro-quick-new-review.jpg b/Documentation/images/intro-quick-new-review.jpg deleted file mode 100644 index 99e6c55661..0000000000 Binary files a/Documentation/images/intro-quick-new-review.jpg and /dev/null differ diff --git a/Documentation/images/intro-quick-new-review.png b/Documentation/images/intro-quick-new-review.png new file mode 100644 index 0000000000..36d93e91e6 Binary files /dev/null and b/Documentation/images/intro-quick-new-review.png differ diff --git a/Documentation/images/intro-quick-review-2-patches.jpg b/Documentation/images/intro-quick-review-2-patches.jpg deleted file mode 100644 index 29c99cc94b..0000000000 Binary files a/Documentation/images/intro-quick-review-2-patches.jpg and /dev/null differ diff --git a/Documentation/images/intro-quick-review-2-patches.png b/Documentation/images/intro-quick-review-2-patches.png new file mode 100644 index 0000000000..d7e9129412 Binary files /dev/null and b/Documentation/images/intro-quick-review-2-patches.png differ diff --git a/Documentation/images/intro-quick-review-line-comment.jpg b/Documentation/images/intro-quick-review-line-comment.jpg deleted file mode 100644 index eeb144af2d..0000000000 Binary files a/Documentation/images/intro-quick-review-line-comment.jpg and /dev/null differ diff --git a/Documentation/images/intro-quick-review-line-comment.png b/Documentation/images/intro-quick-review-line-comment.png new file mode 100644 index 0000000000..796436579a Binary files /dev/null and b/Documentation/images/intro-quick-review-line-comment.png differ diff --git a/Documentation/images/intro-quick-reviewing-the-change.jpg b/Documentation/images/intro-quick-reviewing-the-change.jpg deleted file mode 100644 index bfded9eab4..0000000000 Binary files a/Documentation/images/intro-quick-reviewing-the-change.jpg and /dev/null differ diff --git a/Documentation/images/intro-quick-reviewing-the-change.png b/Documentation/images/intro-quick-reviewing-the-change.png new file mode 100644 index 0000000000..bdce6bdf1b Binary files /dev/null and b/Documentation/images/intro-quick-reviewing-the-change.png differ diff --git a/Documentation/images/intro-quick-verifying.jpg b/Documentation/images/intro-quick-verifying.jpg deleted file mode 100644 index 7679c0a548..0000000000 Binary files a/Documentation/images/intro-quick-verifying.jpg and /dev/null differ diff --git a/Documentation/images/intro-quick-verifying.png b/Documentation/images/intro-quick-verifying.png new file mode 100644 index 0000000000..e343cc95ee Binary files /dev/null and b/Documentation/images/intro-quick-verifying.png differ diff --git a/Documentation/intro-gerrit-walkthrough.txt b/Documentation/intro-gerrit-walkthrough.txt index fcb4de21a7..1fba1dc04b 100644 --- a/Documentation/intro-gerrit-walkthrough.txt +++ b/Documentation/intro-gerrit-walkthrough.txt @@ -4,7 +4,7 @@ To understand how Gerrit works, let's follow a change through its entire life cycle. This example uses a Gerrit server configured as follows: * *Hostname*: gerrithost -* *HTTP interface port*: 8080 +* *HTTP interface port*: 80 * *SSH interface port*: 29418 In this walkthrough, we'll follow two developers, Max and Hannah, as they make @@ -52,19 +52,20 @@ follows: ---- $ $ git commit -[master 9651f22] Change to a proper, yeast based pizza dough. - 1 files changed, 3 insertions(+), 2 deletions(-) +[master 3cc9e62] Change to a proper, yeast based pizza dough. + 1 file changed, 10 insertions(+), 5 deletions(-) $ git push origin HEAD:refs/for/master -Counting objects: 5, done. +Counting objects: 3, done. Delta compression using up to 8 threads. Compressing objects: 100% (2/2), done. -Writing objects: 100% (3/3), 542 bytes, done. +Writing objects: 100% (3/3), 532 bytes | 0 bytes/s, done. Total 3 (delta 0), reused 0 (delta 0) +remote: Processing changes: new: 1, done remote: remote: New Changes: -remote: http://gerrithost:8080/68 +remote: http://gerrithost/#/c/RecipeBook/+/702 Change to a proper, yeast based pizza dough. remote: -To ssh://gerrithost:29418/RecipeBook.git +To ssh://gerrithost:29418/RecipeBook * [new branch] HEAD -> refs/for/master ---- @@ -79,7 +80,7 @@ review this commit. Clicking on that link takes him to a screen similar to the following. .Gerrit Code Review Screen -image::images/intro-quick-new-review.jpg[Gerrit Review Screen] +image::images/intro-quick-new-review.png[Gerrit Review Screen] This is the Gerrit code review screen, where other contributors can review his change. Max can also perform tasks such as: @@ -109,14 +110,12 @@ offers other ways for reviewers to find changes, including: Because Max added Hannah as a reviewer, she receives an email telling her about his change. She opens up the Gerrit code review screen and selects Max's change. -.Gerrit Code Review Screen -image::images/intro-quick-new-review.jpg[Gerrit Review Screen] - -Notice the two "Need" lines: +Notice the *Label status* section above: ---- -* Need Verified -* Need Code-Review +Label Status Needs label: + * Code-Review + * Verified ---- These two lines indicate what checks must be completed before the change is @@ -147,13 +146,13 @@ link:user-review-ui.html#reply[summary] comments. Hannah opts to view the change using Gerrit's side-by-side view: .Side By Side Patch View -image::images/intro-quick-review-line-comment.jpg[Adding a Comment] +image::images/intro-quick-review-line-comment.png[Adding a Comment] Hannah reviews the change and is ready to provide her feedback. She clicks the -*Review* button on the change screen. This allows her to vote on the change. +*REPLY* button on the change screen. This allows her to vote on the change. .Reviewing the Change -image::images/intro-quick-reviewing-the-change.jpg[Reviewing the Change] +image::images/intro-quick-reviewing-the-change.png[Reviewing the Change] For Hannah and Max's team, a code review vote is a numerical score between -2 and 2. The possible options are: @@ -175,7 +174,7 @@ link:config-project-config.html[Project Configuration File Format] topic. Hannah notices a possible issue with Max's change, so she selects a `-1` vote. She uses the *Cover Message* text box to provide Max with some additional feedback. When she is satisfied with her review, Hannah clicks the -*Publish Comments* button. At this point, her vote and cover message become +*SEND* button. At this point, her vote and cover message become visible to to all users. == Reworking the Change @@ -193,18 +192,21 @@ workflow for updating a commit: $ $ $ git commit --amend +[master 30a6f44] Change to a proper, yeast based pizza dough. + Date: Fri Jun 8 16:28:23 2018 +0200 + 1 file changed, 10 insertions(+), 5 deletions(-) $ git push origin HEAD:refs/for/master -Counting objects: 5, done. +Counting objects: 3, done. Delta compression using up to 8 threads. Compressing objects: 100% (2/2), done. -Writing objects: 100% (3/3), 546 bytes, done. +Writing objects: 100% (3/3), 528 bytes | 0 bytes/s, done. Total 3 (delta 0), reused 0 (delta 0) remote: Processing changes: updated: 1, done remote: remote: Updated Changes: -remote: http://gerrithost:8080/68 +remote: http://gerrithost/#/c/RecipeBook/+/702 Change to a proper, yeast based pizza dough. remote: -To ssh://gerrithost:29418/RecipeBook.git +To ssh://gerrithost:29418/RecipeBook * [new branch] HEAD -> refs/for/master ---- @@ -212,13 +214,10 @@ Notice that the output of this command is slightly different from Max's first commit. This time, the output verifies that the change was updated. Having uploaded the reworked commit, Max can go back to the Gerrit web -interface and look at his change. - -.Reviewing the Rework -image::images/intro-quick-review-2-patches.jpg[Reviewing the Rework] - -Notice that there are now two patch sets associated with this change: the -initial submission and the rework. +interface, look at his change and diff the first patch set with his rework in +the second one. Once he has verified that the rework follows Hannahs +recommendation he presses the *DONE* button to let Hannah know that she can +review the changes. When Hannah next looks at Max's change, she sees that he incorporated her feedback. The change looks good to her, so she changes her vote to a `+2`. @@ -254,7 +253,7 @@ NOTE: The Verifier can be the same person as the code reviewer or a different person entirely. .Verifying the Change -image::images/intro-quick-verifying.jpg[Verifying the Change] +image::images/intro-quick-verifying.png[Verifying the Change] Unlike the code review check, the verify check is pass/fail. Hannah can provide a score of either `+1` or `-1`. A change must have at least one `+1` and no @@ -266,7 +265,7 @@ submitted. == Submitting the Change Max is now ready to submit his change. He opens up the change in the Code Review -screen and clicks the *Publish and Submit* button. +screen and clicks the *SUBMIT* button. At this point, Max's change is merged into the repository's master branch and becomes an accepted part of the project. diff --git a/Documentation/intro-user.txt b/Documentation/intro-user.txt index f5042a7a68..436408df0e 100644 --- a/Documentation/intro-user.txt +++ b/Documentation/intro-user.txt @@ -548,6 +548,9 @@ request. ---- Alternatively, click *Ready* from the Change screen. +Only change owners, project owners and site administrators can mark changes as +`work-in-progress` and `ready`. + [[wip-polygerrit]] In the new PolyGerrit UI, you can mark a change as WIP, by selecting *WIP* from the *More* menu. The Change screen updates with a yellow header, indicating that diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index 8d1b2d8152..e64bdb3d5a 100644 --- a/Documentation/rest-api-changes.txt +++ b/Documentation/rest-api-changes.txt @@ -2145,7 +2145,8 @@ Only the change owner, a project owner, or an administrator may fix changes. 'POST /changes/link:#change-id[\{change-id\}]/wip' -- -Marks the change as not ready for review yet. +Marks the change as not ready for review yet. Changes may only be marked not +ready by the owner, project owners or site administrators. The request body does not need to include a link:#work-in-progress-input[WorkInProgressInput] entity if no review comment @@ -2173,7 +2174,8 @@ notifying *OWNER* instead of *ALL*. 'POST /changes/link:#change-id[\{change-id\}]/ready' -- -Marks the change as ready for review (set WIP property to false). +Marks the change as ready for review (set WIP property to false). Changes may +only be marked ready by the owner, project owners or site administrators. Activates notifications of reviewer. The request body does not need to include a link:#work-in-progress-input[WorkInProgressInput] entity diff --git a/Documentation/user-upload.txt b/Documentation/user-upload.txt index c12a38ce86..ce62b939ec 100644 --- a/Documentation/user-upload.txt +++ b/Documentation/user-upload.txt @@ -306,6 +306,9 @@ flag from a change on push, explicitly specify the `ready` option: git push ssh://john.doe@git.example.com:29418/kernel/common HEAD:refs/for/master%ready ---- +Only change owners, project owners and site administrators can specify +`work-in-progress` and `ready` options on push. + [[message]] ==== Message diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/AccountConstants.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/AccountConstants.java index 9799723238..0b32cd5839 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/AccountConstants.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/AccountConstants.java @@ -129,6 +129,8 @@ public interface AccountConstants extends Constants { String buttonGeneratePassword(); + String revokePassword(); + String linkObtainPassword(); String linkEditFullName(); diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/AccountConstants.properties b/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/AccountConstants.properties index ca7cc27245..4b01513619 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/AccountConstants.properties +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/AccountConstants.properties @@ -74,6 +74,7 @@ confirmSetUserNameTitle = Confirm Setting the Username confirmSetUserName = Setting the Username is permanent. Are you sure? buttonClearPassword = Clear Password buttonGeneratePassword = Generate Password +revokePassword = (click 'Generate Password' to revoke an old password) linkObtainPassword = Obtain Password linkEditFullName = Edit linkReloadContact = Reload diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/MyPasswordScreen.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/MyPasswordScreen.java index 3852387c91..5dd7530e44 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/MyPasswordScreen.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/MyPasswordScreen.java @@ -49,7 +49,7 @@ public class MyPasswordScreen extends SettingsScreen { return; } - password = new CopyableLabel("(click 'generate' to revoke an old password)"); + password = new CopyableLabel(Util.C.revokePassword()); password.addStyleName(Gerrit.RESOURCES.css().accountPassword()); generatePassword = new Button(Util.C.buttonGeneratePassword()); diff --git a/java/com/google/gerrit/elasticsearch/AbstractElasticIndex.java b/java/com/google/gerrit/elasticsearch/AbstractElasticIndex.java index 0ccd82001f..3755faa622 100644 --- a/java/com/google/gerrit/elasticsearch/AbstractElasticIndex.java +++ b/java/com/google/gerrit/elasticsearch/AbstractElasticIndex.java @@ -118,7 +118,8 @@ abstract class AbstractElasticIndex implements Index { SitePaths sitePaths, Schema schema, ElasticRestClientProvider client, - String indexName) { + String indexName, + String indexType) { this.sitePaths = sitePaths; this.schema = schema; this.gson = new GsonBuilder().setFieldNamingPolicy(LOWER_CASE_WITH_UNDERSCORES).create(); @@ -126,7 +127,16 @@ abstract class AbstractElasticIndex implements Index { this.indexName = cfg.getIndexName(indexName, schema.getVersion()); this.indexNameRaw = indexName; this.client = client; - this.type = client.adapter().getType(indexName); + this.type = client.adapter().getType(indexType); + } + + AbstractElasticIndex( + ElasticConfiguration cfg, + SitePaths sitePaths, + Schema schema, + ElasticRestClientProvider client, + String indexName) { + this(cfg, sitePaths, schema, client, indexName, indexName); } @Override diff --git a/java/com/google/gerrit/elasticsearch/ElasticAccountIndex.java b/java/com/google/gerrit/elasticsearch/ElasticAccountIndex.java index 58f4fb9999..d18af423f5 100644 --- a/java/com/google/gerrit/elasticsearch/ElasticAccountIndex.java +++ b/java/com/google/gerrit/elasticsearch/ElasticAccountIndex.java @@ -46,14 +46,14 @@ import org.elasticsearch.client.Response; public class ElasticAccountIndex extends AbstractElasticIndex implements AccountIndex { static class AccountMapping { - MappingProperties accounts; + final MappingProperties accounts; AccountMapping(Schema schema, ElasticQueryAdapter adapter) { this.accounts = ElasticMapping.createMapping(schema, adapter); } } - static final String ACCOUNTS = "accounts"; + private static final String ACCOUNTS = "accounts"; private final AccountMapping mapping; private final Provider accountCache; diff --git a/java/com/google/gerrit/elasticsearch/ElasticChangeIndex.java b/java/com/google/gerrit/elasticsearch/ElasticChangeIndex.java index 1ec8d2b4bc..f6af79f228 100644 --- a/java/com/google/gerrit/elasticsearch/ElasticChangeIndex.java +++ b/java/com/google/gerrit/elasticsearch/ElasticChangeIndex.java @@ -76,9 +76,9 @@ import org.elasticsearch.client.Response; class ElasticChangeIndex extends AbstractElasticIndex implements ChangeIndex { static class ChangeMapping { - MappingProperties changes; - MappingProperties openChanges; - MappingProperties closedChanges; + final MappingProperties changes; + final MappingProperties openChanges; + final MappingProperties closedChanges; ChangeMapping(Schema schema, ElasticQueryAdapter adapter) { MappingProperties mapping = ElasticMapping.createMapping(schema, adapter); @@ -88,9 +88,9 @@ class ElasticChangeIndex extends AbstractElasticIndex } } - static final String CHANGES = "changes"; - static final String OPEN_CHANGES = "open_" + CHANGES; - static final String CLOSED_CHANGES = "closed_" + CHANGES; + private static final String CHANGES = "changes"; + private static final String OPEN_CHANGES = "open_" + CHANGES; + private static final String CLOSED_CHANGES = "closed_" + CHANGES; private final ChangeMapping mapping; private final Provider db; diff --git a/java/com/google/gerrit/elasticsearch/ElasticConfiguration.java b/java/com/google/gerrit/elasticsearch/ElasticConfiguration.java index 84dae7f238..4184ec06ce 100644 --- a/java/com/google/gerrit/elasticsearch/ElasticConfiguration.java +++ b/java/com/google/gerrit/elasticsearch/ElasticConfiguration.java @@ -80,11 +80,11 @@ class ElasticConfiguration { } } - public Config getConfig() { + Config getConfig() { return cfg; } - public String getIndexName(String name, int schemaVersion) { + String getIndexName(String name, int schemaVersion) { return String.format("%s%s_%04d", prefix, name, schemaVersion); } diff --git a/java/com/google/gerrit/elasticsearch/ElasticGroupIndex.java b/java/com/google/gerrit/elasticsearch/ElasticGroupIndex.java index cf1a4ed552..bf6b962304 100644 --- a/java/com/google/gerrit/elasticsearch/ElasticGroupIndex.java +++ b/java/com/google/gerrit/elasticsearch/ElasticGroupIndex.java @@ -44,14 +44,14 @@ import org.elasticsearch.client.Response; public class ElasticGroupIndex extends AbstractElasticIndex implements GroupIndex { static class GroupMapping { - MappingProperties groups; + final MappingProperties groups; GroupMapping(Schema schema, ElasticQueryAdapter adapter) { this.groups = ElasticMapping.createMapping(schema, adapter); } } - static final String GROUPS = "groups"; + private static final String GROUPS = "groups"; private final GroupMapping mapping; private final Provider groupCache; diff --git a/java/com/google/gerrit/elasticsearch/ElasticIndexVersionManager.java b/java/com/google/gerrit/elasticsearch/ElasticIndexVersionManager.java index 58272f7874..11d21dc20f 100644 --- a/java/com/google/gerrit/elasticsearch/ElasticIndexVersionManager.java +++ b/java/com/google/gerrit/elasticsearch/ElasticIndexVersionManager.java @@ -63,7 +63,7 @@ public class ElasticIndexVersionManager extends VersionManager { logger.atWarning().log("Unrecognized version in index %s: %s", def.getName(), version); continue; } - versions.put(v, new Version(null, v, true, cfg.getReady(def.getName(), v))); + versions.put(v, new Version<>(null, v, true, cfg.getReady(def.getName(), v))); } } catch (IOException e) { logger.atSevere().withCause(e).log("Error scanning index: %s", def.getName()); diff --git a/java/com/google/gerrit/elasticsearch/ElasticQueryBuilder.java b/java/com/google/gerrit/elasticsearch/ElasticQueryBuilder.java index 2a97e2ef39..394158dbc2 100644 --- a/java/com/google/gerrit/elasticsearch/ElasticQueryBuilder.java +++ b/java/com/google/gerrit/elasticsearch/ElasticQueryBuilder.java @@ -34,7 +34,7 @@ import java.time.Instant; public class ElasticQueryBuilder { - protected QueryBuilder toQueryBuilder(Predicate p) throws QueryParseException { + QueryBuilder toQueryBuilder(Predicate p) throws QueryParseException { if (p instanceof AndPredicate) { return and(p); } else if (p instanceof OrPredicate) { diff --git a/java/com/google/gerrit/elasticsearch/ElasticVersion.java b/java/com/google/gerrit/elasticsearch/ElasticVersion.java index ff26382a93..b65eb317f1 100644 --- a/java/com/google/gerrit/elasticsearch/ElasticVersion.java +++ b/java/com/google/gerrit/elasticsearch/ElasticVersion.java @@ -25,7 +25,7 @@ public enum ElasticVersion { private final String version; private final Pattern pattern; - private ElasticVersion(String version) { + ElasticVersion(String version) { this.version = version; this.pattern = Pattern.compile(version); } diff --git a/java/com/google/gerrit/server/change/ActionJson.java b/java/com/google/gerrit/server/change/ActionJson.java index 887923530e..fbabdd5d24 100644 --- a/java/com/google/gerrit/server/change/ActionJson.java +++ b/java/com/google/gerrit/server/change/ActionJson.java @@ -139,6 +139,7 @@ public class ActionJson { copy.stars = changeInfo.stars; copy.submitted = changeInfo.submitted; copy.submitter = changeInfo.submitter; + copy.workInProgress = changeInfo.workInProgress; copy.id = changeInfo.id; return copy; } diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java index a91fac5591..3f59d5c8bc 100644 --- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java +++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java @@ -23,7 +23,7 @@ import static com.google.gerrit.reviewdb.client.RefNames.isConfigRef; import static com.google.gerrit.server.change.HashtagsUtil.cleanupHashtag; import static com.google.gerrit.server.git.MultiProgressMonitor.UNKNOWN; import static com.google.gerrit.server.git.receive.ReceiveConstants.COMMAND_REJECTION_MESSAGE_FOOTER; -import static com.google.gerrit.server.git.receive.ReceiveConstants.ONLY_OWNER_CAN_MODIFY_WIP; +import static com.google.gerrit.server.git.receive.ReceiveConstants.ONLY_CHANGE_OWNER_OR_PROJECT_OWNER_CAN_MODIFY_WIP; import static com.google.gerrit.server.git.receive.ReceiveConstants.PUSH_OPTION_SKIP_VALIDATION; import static com.google.gerrit.server.git.receive.ReceiveConstants.SAME_CHANGE_ID_IN_MULTIPLE_CHANGES; import static com.google.gerrit.server.git.validators.CommitValidators.NEW_PATCHSET_PATTERN; @@ -2528,8 +2528,10 @@ class ReceiveCommits { if (magicBranch != null && (magicBranch.workInProgress || magicBranch.ready) && magicBranch.workInProgress != change.isWorkInProgress() - && !user.getAccountId().equals(change.getOwner())) { - reject(inputCommand, ONLY_OWNER_CAN_MODIFY_WIP); + && (!user.getAccountId().equals(change.getOwner()) + && !permissions.test(ProjectPermission.WRITE_CONFIG) + && !permissionBackend.user(user).test(GlobalPermission.ADMINISTRATE_SERVER))) { + reject(inputCommand, ONLY_CHANGE_OWNER_OR_PROJECT_OWNER_CAN_MODIFY_WIP); return false; } diff --git a/java/com/google/gerrit/server/git/receive/ReceiveConstants.java b/java/com/google/gerrit/server/git/receive/ReceiveConstants.java index 92723e077e..b71f01e1e5 100644 --- a/java/com/google/gerrit/server/git/receive/ReceiveConstants.java +++ b/java/com/google/gerrit/server/git/receive/ReceiveConstants.java @@ -20,8 +20,8 @@ public final class ReceiveConstants { public static final String PUSH_OPTION_SKIP_VALIDATION = "skip-validation"; @VisibleForTesting - public static final String ONLY_OWNER_CAN_MODIFY_WIP = - "only change owner can modify Work-in-Progress"; + public static final String ONLY_CHANGE_OWNER_OR_PROJECT_OWNER_CAN_MODIFY_WIP = + "only change owner or project owner can modify Work-in-Progress"; static final String COMMAND_REJECTION_MESSAGE_FOOTER = "Please read the documentation and contact an administrator\n" diff --git a/java/com/google/gerrit/server/restapi/change/Move.java b/java/com/google/gerrit/server/restapi/change/Move.java index 5fcf967b48..8d144faa08 100644 --- a/java/com/google/gerrit/server/restapi/change/Move.java +++ b/java/com/google/gerrit/server/restapi/change/Move.java @@ -21,6 +21,7 @@ import static com.google.gerrit.server.query.change.ChangeData.asChanges; import com.google.common.base.Strings; import com.google.common.flogger.FluentLogger; +import com.google.gerrit.common.Nullable; import com.google.gerrit.common.TimeUtil; import com.google.gerrit.common.data.LabelType; import com.google.gerrit.extensions.api.changes.MoveInput; @@ -145,12 +146,13 @@ public class Move extends RetryingRestModifyView revisionFilePaths = getAffectedFilePaths(revision); for (Map.Entry> entry : commentsPerPath.entrySet()) { String path = entry.getKey(); - PatchSet.Id patchSetId = revision.getChange().currentPatchSetId(); + PatchSet.Id patchSetId = revision.getPatchSet().getId(); ensurePathRefersToAvailableOrMagicFile(path, revisionFilePaths, patchSetId); List comments = entry.getValue(); diff --git a/java/com/google/gerrit/server/restapi/change/SetReadyForReview.java b/java/com/google/gerrit/server/restapi/change/SetReadyForReview.java index 5298857f16..8fe56120dd 100644 --- a/java/com/google/gerrit/server/restapi/change/SetReadyForReview.java +++ b/java/com/google/gerrit/server/restapi/change/SetReadyForReview.java @@ -33,6 +33,7 @@ import com.google.gerrit.server.change.WorkInProgressOp.Input; import com.google.gerrit.server.permissions.GlobalPermission; import com.google.gerrit.server.permissions.PermissionBackend; import com.google.gerrit.server.permissions.PermissionBackendException; +import com.google.gerrit.server.permissions.ProjectPermission; import com.google.gerrit.server.update.BatchUpdate; import com.google.gerrit.server.update.RetryHelper; import com.google.gerrit.server.update.RetryingRestModifyView; @@ -66,7 +67,11 @@ public class SetReadyForReview extends RetryingRestModifyView> comments = new HashMap<>(); + comments.put("non-existing.txt", Collections.singletonList(in)); + reviewInput.comments = comments; + reviewInput.message = "comment test"; + + exception.expect(BadRequestException.class); + exception.expectMessage( + String.format("not found in revision %d,1", r.getChange().change().getId().id)); + gApi.changes().id(r.getChangeId()).revision(1).review(reviewInput); + } + @Test public void patch() throws Exception { PushOneCommit.Result r = createChange(); diff --git a/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java b/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java index 8cc5c004d4..cfa7ec4dd4 100644 --- a/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java +++ b/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java @@ -656,11 +656,11 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { assertThat(r.getChange().change().getOwner()).isEqualTo(user.id); assertThat(r.getChange().change().isWorkInProgress()).isTrue(); - // Other user trying to move from WIP to ready should fail. + // Admin user trying to move from WIP to ready should succeed. GitUtil.fetch(testRepo, r.getPatchSet().getRefName() + ":ps"); testRepo.reset("ps"); - r = amendChange(r.getChangeId(), "refs/for/master%ready", admin, testRepo); - r.assertErrorStatus(ReceiveConstants.ONLY_OWNER_CAN_MODIFY_WIP); + r = amendChange(r.getChangeId(), "refs/for/master%ready", user, testRepo); + r.assertOkStatus(); // Other user trying to move from WIP to WIP should succeed. r = amendChange(r.getChangeId(), "refs/for/master%wip", admin, testRepo); @@ -672,14 +672,29 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { r.assertOkStatus(); assertThat(r.getChange().change().isWorkInProgress()).isFalse(); - // Other user trying to move from ready to WIP should fail. + // Admin user trying to move from ready to WIP should succeed. GitUtil.fetch(testRepo, r.getPatchSet().getRefName() + ":ps"); testRepo.reset("ps"); r = amendChange(r.getChangeId(), "refs/for/master%wip", admin, testRepo); - r.assertErrorStatus(ReceiveConstants.ONLY_OWNER_CAN_MODIFY_WIP); + r.assertOkStatus(); - // Other user trying to move from ready to ready should succeed. - r = amendChange(r.getChangeId(), "refs/for/master%ready", admin, testRepo); + // Other user trying to move from wip to wip should succeed. + r = amendChange(r.getChangeId(), "refs/for/master%wip", admin, testRepo); + r.assertOkStatus(); + + // Non owner, non admin and non project owner cannot flip wip bit: + TestAccount user2 = accountCreator.user2(); + grant( + project, "refs/*", Permission.FORGE_COMMITTER, false, SystemGroupBackend.REGISTERED_USERS); + TestRepository user2Repo = cloneProject(project, user2); + GitUtil.fetch(user2Repo, r.getPatchSet().getRefName() + ":ps"); + user2Repo.reset("ps"); + r = amendChange(r.getChangeId(), "refs/for/master%ready", user2, user2Repo); + r.assertErrorStatus(ReceiveConstants.ONLY_CHANGE_OWNER_OR_PROJECT_OWNER_CAN_MODIFY_WIP); + + // Project owner trying to move from WIP to ready should succeed. + allow("refs/*", Permission.OWNER, SystemGroupBackend.REGISTERED_USERS); + r = amendChange(r.getChangeId(), "refs/for/master%ready", user2, user2Repo); r.assertOkStatus(); } diff --git a/javatests/com/google/gerrit/acceptance/ssh/AbstractIndexTests.java b/javatests/com/google/gerrit/acceptance/ssh/AbstractIndexTests.java new file mode 100644 index 0000000000..c72edfb77f --- /dev/null +++ b/javatests/com/google/gerrit/acceptance/ssh/AbstractIndexTests.java @@ -0,0 +1,67 @@ +// Copyright (C) 2018 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.acceptance.ssh; + +import static com.google.common.truth.Truth.assertThat; + +import com.google.common.base.Joiner; +import com.google.common.collect.FluentIterable; +import com.google.gerrit.acceptance.AbstractDaemonTest; +import com.google.gerrit.acceptance.NoHttpd; +import com.google.gerrit.acceptance.PushOneCommit; +import com.google.gerrit.acceptance.UseSsh; +import com.google.gerrit.extensions.common.ChangeInfo; +import com.google.gerrit.server.query.change.ChangeData; +import com.google.inject.Injector; +import java.util.List; +import org.junit.Test; + +@NoHttpd +@UseSsh +public abstract class AbstractIndexTests extends AbstractDaemonTest { + /** @param injector injector */ + public abstract void configureIndex(Injector injector) throws Exception; + + @Test + public void indexChange() throws Exception { + configureIndex(server.getTestInjector()); + + PushOneCommit.Result change = createChange("first change", "test1.txt", "test1"); + String changeId = change.getChangeId(); + String changeLegacyId = change.getChange().getId().toString(); + + disableChangeIndexWrites(); + amendChange(changeId, "second test", "test2.txt", "test2"); + + assertQuery("message:second", change.getChange(), false); + enableChangeIndexWrites(); + + String cmd = Joiner.on(" ").join("gerrit", "index", "changes", changeLegacyId); + adminSshSession.exec(cmd); + + assertQuery("message:second", change.getChange(), true); + } + + protected void assertQuery(String q, ChangeData change, Boolean assertTrue) throws Exception { + List result = query(q); + Iterable ids = ids(result); + if (assertTrue) assertThat(ids).contains(change.getId().get()); + else assertThat(ids).doesNotContain(change.getId().get()); + } + + protected static Iterable ids(Iterable changes) { + return FluentIterable.from(changes).transform(in -> in._number); + } +} diff --git a/javatests/com/google/gerrit/acceptance/ssh/BUILD b/javatests/com/google/gerrit/acceptance/ssh/BUILD index 87b2920cad..eefd9d3b8e 100644 --- a/javatests/com/google/gerrit/acceptance/ssh/BUILD +++ b/javatests/com/google/gerrit/acceptance/ssh/BUILD @@ -1,9 +1,38 @@ load("//javatests/com/google/gerrit/acceptance:tests.bzl", "acceptance_tests") +java_library( + name = "util", + testonly = 1, + srcs = ["AbstractIndexTests.java"], + deps = ["//java/com/google/gerrit/acceptance:lib"], +) + acceptance_tests( - srcs = glob(["*IT.java"]), + srcs = glob( + ["*IT.java"], + exclude = ["ElasticIndexIT.java"], + ), group = "ssh", labels = ["ssh"], vm_args = ["-Xmx512m"], - deps = ["//lib/commons:compress"], + deps = [ + ":util", + "//lib/commons:compress", + ], +) + +acceptance_tests( + srcs = ["ElasticIndexIT.java"], + group = "elastic", + labels = [ + "elastic", + "docker", + "ssh", + ], + deps = [ + ":util", + "//java/com/google/gerrit/elasticsearch", + "//javatests/com/google/gerrit/elasticsearch:elasticsearch_test_utils", + "//lib/commons:compress", + ], ) diff --git a/javatests/com/google/gerrit/acceptance/ssh/ElasticIndexIT.java b/javatests/com/google/gerrit/acceptance/ssh/ElasticIndexIT.java new file mode 100644 index 0000000000..bd0d67f03a --- /dev/null +++ b/javatests/com/google/gerrit/acceptance/ssh/ElasticIndexIT.java @@ -0,0 +1,62 @@ +// Copyright (C) 2018 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.acceptance.ssh; + +import com.google.gerrit.elasticsearch.ElasticContainer; +import com.google.gerrit.elasticsearch.ElasticTestUtils; +import com.google.gerrit.elasticsearch.ElasticTestUtils.ElasticNodeInfo; +import com.google.gerrit.elasticsearch.ElasticVersion; +import com.google.gerrit.testing.ConfigSuite; +import com.google.inject.Injector; +import java.util.UUID; +import org.eclipse.jgit.lib.Config; + +public class ElasticIndexIT extends AbstractIndexTests { + private static ElasticContainer container; + + private static Config getConfig(ElasticVersion version) { + ElasticNodeInfo elasticNodeInfo; + try { + container = ElasticContainer.createAndStart(version); + elasticNodeInfo = new ElasticNodeInfo(container.getHttpHost().getPort()); + } catch (Throwable t) { + return null; + } + String indicesPrefix = UUID.randomUUID().toString(); + Config cfg = new Config(); + ElasticTestUtils.configure(cfg, elasticNodeInfo.port, indicesPrefix); + return cfg; + } + + @ConfigSuite.Default + public static Config elasticsearchV2() { + return getConfig(ElasticVersion.V2_4); + } + + @ConfigSuite.Config + public static Config elasticsearchV5() { + return getConfig(ElasticVersion.V5_6); + } + + @ConfigSuite.Config + public static Config elasticsearchV6() { + return getConfig(ElasticVersion.V6_2); + } + + @Override + public void configureIndex(Injector injector) throws Exception { + ElasticTestUtils.createAllIndexes(injector); + } +} diff --git a/javatests/com/google/gerrit/acceptance/ssh/IndexIT.java b/javatests/com/google/gerrit/acceptance/ssh/IndexIT.java new file mode 100644 index 0000000000..196a1e5e2d --- /dev/null +++ b/javatests/com/google/gerrit/acceptance/ssh/IndexIT.java @@ -0,0 +1,23 @@ +// Copyright (C) 2018 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.acceptance.ssh; + +import com.google.inject.Injector; + +public class IndexIT extends AbstractIndexTests { + + @Override + public void configureIndex(Injector injector) throws Exception {} +} diff --git a/javatests/com/google/gerrit/elasticsearch/ElasticContainer.java b/javatests/com/google/gerrit/elasticsearch/ElasticContainer.java index c78f7c01d9..df15d8ff5c 100644 --- a/javatests/com/google/gerrit/elasticsearch/ElasticContainer.java +++ b/javatests/com/google/gerrit/elasticsearch/ElasticContainer.java @@ -65,7 +65,7 @@ public class ElasticContainer> extends Gener } @Override - protected Set getLivenessCheckPorts() { + public Set getLivenessCheckPortNumbers() { return ImmutableSet.of(getMappedPort(ELASTICSEARCH_DEFAULT_PORT)); } diff --git a/javatests/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java b/javatests/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java index f6d2568b9c..bc3c9a92fc 100644 --- a/javatests/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java +++ b/javatests/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java @@ -71,6 +71,7 @@ import com.google.gerrit.server.extensions.events.GitReferenceUpdated; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.meta.MetaDataUpdate; import com.google.gerrit.server.index.account.AccountField; +import com.google.gerrit.server.index.account.AccountIndex; import com.google.gerrit.server.index.account.AccountIndexCollection; import com.google.gerrit.server.index.account.AccountIndexer; import com.google.gerrit.server.schema.SchemaCreator; @@ -448,6 +449,18 @@ public abstract class AbstractQueryAccountsTest extends GerritServerTests { assertAccounts(queryProvider.get().byWatchedProject(allProjects), user3); } + @Test + public void byDeletedAccount() throws Exception { + AccountInfo user = newAccountWithFullName("jdoe", "John Doe"); + Account.Id userId = Account.Id.tryParse(user._accountId.toString()).get(); + assertQuery("John", user); + + for (AccountIndex index : indexes.getWriteIndexes()) { + index.delete(userId); + } + assertQuery("John"); + } + @Test public void withLimit() throws Exception { String domain = name("test.com"); diff --git a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java index 95f2df3e88..cf85aeb8cc 100644 --- a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java +++ b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java @@ -886,6 +886,18 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests { assertQuery("message:12346", change2); } + @Test + public void byMessageMixedCase() throws Exception { + TestRepository repo = createProject("repo"); + RevCommit commit1 = repo.parseBody(repo.commit().message("Hello gerrit").create()); + Change change1 = insert(repo, newChangeForCommit(repo, commit1)); + RevCommit commit2 = repo.parseBody(repo.commit().message("Hello Gerrit").create()); + Change change2 = insert(repo, newChangeForCommit(repo, commit2)); + + assertQuery("message:gerrit", change2, change1); + assertQuery("message:Gerrit", change2, change1); + } + @Test public void byLabel() throws Exception { accountManager.authenticate(AuthRequest.forUser("anotheruser")); diff --git a/javatests/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java b/javatests/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java index bacbb60089..750813ad7f 100644 --- a/javatests/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java +++ b/javatests/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java @@ -50,6 +50,7 @@ import com.google.gerrit.server.group.InternalGroup; import com.google.gerrit.server.group.db.GroupsUpdate; import com.google.gerrit.server.group.db.InternalGroupUpdate; import com.google.gerrit.server.index.group.GroupField; +import com.google.gerrit.server.index.group.GroupIndex; import com.google.gerrit.server.index.group.GroupIndexCollection; import com.google.gerrit.server.schema.SchemaCreator; import com.google.gerrit.server.util.ManualRequestContext; @@ -104,6 +105,8 @@ public abstract class AbstractQueryGroupsTest extends GerritServerTests { @Inject protected GroupIndexCollection indexes; + @Inject private GroupIndexCollection groupIndexes; + protected LifecycleManager lifecycle; protected Injector injector; protected ReviewDb db; @@ -392,6 +395,19 @@ public abstract class AbstractQueryGroupsTest extends GerritServerTests { assertThat(rawFields.get().getValue(GroupField.UUID)).isEqualTo(uuid.get()); } + @Test + public void byDeletedGroup() throws Exception { + GroupInfo group = createGroup(name("group")); + AccountGroup.UUID uuid = new AccountGroup.UUID(group.id); + String query = "uuid:" + uuid; + assertQuery(query, group); + + for (GroupIndex index : groupIndexes.getWriteIndexes()) { + index.delete(uuid); + } + assertQuery(query); + } + private Account.Id createAccountOutsideRequestContext( String username, String fullName, String email, boolean active) throws Exception { try (ManualRequestContext ctx = oneOffRequestContext.open()) {