Merge "Add more test field types to TestIndexedFields"
diff --git a/Documentation/cmd-create-project.txt b/Documentation/cmd-create-project.txt
index 358324d..87f3851 100644
--- a/Documentation/cmd-create-project.txt
+++ b/Documentation/cmd-create-project.txt
@@ -108,6 +108,7 @@
Action used by Gerrit to submit an approved change to its
destination branch. Supported options are:
+
+* INHERIT: inherits the submit-type from the parent project.
* FAST_FORWARD_ONLY: produces a strictly linear history.
* MERGE_IF_NECESSARY: create a merge commit when required.
* REBASE_IF_NECESSARY: rebase the commit when required.
@@ -116,7 +117,7 @@
* CHERRY_PICK: always cherry-pick the commit.
+
-Defaults to MERGE_IF_NECESSARY unless
+Defaults to INHERIT unless
link:config-gerrit.html#repository.name.defaultSubmitType[
repository.<name>.defaultSubmitType] is set to a different value.
For more details see link:config-project-config.html#submit-type[
diff --git a/Documentation/config-project-config.txt b/Documentation/config-project-config.txt
index 7a0e305..31008f6 100644
--- a/Documentation/config-project-config.txt
+++ b/Documentation/config-project-config.txt
@@ -736,6 +736,15 @@
the parent project. If the property is not set in any parent project, the
default value is `FALSE`.
+[[reviewer.skipAddingAuthorAndCommitterAsReviewers]]reviewer.skipAddingAuthorAndCommitterAsReviewers::
++
+Whether to skip adding the Git commit author and committer as reviewers for
+a new change.
++
+Default is `INHERIT`, which means that this property is inherited from
+the parent project. If the property is not set in any parent project, the
+default value is `FALSE`.
+
[[file-groups]]
== The file +groups+
diff --git a/Documentation/images/user-review-ui-apply-fix.png b/Documentation/images/user-review-ui-apply-fix.png
new file mode 100644
index 0000000..d838d48
--- /dev/null
+++ b/Documentation/images/user-review-ui-apply-fix.png
Binary files differ
diff --git a/Documentation/images/user-review-ui-change-metadata.png b/Documentation/images/user-review-ui-change-metadata.png
new file mode 100644
index 0000000..23abc07
--- /dev/null
+++ b/Documentation/images/user-review-ui-change-metadata.png
Binary files differ
diff --git a/Documentation/images/user-review-ui-change-screen-annotated.png b/Documentation/images/user-review-ui-change-screen-annotated.png
index 5c3f80a..4e12c96 100644
--- a/Documentation/images/user-review-ui-change-screen-annotated.png
+++ b/Documentation/images/user-review-ui-change-screen-annotated.png
Binary files differ
diff --git a/Documentation/images/user-review-ui-change-screen-change-info-labels.png b/Documentation/images/user-review-ui-change-screen-change-info-labels.png
deleted file mode 100644
index 61e2b25..0000000
--- a/Documentation/images/user-review-ui-change-screen-change-info-labels.png
+++ /dev/null
Binary files differ
diff --git a/Documentation/images/user-review-ui-change-screen-comments-tab.png b/Documentation/images/user-review-ui-change-screen-comments-tab.png
new file mode 100644
index 0000000..d522f60
--- /dev/null
+++ b/Documentation/images/user-review-ui-change-screen-comments-tab.png
Binary files differ
diff --git a/Documentation/images/user-review-ui-change-screen-file-list.png b/Documentation/images/user-review-ui-change-screen-file-list.png
index 721b229..b0c2af3 100644
--- a/Documentation/images/user-review-ui-change-screen-file-list.png
+++ b/Documentation/images/user-review-ui-change-screen-file-list.png
Binary files differ
diff --git a/Documentation/images/user-review-ui-change-screen-keyboard-shortcuts.png b/Documentation/images/user-review-ui-change-screen-keyboard-shortcuts.png
index 9ef8f27..224de2d 100644
--- a/Documentation/images/user-review-ui-change-screen-keyboard-shortcuts.png
+++ b/Documentation/images/user-review-ui-change-screen-keyboard-shortcuts.png
Binary files differ
diff --git a/Documentation/images/user-review-ui-change-screen-reply.png b/Documentation/images/user-review-ui-change-screen-reply.png
index 1c50fc5..201db13 100644
--- a/Documentation/images/user-review-ui-change-screen-reply.png
+++ b/Documentation/images/user-review-ui-change-screen-reply.png
Binary files differ
diff --git a/Documentation/images/user-review-ui-change-screen-topleft.png b/Documentation/images/user-review-ui-change-screen-topleft.png
index a1f7813..b3bf8e7f 100644
--- a/Documentation/images/user-review-ui-change-screen-topleft.png
+++ b/Documentation/images/user-review-ui-change-screen-topleft.png
Binary files differ
diff --git a/Documentation/images/user-review-ui-change-screen.png b/Documentation/images/user-review-ui-change-screen.png
index ff2570b..98a5d6d 100644
--- a/Documentation/images/user-review-ui-change-screen.png
+++ b/Documentation/images/user-review-ui-change-screen.png
Binary files differ
diff --git a/Documentation/images/user-review-ui-copy-links.png b/Documentation/images/user-review-ui-copy-links.png
new file mode 100644
index 0000000..f8fa114
--- /dev/null
+++ b/Documentation/images/user-review-ui-copy-links.png
Binary files differ
diff --git a/Documentation/images/user-review-ui-side-by-side-diff-screen-inline-comments.png b/Documentation/images/user-review-ui-side-by-side-diff-screen-inline-comments.png
index 047034c..98cf7af 100644
--- a/Documentation/images/user-review-ui-side-by-side-diff-screen-inline-comments.png
+++ b/Documentation/images/user-review-ui-side-by-side-diff-screen-inline-comments.png
Binary files differ
diff --git a/Documentation/images/user-review-ui-side-by-side-diff-screen.png b/Documentation/images/user-review-ui-side-by-side-diff-screen.png
index 74d02e3..ebdd177 100644
--- a/Documentation/images/user-review-ui-side-by-side-diff-screen.png
+++ b/Documentation/images/user-review-ui-side-by-side-diff-screen.png
Binary files differ
diff --git a/Documentation/images/user-review-ui-submit-requirements.png b/Documentation/images/user-review-ui-submit-requirements.png
new file mode 100644
index 0000000..e4b88c1
--- /dev/null
+++ b/Documentation/images/user-review-ui-submit-requirements.png
Binary files differ
diff --git a/Documentation/images/user-review-ui-suggest-fix.png b/Documentation/images/user-review-ui-suggest-fix.png
new file mode 100644
index 0000000..e08fb26
--- /dev/null
+++ b/Documentation/images/user-review-ui-suggest-fix.png
Binary files differ
diff --git a/Documentation/index.txt b/Documentation/index.txt
index c3d79b1..89b88aa 100644
--- a/Documentation/index.txt
+++ b/Documentation/index.txt
@@ -24,7 +24,7 @@
== Tutorials
. Web
-.. link:user-review-ui.html[Reviewing Changes]
+.. link:user-review-ui.html[Review UI Overview]
.. link:user-search.html[Searching Changes]
.. link:user-inline-edit.html[Manipulating Changes in Browser]
.. link:user-notify.html[Subscribing to Email Notifications]
diff --git a/Documentation/rest-api-projects.txt b/Documentation/rest-api-projects.txt
index 94db15e..9e71df7 100644
--- a/Documentation/rest-api-projects.txt
+++ b/Documentation/rest-api-projects.txt
@@ -3896,17 +3896,19 @@
Map with the comment link configurations of the project. The name of
the comment link configuration is mapped to a link:#commentlink-info[
CommentlinkInfo] entity.
-|`plugin_config` |optional|
+|`plugin_config` |optional|
Plugin configuration as map which maps the plugin name to a map of
parameter names to link:#config-parameter-info[ConfigParameterInfo]
entities. Only filled for users who have read access to `refs/meta/config`.
-|`actions` |optional|
+|`actions` |optional|
Actions the caller might be able to perform on this project. The
information is a map of view names to
-|`reject_empty_commit` |optional|
+|`reject_empty_commit` |optional|
link:#inherited-boolean-info[InheritedBooleanInfo] that tells whether
empty commits should be rejected when a change is merged.
link:rest-api-changes.html#action-info[ActionInfo] entities.
+|`skip_adding_author_and_committer_as_reviewers` |optional|
+Whether to skip adding the Git commit author and committer as reviewers for a new change.
|=======================================================
[[config-input]]
diff --git a/Documentation/user-review-ui.txt b/Documentation/user-review-ui.txt
index 6f5f7297..73668d7 100644
--- a/Documentation/user-review-ui.txt
+++ b/Documentation/user-review-ui.txt
@@ -1,21 +1,15 @@
:linkattrs:
-= Review UI
+= Review UI Overview
Reviewing changes is an important task and the Gerrit Web UI provides
many functionalities to make the review process comfortable and
efficient.
-The UI has three different main views,
-
-** The dashboard, which shows all changes that are relevant to you
-** The change screen, which shows the change with all its metadata
-** The diff view, which shows changes to a single file
-
[[change-screen]]
== Change Screen
-The change screen shows the details of a single change and provides
-various actions on it.
+The change screen is the main view for a change. It shows the details of a
+single change and allows various actions on it.
image::images/user-review-ui-change-screen.png[width=800, link="images/user-review-ui-change-screen.png"]
@@ -28,44 +22,81 @@
Top left, you find the status of the change, and a permalink.
-image::images/user-review-ui-change-screen-topleft.png[width=400, link="images/user-review-ui-change-screen-topleft.png"]
+image::images/user-review-ui-change-screen-topleft.png[width=600, link="images/user-review-ui-change-screen-topleft.png"]
[[change-status]]
The change status shows the state of the change:
-- [[active]]`Active`:
+- `Active`:
+
The change is under active review.
-- [[merge-conflict]]`Merge Conflict`:
+- `Merge Conflict`:
+
-The change can't be merged due to conflicts.
+The change can't be merged into the destination branch due to conflicts.
-- [[ready-to-submit]]`Ready to Submit`:
+- `Ready to Submit`:
+
-The change has all necessary approvals and may be submitted.
+The change has all necessary approvals and fulfils all other submit
+requirements. It can be submitted.
-- [[merged]]`Merged`:
+- `Merged`:
+
The change was successfully merged into the destination branch.
-- [[abandoned]]`Abandoned`:
+- `Abandoned`:
+
-The change was abandoned.
+The change was abandoned. It is not intended to be updated, reviewed or
+submitted anymore.
+
+- `Private`:
++
+The change is marked as link:intro-user.html#private-changes[Private]. And has
+reduced visibility.
+
+- `Revert Created|Revert Submitted`:
++
+The change has a corresponding revert change. Revert changes can be created
+through UI (see <<actions, Actions section>>).
+
+- `WIP`:
++
+The change was marked as "Work in Progress". For example to indicate to
+reviewers that they shouldn't review the change yet.
[[star]]
=== Star Change
-Clicking the star icon marks the change as a favorite: it turns on
+Clicking the star icon bookmarks the change: it turns on
email notifications for this change, and the change is added to the
list under `Your` > `Starred Changes`. They can be queried by the
link:user-search.html#is[is:starred] search operator.
+[[quick-links]]
+=== Links Menu
+
+Links menu contains various change related strings for quick copying. Such as:
+Change Number, URL, Title+Url, etc. The lines in this menu can also be accessed
+via shortcuts for convenience.
+
+image::images/user-review-ui-copy-links.png[width=600, link="images/user-review-ui-copy-links.png"]
+
[[change-info]]
=== Change metadata
-The change metadata block contains detailed information about the change
-and offers actions on the change.
+The change metadata block contains detailed information about the change.
+
+image::images/user-review-ui-change-metadata.png[width=600, link="images/user-review-ui-change-metadata.png"]
+
+- [[owner]]Owner/Uploader/Author/Committer
++
+Owner is the person who created the change
++
+Uploader is the person who uploaded the latest patchset (the patchset that will
+be merged if the change is submitted)
++
+Author/Committer are concepts from Git and are retrieved from the commit when
+it's sent for review.
- [[reviewers]]Reviewers:
+
@@ -74,16 +105,36 @@
For each reviewer there is a tooltip that shows on which labels the
reviewer is allowed to vote.
+
-New reviewers can be added by clicking on the pencil icon. Typing
-into the pop-up text field activates auto completion of user and group
-names.
+New reviewers can be added through reply dialog that is opened by clicking on
+the pencil icon or on "Reply" button. Typing into the reviewer text field
+activates auto completion of user and group names.
+
+
+- [[cc-list]]CC:
++
+Accounts in CC receive notifications for the updates on the change, but don't
+need to vote/review. If the CC'ed user votes they are moved to reviewers.
++
+
+- [[attention-set]]Attention set:
++
+Users in attention set are marked by "chevron" symbol (see screenshot above).
+The mark indicates that there are actions their attention is required on the
+change: Something updated/changed since last review, their vote is required,
+etc.
++
+Changes for which you are currently in attention set can be found using
+`attention:<User>` in search and show up in a separate category of personal
+dashboard.
++
+Clicking on the mark removes the user from attention set.
+
+
[[remove-reviewer]]
-Reviewers can be removed from the change by clicking on the `x` icon
-in the reviewer's chip token. Removing a reviewer also removes the
-current votes of the reviewer. The removal of votes is recorded as a
-message on the change.
-+
+Reviewers can be removed from the change by selecting the appropriate option on
+the chip's hovercard. Removing a reviewer also removes current votes of the
+reviewer. The removal of votes is recorded in the change log.
+
Removing reviewers is protected by permissions:
** Users can always remove themselves.
@@ -92,10 +143,7 @@
Remove Reviewer] access right, the branch owner, the project owner
and Gerrit administrators may remove anyone.
-+
-image::images/user-review-ui-change-screen-info-reviewers.png[width=600, link="images/user-review-ui-change-screen-reviewers.png"]
-
-- [[project-branch-topic]]Project / Branch / Topic:
+- [[repo-branch-topic]]Project (Repo) / Branch / Topic:
+
The name of the project for which the change was done is displayed as a
link to the link:user-dashboards.html#project-default-dashboard[default
@@ -112,15 +160,55 @@
access right. To be able to set a topic on a closed change, the
`Edit Topic Name` must be assigned with the `force` flag.
+- [[parent]]Parent:
++
+Parent commit of the latest uploaded patchset. Or if the change has been merged
+the parent of the commit it was merged as into the destination branch.
+
+- [[merged-as]]Merged As:
++
+The SHA of the commit corresponding to the merged change on the destination
+branch.
+
+- [[revert-created-as]]Revert (Created|Submitted) As
++
+Points to the revert change, if one was created.
+
+- [[cherry-pick-of]]Cherry-pick of
++
+If the change was created as cherry-pick of some other change to a different
+branch, points to the original change.
+
- [[submit-strategy]]Submit Strategy:
+
The link:project-setup.html#submit_type[submit strategy] that will be
used to submit the change. The submit strategy is only displayed for
open changes.
-- [[actions]]Actions:
+- [[hastags]]Hashtags:
+
-Actions buttons are at the top, and in the overflow menu.
+Arbitrary string hashtags, that can be used to categorize changes and later use
+hashtags for search queries.
+
+[[submit-requirements]]
+=== Submit Requirements
+
+image::images/user-review-ui-submit-requirements.png[width=600, link="images/user-review-ui-copy-links.png"]
+
+Submit Requirements describe various conditions that must be fulfilled before
+the change can be submitted. Hovering over the requirement will show the
+description of the requirement, as well as additional information, such as:
+corresponding expression that is being evaluated, who can vote on the related
+labels etc.
+
+Approving votes are colored green; negative votes are colored red.
+
+For more detail on Submit Requirements see
+link:config-submit-requirements.html[Submit Requirement Configuration] page.
+
+[[actions]]
+=== Actions:
+Actions buttons are at the top right and in the overflow menu.
Depending on the change state and the permissions of the user, different
actions are available on the change:
@@ -220,13 +308,7 @@
+
image::images/user-review-ui-change-screen-change-info-actions.png[width=400, link="images/user-review-ui-change-screen-change-info-actions.png"]
-- [[labels]]Labels & Votes:
-+
-Approving votes are colored green; negative votes are colored red.
-+
-image::images/user-review-ui-change-screen-change-info-labels.png[width=400, link="images/user-review-ui-change-screen-change-info-labels.png"]
-
-[[files]]
+[[files-tab]]
=== File List
The file list shows the files that are modified in the currently viewed
@@ -251,17 +333,40 @@
The list of commits that are being integrated into the destination
branch by submitting the merge commit.
+Every file is accompanied by a number of extra information, such as status
+(modified, added, deleted, etc.), number of changed lines, type (executable,
+link, plain), comments and others. Hovering over most icons and columns reveals
+additional information.
+
+Each file can be expanded to view the contents of the file and diff. For more
+information see <<diff-view, Diff View>> section.
+
+[[comments-tab]]
+=== Comments Tab
+
+Instead of the file list, a comments tab can be selected. Comments tab presents
+comments along with related file/diff snippets. It also offers some filtering
+opportunities at the top (ex. only unresolved, only comments from user X, etc.)
+
+image::images/user-review-ui-change-screen-comments-tab.png[width=800, link="images/user-review-ui-change-screen-comments-tab.png"]
+
+[[checks-tab]]
+=== Checks Tab
+Checks tab contains results of different "Check Runs" installed by plugins. For
+more information see link:pg-plugin-checks-api.html[Checks API] page.
[[patch-sets]]
=== Patch Sets
-The change screen only presents one patch set at a time. Which patch
-set is currently viewed can be seen from the `Patch Sets` drop-down
-panel in the change header.
+The change screen only presents one pair of patch sets (`Patchset A` and
+`Patchset B`) at a time. `A` is always an earlier upload than `B` and serves as
+a base for diffing when viewing changes in the files. Which patch
+sets is currently viewed can be seen from the `Patch Sets` drop-down
+panel in the change header. If patchset 'A' is not selected a parent commit of
+patchset 'B' is used by default.
image::images/user-review-ui-change-screen-patch-sets.png[width=300, link="images/user-review-ui-change-screen-patch-sets.png"]
-
[[download]]
=== Download
@@ -278,7 +383,8 @@
Each command has a copy-to-clipboard icon that allows the command to be
copied into the clipboard. This makes it easy to paste and execute the
-command on a Git command line.
+command on a Git command line. Additionally each line can copied to clipboard
+using number (1..9) of the appropriate line as a keyboard shortcut.
If several download schemes are configured on the server (e.g. SSH and
HTTP) there is a drop-down list to switch between the download schemes.
@@ -306,22 +412,20 @@
image::images/user-review-ui-change-screen-included-in.png[width=800, link="images/user-review-ui-change-screen-included-in.png"]
-
-
[[related-changes]]
=== Related Changes
If there are changes that are related to the currently viewed change
they are displayed in the third column of the change screen.
-There are several lists of related changes and a tab control is used to
-display each list of related changes in its own tab.
+There are several lists of related changes that are displayed in separate
+sectionsunder each other.
-The following tabs may be displayed:
+The following sections may be displayed:
-- [[related-changes-tab]]`Related Changes`:
+- [[related-changes-section]]`Related Changes`:
+
-This tab page shows changes on which the current change depends
+This section shows changes on which the current change depends
(ancestors) and open changes that depend on the current change
(descendants). For merge commits it also shows the closed changes that
will be merged into the destination branch by submitting the merge
@@ -341,10 +445,10 @@
+
** [[not-current]]Not current:
+
-The selected patch set of the change is outdated; it is not the current
-patch set of the change.
+The patch set of the related change which is related to the current change is
+outdated; it is not the current patch set of the change.
+
-It means that the
+For ancestor it means that the
currently viewed patch set depends on a outdated patch set of the
ancestor change. This is because a new patch set for the ancestor
change was uploaded in the meantime and as result the currently viewed
@@ -364,20 +468,24 @@
note that following the link to an indirect descendant change may
result in a completely different related changes listing.
-** [[closed-ancestor]]Closed ancestor:
+** [[merged-related-change]]Merged
+
-Indicates a closed ancestor, e.g. the commit was directly pushed into
-the repository bypassing code review, or the ancestor change was
-reviewed and submitted on another branch. The latter may indicate that
-the user has accidentally pushed the commit to the wrong branch, e.g.
-the commit was done on `branch-a`, but was then pushed to
-`refs/for/branch-b`.
+The change has been merged.
++
+If the relationship to submitted change falls under conditions described in
+<<not-current, Not current>> the status is orange. Such changes can appear as
+both ancestors and descendants of the change.
+
+** [[submittable-related-change]]Submittable
++
+All the submit requirements are fulfilled for the related change and it can be
+submitted when all of its ancestors are submitted.
** [[closed-ancestor-abandoned]]Abandoned:
+
Indicates an abandoned change.
-- [[conflicts-with]]`Conflicts With`:
+- [[conflicts-with]]`Merge Conflicts`:
+
This section shows changes that conflict with the current change.
Non-mergeable changes are filtered out; only conflicting changes that
@@ -393,10 +501,9 @@
currently viewed change, when clicking the submit button. It includes
ancestors of the current patch set.
+
-This may include changes and its ancestors with the same topic if
-`change.submitWholeTopic` is enabled. Only open changes with the
-same topic are included in the list.
-+
+If `change.submitWholeTopic` is enabled this section also includes changes with
+the same topic. The list recursively includes all changes that can be reached by
+ancestor and topic relationships. Only open changes are included in the result.
- [[cherry-picks]]`Cherry-Picks`:
+
@@ -411,12 +518,18 @@
If there are no related changes for a tab, the tab is not displayed.
+- [[same-topic]]`Same Topic`:
++
+This section shows changes which are part of the same topic. If
+`change.submitWholeTopic` is enabled, then this section is omitted and changes
+are included as part of <<submitted-together, `Submitted Together`>>
+
[[reply]]
=== Reply
The `Reply...` button in the change header allows to reply to the
currently viewed patch set; one can add a summary comment, publish
-inline draft comments, and vote on the labels.
+inline draft comments, vote on the labels and adjust attention set.
image::images/user-review-ui-change-screen-reply.png[width=800, link="images/user-review-ui-change-screen-reply.png"]
@@ -424,10 +537,8 @@
[[summary-comment]]
A text box allows to type a summary comment for the currently viewed
-patch set. Some basic markdown-like syntax is supported which renders
-indented lines preformatted, lines starting with "- " or "* " as list
-items, and lines starting with "> " as block quotes (also see replying to
-link:#reply-to-message[messages] and link:#reply-inline-comment[inline comments]).
+patch set. Markdown syntax is supported same as in other
+<<comments-markdown, Comments>>.
[[vote]]
If the current patch set is viewed, buttons are displayed for
@@ -439,7 +550,7 @@
are links to navigate to the inline comments which can be used if a
comment needs to be edited.
-The `Post` button publishes the comments and the votes.
+The `SEND` button publishes the comments and the votes.
[[quick-approve]]
If a user can approve a label that is still required, a quick approve
@@ -460,12 +571,12 @@
image::images/user-review-ui-change-screen-quick-approve.png[width=800, link="images/user-review-ui-change-screen-quick-approve.png"]
-[[history]]
-=== History
+[[change-log]]
+=== Change Log
The history of the change can be seen in the lower part of the screen.
-The history contains messages for all kinds of change updates, e.g. a
+The log contains messages for all kinds of change updates, e.g. a
message is added when a new patch set is uploaded or when a review was
done.
@@ -491,12 +602,12 @@
image::images/user-review-ui-change-screen-plugin-extensions.png[width=300, link="images/user-review-ui-change-screen-plugin-extensions.png"]
-[[side-by-side]]
+[[diff-view]]
== Side-by-Side Diff Screen
-The side-by-side diff screen shows a single patch; the old file version
-is displayed on the left side of the screen; the new file version is
-displayed on the right side of the screen.
+The side-by-side diff screen shows a single patch (or difference between two
+patchsets); the old file version is displayed on the left side of the screen;
+the new file version is displayed on the right side of the screen.
This screen allows to review a patch and to comment on it.
@@ -557,6 +668,10 @@
Code blocks with comments may overlap. This means it is possible to
attach several comments to the same code.
+[[comments-markdown]]
+The comments support markdown. It follows the CommonMark spec, except inline
+images and direct HTML are not rendered and kept as plaintext.
+
[[line-links]]
The lines of the patch file are linkable: simply append
'#<linenumber>' to the URL, or click on the line-number. This not only
@@ -565,15 +680,14 @@
[[reply-inline-comment]]
Clicking on the `Reply` button opens an editor to type the reply.
-Quoting is supported, but only by manually copying & pasting the old
-comment that should be quoted and prefixing every line by "> ". Please
-note that for a correct rendering it is important to leave a blank line
-between a quoted block and the reply to it.
+Previous comment can be quoted using "Quote" button. A new draft would be open
+on the same comment thread with the text of the previoused comment quoted using
+markdown syntax.
image::images/user-review-ui-side-by-side-diff-screen-inline-comments.png[width=800, link="images/user-review-ui-side-by-side-diff-screen-inline-comments.png"]
Comments are first saved as drafts, and you can revisit the drafts as
-you read through code review. Finally, they should be published by
+you read through code review. Finally, they will be published by
clicking the "Reply".
[[done]]
@@ -610,6 +724,21 @@
make it visible to other users it must be published from the change
screen by link:#reply[replying] to the change.
+[[suggest-fix]]
+=== Suggest fix (WIP)
+Comments can contain suggested fixes.
+
+Clicking "Suggest Fix" will insert a special code-block in the text of the
+comment. The contents of this code block will replace the lines the comment is
+attached to (what gets highlighted when hovering over comment).
+
+image::images/user-review-ui-suggest-fix.png[width=400, link="images/user-review-ui-suggest-fix.png"]
+
+The author of the change can then preview and apply the change. This will created
+a new patchset with changes applied.
+
+image::images/user-review-ui-apply-fix.png[width=800, link="images/user-review-ui-apply-fix.png"]
+
[[file-level-comments]]
=== File Level Comments
diff --git a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
index 3e48eec..5b4a9e5 100644
--- a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
+++ b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
@@ -1080,6 +1080,19 @@
}
}
+ protected void setSkipAddingAuthorAndCommitterAsReviewers(InheritableBoolean value)
+ throws Exception {
+ try (MetaDataUpdate md = metaDataUpdateFactory.create(project)) {
+ ProjectConfig config = projectConfigFactory.read(md);
+ config.updateProject(
+ p ->
+ p.setBooleanConfig(
+ BooleanProjectConfig.SKIP_ADDING_AUTHOR_AND_COMMITTER_AS_REVIEWERS, value));
+ config.commit(md);
+ projectCache.evictAndReindex(config.getProject());
+ }
+ }
+
protected void blockAnonymousRead() throws Exception {
String allRefs = RefNames.REFS + "*";
projectOperations
diff --git a/java/com/google/gerrit/common/RawInputUtil.java b/java/com/google/gerrit/common/RawInputUtil.java
index 4a676e6..23e4a23 100644
--- a/java/com/google/gerrit/common/RawInputUtil.java
+++ b/java/com/google/gerrit/common/RawInputUtil.java
@@ -14,7 +14,6 @@
package com.google.gerrit.common;
-import static com.google.common.base.Preconditions.checkArgument;
import static java.nio.charset.StandardCharsets.UTF_8;
import static java.util.Objects.requireNonNull;
@@ -31,7 +30,6 @@
public static RawInput create(byte[] bytes, String contentType) {
requireNonNull(bytes);
- checkArgument(bytes.length > 0);
return new RawInput() {
@Override
public InputStream getInputStream() throws IOException {
diff --git a/java/com/google/gerrit/entities/BooleanProjectConfig.java b/java/com/google/gerrit/entities/BooleanProjectConfig.java
index 5201f6d..605c40c 100644
--- a/java/com/google/gerrit/entities/BooleanProjectConfig.java
+++ b/java/com/google/gerrit/entities/BooleanProjectConfig.java
@@ -41,7 +41,9 @@
ENABLE_REVIEWER_BY_EMAIL("reviewer", "enableByEmail"),
MATCH_AUTHOR_TO_COMMITTER_DATE("submit", "matchAuthorToCommitterDate"),
REJECT_EMPTY_COMMIT("submit", "rejectEmptyCommit"),
- WORK_IN_PROGRESS_BY_DEFAULT("change", "workInProgressByDefault");
+ WORK_IN_PROGRESS_BY_DEFAULT("change", "workInProgressByDefault"),
+ SKIP_ADDING_AUTHOR_AND_COMMITTER_AS_REVIEWERS(
+ "reviewer", "skipAddingAuthorAndCommitterAsReviewers");
// Git config
private final String section;
diff --git a/java/com/google/gerrit/entities/LabelFunction.java b/java/com/google/gerrit/entities/LabelFunction.java
index f361741..d49ab0f 100644
--- a/java/com/google/gerrit/entities/LabelFunction.java
+++ b/java/com/google/gerrit/entities/LabelFunction.java
@@ -14,6 +14,7 @@
package com.google.gerrit.entities;
+import com.google.common.collect.ImmutableSet;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.SubmitRecord.Label;
import java.util.Collections;
@@ -48,6 +49,16 @@
ALL = Collections.unmodifiableMap(all);
}
+ public static final Map<String, LabelFunction> ALL_NON_DEPRECATED;
+
+ static {
+ Map<String, LabelFunction> allNonDeprecated = new LinkedHashMap<>();
+ for (LabelFunction f : ImmutableSet.of(NO_BLOCK, NO_OP, PATCH_SET_LOCK)) {
+ allNonDeprecated.put(f.getFunctionName(), f);
+ }
+ ALL_NON_DEPRECATED = Collections.unmodifiableMap(allNonDeprecated);
+ }
+
public static Optional<LabelFunction> parse(@Nullable String str) {
return Optional.ofNullable(ALL.get(str));
}
diff --git a/java/com/google/gerrit/entities/SubmitRequirementExpressionResult.java b/java/com/google/gerrit/entities/SubmitRequirementExpressionResult.java
index c24227d..fbb2fd7 100644
--- a/java/com/google/gerrit/entities/SubmitRequirementExpressionResult.java
+++ b/java/com/google/gerrit/entities/SubmitRequirementExpressionResult.java
@@ -197,8 +197,6 @@
@AutoValue.Builder
public abstract static class Builder {
- public abstract Builder childPredicateResults(ImmutableList<PredicateResult> value);
-
protected abstract ImmutableList.Builder<PredicateResult> childPredicateResultsBuilder();
public abstract Builder predicateString(String value);
diff --git a/java/com/google/gerrit/extensions/api/projects/ConfigInfo.java b/java/com/google/gerrit/extensions/api/projects/ConfigInfo.java
index 3ba1277..1a51c15 100644
--- a/java/com/google/gerrit/extensions/api/projects/ConfigInfo.java
+++ b/java/com/google/gerrit/extensions/api/projects/ConfigInfo.java
@@ -40,6 +40,7 @@
public InheritedBooleanInfo enableReviewerByEmail;
public InheritedBooleanInfo matchAuthorToCommitterDate;
public InheritedBooleanInfo rejectEmptyCommit;
+ public InheritedBooleanInfo skipAddingAuthorAndCommitterAsReviewers;
public MaxObjectSizeLimitInfo maxObjectSizeLimit;
@Deprecated // Equivalent to defaultSubmitType.value
diff --git a/java/com/google/gerrit/extensions/api/projects/ConfigInput.java b/java/com/google/gerrit/extensions/api/projects/ConfigInput.java
index 8005fc5..906fc4c 100644
--- a/java/com/google/gerrit/extensions/api/projects/ConfigInput.java
+++ b/java/com/google/gerrit/extensions/api/projects/ConfigInput.java
@@ -34,6 +34,7 @@
public InheritableBoolean enableReviewerByEmail;
public InheritableBoolean matchAuthorToCommitterDate;
public InheritableBoolean rejectEmptyCommit;
+ public InheritableBoolean skipAddingAuthorAndCommitterAsReviewers;
public String maxObjectSizeLimit;
public SubmitType submitType;
public ProjectState state;
diff --git a/java/com/google/gerrit/index/IndexedField.java b/java/com/google/gerrit/index/IndexedField.java
index 1475e4b..99004bb 100644
--- a/java/com/google/gerrit/index/IndexedField.java
+++ b/java/com/google/gerrit/index/IndexedField.java
@@ -249,6 +249,8 @@
public SearchSpec integerRange(String name) {
checkState(fieldType().equals(INTEGER_TYPE));
+ // we currently store all integer range fields, this may change in the future
+ checkState(stored());
return addSearchSpec(name, SearchOption.RANGE);
}
diff --git a/java/com/google/gerrit/index/testing/AbstractFakeIndex.java b/java/com/google/gerrit/index/testing/AbstractFakeIndex.java
index 6f79dce..591ae26 100644
--- a/java/com/google/gerrit/index/testing/AbstractFakeIndex.java
+++ b/java/com/google/gerrit/index/testing/AbstractFakeIndex.java
@@ -251,7 +251,7 @@
protected Map<String, Object> docFor(ChangeData value) {
ImmutableMap.Builder<String, Object> doc = ImmutableMap.builder();
for (SchemaField<ChangeData, ?> field : getSchema().getSchemaFields().values()) {
- if (ChangeField.MERGEABLE.getName().equals(field.getName()) && skipMergable) {
+ if (ChangeField.MERGEABLE_SPEC.getName().equals(field.getName()) && skipMergable) {
continue;
}
Object docifiedValue = field.get(value);
diff --git a/java/com/google/gerrit/lucene/LuceneChangeIndex.java b/java/com/google/gerrit/lucene/LuceneChangeIndex.java
index 4122181..de4b26f 100644
--- a/java/com/google/gerrit/lucene/LuceneChangeIndex.java
+++ b/java/com/google/gerrit/lucene/LuceneChangeIndex.java
@@ -143,7 +143,7 @@
this.skipFields =
MergeabilityComputationBehavior.fromConfig(cfg).includeInIndex()
? ImmutableSet.of()
- : ImmutableSet.of(ChangeField.MERGEABLE.getName());
+ : ImmutableSet.of(ChangeField.MERGEABLE_SPEC.getName());
GerritIndexWriterConfig openConfig = new GerritIndexWriterConfig(cfg, "changes_open");
GerritIndexWriterConfig closedConfig = new GerritIndexWriterConfig(cfg, "changes_closed");
diff --git a/java/com/google/gerrit/server/approval/ApprovalCopier.java b/java/com/google/gerrit/server/approval/ApprovalCopier.java
index 059445e..cd2d79e 100644
--- a/java/com/google/gerrit/server/approval/ApprovalCopier.java
+++ b/java/com/google/gerrit/server/approval/ApprovalCopier.java
@@ -32,6 +32,7 @@
import com.google.gerrit.entities.Project;
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.extensions.client.ChangeKind;
+import com.google.gerrit.index.query.Predicate;
import com.google.gerrit.index.query.QueryParseException;
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.change.ChangeKindCache;
@@ -85,7 +86,7 @@
* <li>the approval is not overridden by a current approval on the patch set
* </ul>
*/
- public abstract ImmutableSet<PatchSetApproval> copiedApprovals();
+ public abstract ImmutableSet<PatchSetApprovalData> copiedApprovals();
/**
* Approvals on the previous patch set that have not been copied to the patch set.
@@ -96,7 +97,7 @@
* <p>Only returns non-copied approvals of the previous patch set. Approvals from earlier patch
* sets that were outdated before are not included.
*/
- public abstract ImmutableSet<PatchSetApproval> outdatedApprovals();
+ public abstract ImmutableSet<PatchSetApprovalData> outdatedApprovals();
static Result empty() {
return create(
@@ -105,10 +106,68 @@
@VisibleForTesting
public static Result create(
- ImmutableSet<PatchSetApproval> copiedApprovals,
- ImmutableSet<PatchSetApproval> outdatedApprovals) {
+ ImmutableSet<PatchSetApprovalData> copiedApprovals,
+ ImmutableSet<PatchSetApprovalData> outdatedApprovals) {
return new AutoValue_ApprovalCopier_Result(copiedApprovals, outdatedApprovals);
}
+
+ /**
+ * A {@link PatchSetApproval} with information about which atoms of the copy condition are
+ * passing/failing.
+ */
+ @AutoValue
+ public abstract static class PatchSetApprovalData {
+ /** The approval. */
+ public abstract PatchSetApproval patchSetApproval();
+
+ /**
+ * Lists the leaf predicates of the copy condition that are fulfilled.
+ *
+ * <p>Example: The expression
+ *
+ * <pre>
+ * changekind:TRIVIAL_REBASE OR is:MIN
+ * </pre>
+ *
+ * has two leaf predicates:
+ *
+ * <ul>
+ * <li>changekind:TRIVIAL_REBASE
+ * <li>is:MIN
+ * </ul>
+ *
+ * This method will return the leaf predicates that are fulfilled, for example if only the
+ * first predicate is fulfilled, the returned list will be equal to
+ * ["changekind:TRIVIAL_REBASE"].
+ *
+ * <p>Empty if the label type is missing, if there is no copy condition or if the copy
+ * condition is not parseable.
+ */
+ public abstract ImmutableSet<String> passingAtoms();
+
+ /**
+ * Lists the leaf predicates of the copy condition that are not fulfilled. See {@link
+ * #passingAtoms()} for more details.
+ *
+ * <p>Empty if the label type is missing, if there is no copy condition or if the copy
+ * condition is not parseable.
+ */
+ public abstract ImmutableSet<String> failingAtoms();
+
+ @VisibleForTesting
+ public static PatchSetApprovalData create(
+ PatchSetApproval approval,
+ ImmutableSet<String> passingAtoms,
+ ImmutableSet<String> failingAtoms) {
+ return new AutoValue_ApprovalCopier_Result_PatchSetApprovalData(
+ approval, passingAtoms, failingAtoms);
+ }
+
+ private static PatchSetApprovalData createForMissingLabelType(PatchSetApproval approval) {
+ return new AutoValue_ApprovalCopier_Result_PatchSetApprovalData(
+ approval, ImmutableSet.of(), ImmutableSet.of());
+ }
+ }
}
private final GitRepositoryManager repoManager;
@@ -227,17 +286,18 @@
followUpPatchSet.commitId());
boolean isMerge = isMerge(changeNotes.getProjectName(), revWalk, followUpPatchSet);
- if (canCopy(
- changeNotes,
- priorPatchSet.id(),
- followUpPatchSet,
- approverId,
- labelType.get(),
- approvalValue,
- changeKind,
- isMerge,
- revWalk,
- repo.getConfig())) {
+ if (computeCopyResult(
+ changeNotes,
+ priorPatchSet.id(),
+ followUpPatchSet,
+ approverId,
+ labelType.get(),
+ approvalValue,
+ changeKind,
+ isMerge,
+ revWalk,
+ repo.getConfig())
+ .canCopy()) {
targetPatchSetsBuilder.add(followUpPatchSetId);
} else {
// The approval is not copyable to this follow-up patch set.
@@ -251,7 +311,14 @@
return targetPatchSetsBuilder.build();
}
- private boolean canCopy(
+ /**
+ * Checks whether a given approval can be copied from the given source patch set to the given
+ * target patch set.
+ *
+ * <p>The returned result also informs about which atoms of the copy condition are
+ * passing/failing.
+ */
+ private ApprovalCopyResult computeCopyResult(
ChangeNotes changeNotes,
PatchSet.Id sourcePatchSetId,
PatchSet targetPatchSet,
@@ -263,7 +330,7 @@
RevWalk revWalk,
Config repoConfig) {
if (!labelType.getCopyCondition().isPresent()) {
- return false;
+ return ApprovalCopyResult.createForMissingCopyCondition();
}
ApprovalContext ctx =
ApprovalContext.create(
@@ -283,15 +350,18 @@
// request (e.g. a group used in this query might not be visible to the person sending this
// request).
try (ManualRequestContext ignored = requestContext.open()) {
- return approvalQueryBuilder
- .parse(labelType.getCopyCondition().get())
- .asMatchable()
- .match(ctx);
+ Predicate<ApprovalContext> copyConditionPredicate =
+ approvalQueryBuilder.parse(labelType.getCopyCondition().get());
+ boolean canCopy = copyConditionPredicate.asMatchable().match(ctx);
+ ImmutableSet.Builder<String> passingAtoms = ImmutableSet.builder();
+ ImmutableSet.Builder<String> failingAtoms = ImmutableSet.builder();
+ evaluateAtoms(copyConditionPredicate, ctx, passingAtoms, failingAtoms);
+ return ApprovalCopyResult.create(canCopy, passingAtoms.build(), failingAtoms.build());
}
} catch (QueryParseException e) {
logger.atWarning().withCause(e).log(
"Unable to copy label because config is invalid. This should have been caught before.");
- return false;
+ return ApprovalCopyResult.createForNonParseableCopyCondition();
}
}
@@ -321,8 +391,10 @@
nonCopiedApprovalsForGivenPatchSet.forEach(
psa -> currentApprovalsByUser.put(psa.label(), psa.accountId(), psa));
- Table<String, Account.Id, PatchSetApproval> copiedApprovalsByUser = HashBasedTable.create();
- ImmutableSet.Builder<PatchSetApproval> outdatedApprovalsBuilder = ImmutableSet.builder();
+ Table<String, Account.Id, Result.PatchSetApprovalData> copiedApprovalsByUser =
+ HashBasedTable.create();
+ ImmutableSet.Builder<Result.PatchSetApprovalData> outdatedApprovalsBuilder =
+ ImmutableSet.builder();
ImmutableList<PatchSetApproval> priorApprovals =
notes.load().getApprovals().all().get(priorPatchSet.getKey());
@@ -362,35 +434,55 @@
priorPsa.key().patchSetId().changeId().get(),
targetPsId.get(),
projectName);
- outdatedApprovalsBuilder.add(priorPsa);
+ outdatedApprovalsBuilder.add(
+ Result.PatchSetApprovalData.createForMissingLabelType(priorPsa));
continue;
}
- if (canCopy(
- notes,
- priorPsa.patchSetId(),
- targetPatchSet,
- priorPsa.accountId(),
- labelType.get(),
- priorPsa.value(),
- changeKind,
- isMerge,
- rw,
- repoConfig)) {
+ ApprovalCopyResult approvalCopyResult =
+ computeCopyResult(
+ notes,
+ priorPsa.patchSetId(),
+ targetPatchSet,
+ priorPsa.accountId(),
+ labelType.get(),
+ priorPsa.value(),
+ changeKind,
+ isMerge,
+ rw,
+ repoConfig);
+ if (approvalCopyResult.canCopy()) {
if (!currentApprovalsByUser.contains(priorPsa.label(), priorPsa.accountId())) {
+ PatchSetApproval copiedApproval = priorPsa.copyWithPatchSet(targetPatchSet.id());
+
+ // Normalize the copied approval.
+ Optional<PatchSetApproval> copiedApprovalNormalized =
+ labelNormalizer.normalize(notes, copiedApproval);
+ logger.atFine().log(
+ "Copied approval %s has been normalized to %s",
+ copiedApproval,
+ copiedApprovalNormalized.map(PatchSetApproval::toString).orElse("n/a"));
+ if (!copiedApprovalNormalized.isPresent()) {
+ continue;
+ }
+
copiedApprovalsByUser.put(
priorPsa.label(),
priorPsa.accountId(),
- priorPsa.copyWithPatchSet(targetPatchSet.id()));
+ Result.PatchSetApprovalData.create(
+ copiedApprovalNormalized.get(),
+ approvalCopyResult.passingAtoms(),
+ approvalCopyResult.failingAtoms()));
}
} else {
- outdatedApprovalsBuilder.add(priorPsa);
+ outdatedApprovalsBuilder.add(
+ Result.PatchSetApprovalData.create(
+ priorPsa, approvalCopyResult.passingAtoms(), approvalCopyResult.failingAtoms()));
continue;
}
}
- ImmutableSet<PatchSetApproval> copiedApprovals =
- labelNormalizer.normalize(notes, copiedApprovalsByUser.values()).getNormalized();
- return Result.create(copiedApprovals, outdatedApprovalsBuilder.build());
+ return Result.create(
+ ImmutableSet.copyOf(copiedApprovalsByUser.values()), outdatedApprovalsBuilder.build());
}
private boolean isMerge(Project.NameKey project, RevWalk rw, PatchSet patchSet) {
@@ -404,4 +496,72 @@
e);
}
}
+
+ /**
+ * Evaluates a predicate of the copy condition and adds its passing and failing atoms to the given
+ * builders.
+ *
+ * @param predicate a predicate of the copy condition that should be evaluated
+ * @param approvalContext the approval context against which the predicate should be evaluated
+ * @param passingAtoms a builder to which passing atoms should be added
+ * @param failingAtoms a builder to which failing atoms should be added
+ */
+ private static void evaluateAtoms(
+ Predicate<ApprovalContext> predicate,
+ ApprovalContext approvalContext,
+ ImmutableSet.Builder<String> passingAtoms,
+ ImmutableSet.Builder<String> failingAtoms) {
+ if (predicate.isLeaf()) {
+ boolean isPassing = predicate.asMatchable().match(approvalContext);
+ (isPassing ? passingAtoms : failingAtoms).add(predicate.getPredicateString());
+ return;
+ }
+ predicate
+ .getChildren()
+ .forEach(
+ childPredicate ->
+ evaluateAtoms(childPredicate, approvalContext, passingAtoms, failingAtoms));
+ }
+
+ /** Result for checking if an approval can be copied to the next patch set. */
+ @AutoValue
+ abstract static class ApprovalCopyResult {
+ /** Whether the approval can be copied to the next patch set. */
+ abstract boolean canCopy();
+
+ /**
+ * Lists the leaf predicates of the copy condition that are fulfilled. See {@link
+ * Result.PatchSetApprovalData#passingAtoms()} for more details.
+ *
+ * <p>Empty if there is no copy condition or if the copy condition is not parseable.
+ */
+ abstract ImmutableSet<String> passingAtoms();
+
+ /**
+ * Lists the leaf predicates of the copy condition that are not fulfilled. See {@link
+ * Result.PatchSetApprovalData#passingAtoms()} for more details.
+ *
+ * <p>Empty if there is no copy condition or if the copy condition is not parseable.
+ */
+ abstract ImmutableSet<String> failingAtoms();
+
+ private static ApprovalCopyResult create(
+ boolean canCopy, ImmutableSet<String> passingAtoms, ImmutableSet<String> failingAtoms) {
+ return new AutoValue_ApprovalCopier_ApprovalCopyResult(canCopy, passingAtoms, failingAtoms);
+ }
+
+ private static ApprovalCopyResult createForMissingCopyCondition() {
+ return new AutoValue_ApprovalCopier_ApprovalCopyResult(
+ /* canCopy= */ false,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of());
+ }
+
+ private static ApprovalCopyResult createForNonParseableCopyCondition() {
+ return new AutoValue_ApprovalCopier_ApprovalCopyResult(
+ /* canCopy= */ false,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of());
+ }
+ }
}
diff --git a/java/com/google/gerrit/server/approval/ApprovalsUtil.java b/java/com/google/gerrit/server/approval/ApprovalsUtil.java
index bd31356f..09820b1 100644
--- a/java/com/google/gerrit/server/approval/ApprovalsUtil.java
+++ b/java/com/google/gerrit/server/approval/ApprovalsUtil.java
@@ -15,8 +15,10 @@
package com.google.gerrit.server.approval;
import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.base.Preconditions.checkState;
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.collect.ImmutableListMultimap.toImmutableListMultimap;
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.gerrit.server.notedb.ReviewerStateInternal.CC;
import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
import static com.google.gerrit.server.project.ProjectCache.illegalState;
@@ -25,6 +27,7 @@
import static java.util.stream.Collectors.joining;
import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Function;
import com.google.common.collect.ArrayListMultimap;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableListMultimap;
@@ -85,6 +88,7 @@
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
+import java.util.StringTokenizer;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.revwalk.RevWalk;
@@ -399,12 +403,17 @@
ChangeUpdate changeUpdate) {
ApprovalCopier.Result approvalCopierResult =
approvalCopier.forPatchSet(notes, patchSet, revWalk, repoConfig);
- approvalCopierResult.copiedApprovals().forEach(a -> changeUpdate.putCopiedApproval(a));
+ approvalCopierResult
+ .copiedApprovals()
+ .forEach(approvalData -> changeUpdate.putCopiedApproval(approvalData.patchSetApproval()));
if (!notes.getChange().isWorkInProgress()) {
// The attention set should not be updated when the change is work-in-progress.
addAttentionSetUpdatesForOutdatedApprovals(
- changeUpdate, approvalCopierResult.outdatedApprovals());
+ changeUpdate,
+ approvalCopierResult.outdatedApprovals().stream()
+ .map(ApprovalCopier.Result.PatchSetApprovalData::patchSetApproval)
+ .collect(toImmutableSet()));
}
return approvalCopierResult;
@@ -511,31 +520,40 @@
* "is:FOO")}
* </ul>
*
- * @param approvals the approvals that should be formatted
+ * @param approvalDatas the approvals that should be formatted, with approval meta data
* @param labelTypes the label types
* @return bullet list with the formatted approvals
*/
private String formatApprovalListWithCopyCondition(
- ImmutableSet<PatchSetApproval> approvals, LabelTypes labelTypes) {
+ ImmutableSet<ApprovalCopier.Result.PatchSetApprovalData> approvalDatas,
+ LabelTypes labelTypes) {
StringBuilder message = new StringBuilder();
// sort approvals by label vote so that we list them in a deterministic order
- ImmutableList<PatchSetApproval> approvalsSortedByLabelVote =
- approvals.stream()
- .sorted(comparing(psa -> LabelVote.create(psa.label(), psa.value()).format()))
+ ImmutableList<ApprovalCopier.Result.PatchSetApprovalData> approvalsSortedByLabelVote =
+ approvalDatas.stream()
+ .sorted(
+ comparing(
+ approvalData ->
+ LabelVote.create(
+ approvalData.patchSetApproval().label(),
+ approvalData.patchSetApproval().value())
+ .format()))
.collect(toImmutableList());
- ImmutableListMultimap<String, PatchSetApproval> approvalsByLabel =
- Multimaps.index(approvalsSortedByLabelVote, PatchSetApproval::label);
+ ImmutableListMultimap<String, ApprovalCopier.Result.PatchSetApprovalData> approvalsByLabel =
+ Multimaps.index(
+ approvalsSortedByLabelVote, approvalData -> approvalData.patchSetApproval().label());
- for (Map.Entry<String, Collection<PatchSetApproval>> approvalsByLabelEntry :
- approvalsByLabel.asMap().entrySet()) {
+ for (Map.Entry<String, Collection<ApprovalCopier.Result.PatchSetApprovalData>>
+ approvalsByLabelEntry : approvalsByLabel.asMap().entrySet()) {
String label = approvalsByLabelEntry.getKey();
- Collection<PatchSetApproval> approvalsForSameLabel = approvalsByLabelEntry.getValue();
+ Collection<ApprovalCopier.Result.PatchSetApprovalData> approvalsForSameLabel =
+ approvalsByLabelEntry.getValue();
- message.append("* ");
if (!labelTypes.byLabel(label).isPresent()) {
message
+ .append("* ")
.append(formatApprovalsAsLabelVotesList(approvalsForSameLabel))
.append(" (label type is missing)\n");
continue;
@@ -543,22 +561,65 @@
LabelType labelType = labelTypes.byLabel(label).get();
if (!labelType.getCopyCondition().isPresent()) {
- message.append(formatApprovalsAsLabelVotesList(approvalsForSameLabel)).append("\n");
+ message
+ .append("* ")
+ .append(formatApprovalsAsLabelVotesList(approvalsForSameLabel))
+ .append("\n");
continue;
}
- message
- .append(
- formatApprovalsWithCopyCondition(
- approvalsForSameLabel, labelType.getCopyCondition().get()))
- .append("\n");
+ // Group the approvals that have the same label by the passing atoms. If approvals have the
+ // same label, but have different passing atoms, we need to list them in separate lines
+ // (because in each line we will highlight different passing atoms that matched). Approvals
+ // with the same label and the same passing atoms are formatted as a single line.
+ ImmutableListMultimap<ImmutableSet<String>, ApprovalCopier.Result.PatchSetApprovalData>
+ approvalsForSameLabelByPassingAndFailingAtoms =
+ Multimaps.index(
+ approvalsForSameLabel, ApprovalCopier.Result.PatchSetApprovalData::passingAtoms);
+
+ // Approvals with the same label that have the same passing atoms should have the same failing
+ // atoms (since the label is the same they have the same copy condition).
+ approvalsForSameLabelByPassingAndFailingAtoms
+ .asMap()
+ .values()
+ .forEach(
+ approvalsForSameLabelAndSamePassingAtoms ->
+ checkThatPropertyIsTheSameForAllApprovals(
+ approvalsForSameLabelAndSamePassingAtoms,
+ "failing atoms",
+ approvalData -> approvalData.failingAtoms()));
+
+ // The order in which we add lines for approvals with the same label but different passing
+ // atoms needs to be deterministic for tests. Just sort them by the string representation of
+ // the passing atoms.
+ for (Collection<ApprovalCopier.Result.PatchSetApprovalData>
+ approvalsForSameLabelWithSamePassingAndFailingAtoms :
+ approvalsForSameLabelByPassingAndFailingAtoms.asMap().entrySet().stream()
+ .sorted(
+ comparing(
+ (Map.Entry<
+ ImmutableSet<String>,
+ Collection<ApprovalCopier.Result.PatchSetApprovalData>>
+ e) -> e.getKey().toString()))
+ .map(Map.Entry::getValue)
+ .collect(toImmutableList())) {
+ message
+ .append("* ")
+ .append(
+ formatApprovalsWithCopyCondition(
+ approvalsForSameLabelWithSamePassingAndFailingAtoms,
+ labelType.getCopyCondition().get()))
+ .append("\n");
+ }
}
return message.toString();
}
/**
- * Formats the given approvals of the same label with the given copy condition.
+ * Formats the given approvals with the given copy condition.
+ *
+ * <p>The given approvals must have the same label and the same passing and failing atoms.
*
* <p>E.g.: {Code-Review+1, Code-Review+2 (copy condition: "is:MIN")}
*
@@ -578,12 +639,29 @@
* "is:FOO")}
* </ul>
*
- * @param approvalsForSameLabel the approvals that should be formatted, must be for the same label
+ * @param approvalsWithSameLabelAndSamePassingAndFailingAtoms the approvals that should be
+ * formatted, must be for the same label
* @param copyCondition the copy condition of the label
* @return the formatted approvals
*/
private String formatApprovalsWithCopyCondition(
- Collection<PatchSetApproval> approvalsForSameLabel, String copyCondition) {
+ Collection<ApprovalCopier.Result.PatchSetApprovalData>
+ approvalsWithSameLabelAndSamePassingAndFailingAtoms,
+ String copyCondition) {
+ // Check that all given approvals have the same label and the same passing and failing atoms.
+ checkThatPropertyIsTheSameForAllApprovals(
+ approvalsWithSameLabelAndSamePassingAndFailingAtoms,
+ "label",
+ approvalData -> approvalData.patchSetApproval().label());
+ checkThatPropertyIsTheSameForAllApprovals(
+ approvalsWithSameLabelAndSamePassingAndFailingAtoms,
+ "passing atoms",
+ approvalData -> approvalData.passingAtoms());
+ checkThatPropertyIsTheSameForAllApprovals(
+ approvalsWithSameLabelAndSamePassingAndFailingAtoms,
+ "failing atoms",
+ approvalData -> approvalData.failingAtoms());
+
StringBuilder message = new StringBuilder();
boolean containsUserInPredicate;
@@ -591,7 +669,8 @@
containsUserInPredicate = containsUserInPredicate(copyCondition);
} catch (QueryParseException e) {
logger.atWarning().withCause(e).log("Non-parsable query condition");
- message.append(formatApprovalsAsLabelVotesList(approvalsForSameLabel));
+ message.append(
+ formatApprovalsAsLabelVotesList(approvalsWithSameLabelAndSamePassingAndFailingAtoms));
message.append(String.format(" (non-parseable copy condition: \"%s\")", copyCondition));
return message.toString();
}
@@ -618,26 +697,35 @@
// sort the approvals by their approvers name-email so that the approvers always appear in a
// deterministic order
- ImmutableList<PatchSetApproval> approvalsSortedByLabelVoteAndApprover =
- approvalsForSameLabel.stream()
- .sorted(
- comparing(
- (PatchSetApproval psa) ->
- LabelVote.create(psa.label(), psa.value()).format())
- .thenComparing(
- psa ->
- accountCache
- .getEvenIfMissing(psa.accountId())
- .account()
- .getNameEmail(anonymousCowardName)))
- .collect(toImmutableList());
+ ImmutableList<ApprovalCopier.Result.PatchSetApprovalData>
+ approvalsSortedByLabelVoteAndApprover =
+ approvalsWithSameLabelAndSamePassingAndFailingAtoms.stream()
+ .sorted(
+ comparing(
+ (ApprovalCopier.Result.PatchSetApprovalData approvalData) ->
+ LabelVote.create(
+ approvalData.patchSetApproval().label(),
+ approvalData.patchSetApproval().value())
+ .format())
+ .thenComparing(
+ approvalData ->
+ accountCache
+ .getEvenIfMissing(approvalData.patchSetApproval().accountId())
+ .account()
+ .getNameEmail(anonymousCowardName)))
+ .collect(toImmutableList());
ImmutableListMultimap<LabelVote, Account.Id> approversByLabelVote =
Multimaps.index(
approvalsSortedByLabelVoteAndApprover,
- psa -> LabelVote.create(psa.label(), psa.value()))
+ approvalData ->
+ LabelVote.create(
+ approvalData.patchSetApproval().label(),
+ approvalData.patchSetApproval().value()))
.entries().stream()
- .collect(toImmutableListMultimap(e -> e.getKey(), e -> e.getValue().accountId()));
+ .collect(
+ toImmutableListMultimap(
+ e -> e.getKey(), e -> e.getValue().patchSetApproval().accountId()));
message.append(
approversByLabelVote.asMap().entrySet().stream()
.map(
@@ -647,12 +735,64 @@
.collect(joining(", ")));
} else {
// copy condition doesn't contain a UserInPredicate
- message.append(formatApprovalsAsLabelVotesList(approvalsForSameLabel));
+ message.append(
+ formatApprovalsAsLabelVotesList(approvalsWithSameLabelAndSamePassingAndFailingAtoms));
}
- message.append(String.format(" (copy condition: \"%s\")", copyCondition));
+ ImmutableSet<String> passingAtoms =
+ !approvalsWithSameLabelAndSamePassingAndFailingAtoms.isEmpty()
+ ? approvalsWithSameLabelAndSamePassingAndFailingAtoms.iterator().next().passingAtoms()
+ : ImmutableSet.of();
+ message.append(
+ String.format(
+ " (copy condition: \"%s\")",
+ formatCopyConditionAsMarkdown(copyCondition, passingAtoms)));
return message.toString();
}
+ /** Checks that all given approvals have the same value for a given property. */
+ private void checkThatPropertyIsTheSameForAllApprovals(
+ Collection<ApprovalCopier.Result.PatchSetApprovalData> approvals,
+ String propertyName,
+ Function<ApprovalCopier.Result.PatchSetApprovalData, ?> propertyExtractor) {
+ if (approvals.isEmpty()) {
+ return;
+ }
+
+ Object propertyOfFirstEntry = propertyExtractor.apply(approvals.iterator().next());
+ approvals.forEach(
+ approvalData ->
+ checkState(
+ propertyExtractor.apply(approvalData).equals(propertyOfFirstEntry),
+ "property %s of approval %s does not match, expected value: %s",
+ propertyName,
+ approvalData,
+ propertyOfFirstEntry));
+ }
+
+ /**
+ * Formats the given copy condition as a Markdown string.
+ *
+ * <p>Passing atoms are formatted as bold.
+ *
+ * @param copyCondition the copy condition that should be formatted
+ * @param passingAtoms atoms of the copy conditions which are passing/matching
+ * @return the formatted copy condition as a Markdown string
+ */
+ private String formatCopyConditionAsMarkdown(
+ String copyCondition, ImmutableSet<String> passingAtoms) {
+ StringBuilder formattedCopyCondition = new StringBuilder();
+ StringTokenizer tokenizer = new StringTokenizer(copyCondition, " ()", /* returnDelims= */ true);
+ while (tokenizer.hasMoreTokens()) {
+ String token = tokenizer.nextToken();
+ if (passingAtoms.contains(token)) {
+ formattedCopyCondition.append("**" + token.replace("*", "\\*") + "**");
+ } else {
+ formattedCopyCondition.append(token);
+ }
+ }
+ return formattedCopyCondition.toString();
+ }
+
private boolean containsUserInPredicate(String copyCondition) throws QueryParseException {
// Use a request context to run checks as an internal user with expanded visibility. This is
// so that the output of the copy condition does not depend on who is running the current
@@ -675,8 +815,9 @@
* @return the given approvals as a comma-separated list of label votes
*/
private String formatApprovalsAsLabelVotesList(
- Collection<PatchSetApproval> sortedApprovalsForSameLabel) {
+ Collection<ApprovalCopier.Result.PatchSetApprovalData> sortedApprovalsForSameLabel) {
return sortedApprovalsForSameLabel.stream()
+ .map(ApprovalCopier.Result.PatchSetApprovalData::patchSetApproval)
.map(psa -> LabelVote.create(psa.label(), psa.value()))
.distinct()
.map(LabelVote::format)
diff --git a/java/com/google/gerrit/server/args4j/ObjectIdHandler.java b/java/com/google/gerrit/server/args4j/ObjectIdHandler.java
index aa8a958..3cad7ce 100644
--- a/java/com/google/gerrit/server/args4j/ObjectIdHandler.java
+++ b/java/com/google/gerrit/server/args4j/ObjectIdHandler.java
@@ -16,9 +16,11 @@
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
+import org.eclipse.jgit.errors.InvalidObjectIdException;
import org.eclipse.jgit.lib.ObjectId;
import org.kohsuke.args4j.CmdLineException;
import org.kohsuke.args4j.CmdLineParser;
+import org.kohsuke.args4j.NamedOptionDef;
import org.kohsuke.args4j.OptionDef;
import org.kohsuke.args4j.spi.OptionHandler;
import org.kohsuke.args4j.spi.Parameters;
@@ -37,7 +39,14 @@
@Override
public int parseArguments(Parameters params) throws CmdLineException {
final String n = params.getParameter(0);
- setter.addValue(ObjectId.fromString(n));
+ try {
+ setter.addValue(ObjectId.fromString(n));
+ } catch (InvalidObjectIdException e) {
+ throw new CmdLineException(
+ owner,
+ String.format("expected SHA1 for option %s: %s", ((NamedOptionDef) option).name(), n),
+ e);
+ }
return 1;
}
diff --git a/java/com/google/gerrit/server/change/ChangeInserter.java b/java/com/google/gerrit/server/change/ChangeInserter.java
index d575324..2883ef8 100644
--- a/java/com/google/gerrit/server/change/ChangeInserter.java
+++ b/java/com/google/gerrit/server/change/ChangeInserter.java
@@ -32,6 +32,7 @@
import com.google.errorprone.annotations.CanIgnoreReturnValue;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Account;
+import com.google.gerrit.entities.BooleanProjectConfig;
import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.LabelType;
@@ -653,6 +654,9 @@
}
private ImmutableList<InternalReviewerInput> getReviewerInputs() {
+ if (projectState.is(BooleanProjectConfig.SKIP_ADDING_AUTHOR_AND_COMMITTER_AS_REVIEWERS)) {
+ return reviewerInputs;
+ }
return Streams.concat(
reviewerInputs.stream(),
Streams.stream(
diff --git a/java/com/google/gerrit/server/change/GetRelatedChangesUtil.java b/java/com/google/gerrit/server/change/GetRelatedChangesUtil.java
index b1f9726..194a4f0 100644
--- a/java/com/google/gerrit/server/change/GetRelatedChangesUtil.java
+++ b/java/com/google/gerrit/server/change/GetRelatedChangesUtil.java
@@ -72,8 +72,8 @@
}
List<ChangeData> cds =
- InternalChangeQuery.byProjectGroups(
- queryProvider, indexConfig, changeData.project(), groups);
+ InternalChangeQuery.byBranchGroups(
+ queryProvider, indexConfig, changeData.change().getDest(), groups);
if (cds.isEmpty()) {
return Collections.emptyList();
}
diff --git a/java/com/google/gerrit/server/change/LabelNormalizer.java b/java/com/google/gerrit/server/change/LabelNormalizer.java
index 79e2054..b1fcf48 100644
--- a/java/com/google/gerrit/server/change/LabelNormalizer.java
+++ b/java/com/google/gerrit/server/change/LabelNormalizer.java
@@ -21,6 +21,7 @@
import com.google.auto.value.AutoValue;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Iterables;
import com.google.common.collect.Streams;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.LabelType;
@@ -43,6 +44,16 @@
* what labels are defined for the project. The label definition can change between the time a vote
* is originally made and a later point, for example when a change is submitted. This class
* normalizes old votes against current project configuration.
+ *
+ * <p>Normalizing a vote means making it compliant with the current label definition:
+ *
+ * <ul>
+ * <li>If the voting value is greater than the max allowed value according to the label
+ * definition, the voting value is changed to the max allowed value.
+ * <li>If the voting value is lower than the min allowed value according to the label definition,
+ * the voting value is changed to the min allowed value.
+ * <li>If the label definition for a vote is missing, the vote is deleted.
+ * </ul>
*/
@Singleton
public class LabelNormalizer {
@@ -121,6 +132,20 @@
return Result.create(unchanged, updated, deleted);
}
+ /**
+ * Returns a copy of the given approval normalized to the defined ranges for the label type. If
+ * the approval is for an unknown label {@link Optional#empty()} is returned
+ *
+ * @param notes change notes containing the given approval
+ * @param approval approval that should be normalized
+ */
+ public Optional<PatchSetApproval> normalize(ChangeNotes notes, PatchSetApproval approval) {
+ Result result = normalize(notes, ImmutableSet.of(approval));
+ return Optional.ofNullable(
+ Iterables.getFirst(
+ result.unchanged(), Iterables.getFirst(result.updated(), /* defaultValue= */ null)));
+ }
+
private PatchSetApproval applyTypeFloor(LabelType lt, PatchSetApproval a) {
PatchSetApproval.Builder b = a.toBuilder();
LabelValue atMin = lt.getMin();
diff --git a/java/com/google/gerrit/server/change/PatchSetInserter.java b/java/com/google/gerrit/server/change/PatchSetInserter.java
index f7bec1c0..1fe67af 100644
--- a/java/com/google/gerrit/server/change/PatchSetInserter.java
+++ b/java/com/google/gerrit/server/change/PatchSetInserter.java
@@ -15,6 +15,7 @@
package com.google.gerrit.server.change;
import static com.google.common.base.Preconditions.checkState;
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.gerrit.server.notedb.ReviewerStateInternal.CC;
import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
import static com.google.gerrit.server.project.ProjectCache.illegalState;
@@ -368,7 +369,9 @@
ctx,
patchSet,
mailMessage,
- approvalCopierResult.outdatedApprovals(),
+ approvalCopierResult.outdatedApprovals().stream()
+ .map(ApprovalCopier.Result.PatchSetApprovalData::patchSetApproval)
+ .collect(toImmutableSet()),
oldReviewers.byState(REVIEWER),
oldReviewers.byState(CC),
changeKind,
diff --git a/java/com/google/gerrit/server/events/EventFactory.java b/java/com/google/gerrit/server/events/EventFactory.java
index 60e30bc..cd7e29a 100644
--- a/java/com/google/gerrit/server/events/EventFactory.java
+++ b/java/com/google/gerrit/server/events/EventFactory.java
@@ -295,8 +295,8 @@
// Find changes in the same related group as this patch set, having a patch
// set whose parent matches this patch set's revision.
for (ChangeData cd :
- InternalChangeQuery.byProjectGroups(
- queryProvider, indexConfig, change.getProject(), currentPs.groups())) {
+ InternalChangeQuery.byBranchGroups(
+ queryProvider, indexConfig, change.getDest(), currentPs.groups())) {
PATCH_SETS:
for (PatchSet ps : cd.patchSets()) {
RevCommit commit = rw.parseCommit(ps.commitId());
diff --git a/java/com/google/gerrit/server/git/receive/ReplaceOp.java b/java/com/google/gerrit/server/git/receive/ReplaceOp.java
index 6e5cfff..0e17342 100644
--- a/java/com/google/gerrit/server/git/receive/ReplaceOp.java
+++ b/java/com/google/gerrit/server/git/receive/ReplaceOp.java
@@ -510,7 +510,9 @@
ctx,
newPatchSet,
mailMessage,
- approvalCopierResult.outdatedApprovals(),
+ approvalCopierResult.outdatedApprovals().stream()
+ .map(ApprovalCopier.Result.PatchSetApprovalData::patchSetApproval)
+ .collect(toImmutableSet()),
Streams.concat(
oldRecipients.getReviewers().stream(),
reviewerAdditions.flattenResults(ReviewerOp.Result::addedReviewers).stream()
@@ -595,6 +597,7 @@
return Optional.of(
"The following approvals got outdated and were removed:\n"
+ approvalCopierResult.outdatedApprovals().stream()
+ .map(ApprovalCopier.Result.PatchSetApprovalData::patchSetApproval)
.map(
outdatedApproval ->
String.format(
diff --git a/java/com/google/gerrit/server/index/change/ChangeField.java b/java/com/google/gerrit/server/index/change/ChangeField.java
index e60a2fd..7aefa63 100644
--- a/java/com/google/gerrit/server/index/change/ChangeField.java
+++ b/java/com/google/gerrit/server/index/change/ChangeField.java
@@ -19,8 +19,6 @@
import static com.google.common.collect.ImmutableListMultimap.toImmutableListMultimap;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.gerrit.index.FieldDef.exact;
-import static com.google.gerrit.index.FieldDef.fullText;
-import static com.google.gerrit.index.FieldDef.intRange;
import static com.google.gerrit.index.FieldDef.integer;
import static com.google.gerrit.index.FieldDef.prefix;
import static com.google.gerrit.index.FieldDef.storedOnly;
@@ -452,7 +450,7 @@
* Users included in the attention set of the change. This omits timestamp, reason and possible
* future fields.
*
- * @see #ATTENTION_SET_FULL
+ * @see #ATTENTION_SET_FULL_SPEC
*/
public static final IndexedField<ChangeData, Iterable<Integer>> ATTENTION_SET_USERS_FIELD =
IndexedField.<ChangeData>iterableIntegerBuilder("AttentionSetUsers")
@@ -464,6 +462,7 @@
/** Number of changes that contain attention set. */
public static final IndexedField<ChangeData, Integer> ATTENTION_SET_USERS_COUNT_FIELD =
IndexedField.<ChangeData>integerBuilder("AttentionSetUsersCount")
+ .stored()
.build(cd -> additionsOnly(cd.attentionSet()).size());
public static final IndexedField<ChangeData, Integer>.SearchSpec ATTENTION_SET_USERS_COUNT =
@@ -475,9 +474,11 @@
*
* @see #ATTENTION_SET_USERS
*/
- public static final FieldDef<ChangeData, Iterable<byte[]>> ATTENTION_SET_FULL =
- storedOnly(ChangeQueryBuilder.FIELD_ATTENTION_SET_FULL)
- .buildRepeatable(
+ public static final IndexedField<ChangeData, Iterable<byte[]>> ATTENTION_SET_FULL_FIELD =
+ IndexedField.<ChangeData>iterableByteArrayBuilder("AttentionSetFull")
+ .stored()
+ .required()
+ .build(
ChangeField::storedAttentionSet,
(cd, value) ->
parseAttentionSet(
@@ -486,6 +487,10 @@
.collect(toImmutableSet()),
cd));
+ public static final IndexedField<ChangeData, Iterable<byte[]>>.SearchSpec
+ ATTENTION_SET_FULL_SPEC =
+ ATTENTION_SET_FULL_FIELD.storedOnly(ChangeQueryBuilder.FIELD_ATTENTION_SET_FULL);
+
/** The user assigned to the change. */
public static final IndexedField<ChangeData, Integer> ASSIGNEE_FIELD =
IndexedField.<ChangeData>integerBuilder("Assignee")
@@ -749,25 +754,37 @@
}
/** Commit ID of any patch set on the change, using prefix match. */
- public static final FieldDef<ChangeData, Iterable<String>> COMMIT =
- prefix(ChangeQueryBuilder.FIELD_COMMIT).buildRepeatable(ChangeField::getRevisions);
+ public static final IndexedField<ChangeData, Iterable<String>> COMMIT_FIELD =
+ IndexedField.<ChangeData>iterableStringBuilder("Commit")
+ .size(40)
+ .required()
+ .build(ChangeField::getRevisions);
+
+ public static final IndexedField<ChangeData, Iterable<String>>.SearchSpec COMMIT_SPEC =
+ COMMIT_FIELD.prefix(ChangeQueryBuilder.FIELD_COMMIT);
/** Commit ID of any patch set on the change, using exact match. */
- public static final FieldDef<ChangeData, Iterable<String>> EXACT_COMMIT =
- exact(ChangeQueryBuilder.FIELD_EXACTCOMMIT).buildRepeatable(ChangeField::getRevisions);
+ public static final IndexedField<ChangeData, Iterable<String>>.SearchSpec EXACT_COMMIT_SPEC =
+ COMMIT_FIELD.exact(ChangeQueryBuilder.FIELD_EXACTCOMMIT);
private static ImmutableSet<String> getRevisions(ChangeData cd) {
return cd.patchSets().stream().map(ps -> ps.commitId().name()).collect(toImmutableSet());
}
/** Tracking id extracted from a footer. */
- public static final FieldDef<ChangeData, Iterable<String>> TR =
- exact(ChangeQueryBuilder.FIELD_TR)
- .buildRepeatable(cd -> ImmutableSet.copyOf(cd.trackingFooters().values()));
+ public static final IndexedField<ChangeData, Iterable<String>> TR_FIELD =
+ IndexedField.<ChangeData>iterableStringBuilder("TrackingFooter")
+ .build(cd -> ImmutableSet.copyOf(cd.trackingFooters().values()));
+
+ public static final IndexedField<ChangeData, Iterable<String>>.SearchSpec TR_SPEC =
+ TR_FIELD.exact(ChangeQueryBuilder.FIELD_TR);
/** List of labels on the current patch set including change owner votes. */
- public static final FieldDef<ChangeData, Iterable<String>> LABEL =
- exact("label2").buildRepeatable(cd -> getLabels(cd));
+ public static final IndexedField<ChangeData, Iterable<String>> LABEL_FIELD =
+ IndexedField.<ChangeData>iterableStringBuilder("Label").required().build(cd -> getLabels(cd));
+
+ public static final IndexedField<ChangeData, Iterable<String>>.SearchSpec LABEL_SPEC =
+ LABEL_FIELD.exact("label2");
private static Iterable<String> getLabels(ChangeData cd) {
Set<String> allApprovals = new HashSet<>();
@@ -1010,18 +1027,29 @@
}
/** Commit message of the current patch set. */
- public static final FieldDef<ChangeData, String> COMMIT_MESSAGE =
- fullText(ChangeQueryBuilder.FIELD_MESSAGE).build(ChangeData::commitMessage);
+ public static final IndexedField<ChangeData, String> COMMIT_MESSAGE_FIELD =
+ IndexedField.<ChangeData>stringBuilder("CommitMessage")
+ .required()
+ .build(ChangeData::commitMessage);
- /** Commit message of the current patch set. */
- public static final FieldDef<ChangeData, String> COMMIT_MESSAGE_EXACT =
- exact(ChangeQueryBuilder.FIELD_MESSAGE_EXACT)
+ public static final IndexedField<ChangeData, String>.SearchSpec COMMIT_MESSAGE =
+ COMMIT_MESSAGE_FIELD.fullText(ChangeQueryBuilder.FIELD_MESSAGE);
+
+ /** Commit message of the current patch set, used to exactly match the commit message */
+ public static final IndexedField<ChangeData, String> COMMIT_MESSAGE_EXACT_FIELD =
+ IndexedField.<ChangeData>stringBuilder("CommitMessageExact")
+ .required()
+ .description(
+ "Same as CommitMessage, but truncated, since supporting such large tokens may be problematic for indexes.")
.build(cd -> truncateStringValueToMaxTermLength(cd.commitMessage()));
+ public static final IndexedField<ChangeData, String>.SearchSpec COMMIT_MESSAGE_EXACT =
+ COMMIT_MESSAGE_EXACT_FIELD.exact(ChangeQueryBuilder.FIELD_MESSAGE_EXACT);
+
/** Summary or inline comment. */
- public static final FieldDef<ChangeData, Iterable<String>> COMMENT =
- fullText(ChangeQueryBuilder.FIELD_COMMENT)
- .buildRepeatable(
+ public static final IndexedField<ChangeData, Iterable<String>> COMMENT_FIELD =
+ IndexedField.<ChangeData>iterableStringBuilder("Comment")
+ .build(
cd ->
Stream.concat(
cd.publishedComments().stream().map(c -> c.message),
@@ -1032,22 +1060,35 @@
cd.messages().stream().map(ChangeMessage::getMessage))
.collect(toSet()));
+ public static final IndexedField<ChangeData, Iterable<String>>.SearchSpec COMMENT_SPEC =
+ COMMENT_FIELD.fullText(ChangeQueryBuilder.FIELD_COMMENT);
+
/** Number of unresolved comment threads of the change, including robot comments. */
- public static final FieldDef<ChangeData, Integer> UNRESOLVED_COMMENT_COUNT =
- intRange(ChangeQueryBuilder.FIELD_UNRESOLVED_COMMENT_COUNT)
+ public static final IndexedField<ChangeData, Integer> UNRESOLVED_COMMENT_COUNT_FIELD =
+ IndexedField.<ChangeData>integerBuilder("UnresolvedCommentCount")
+ .stored()
.build(
ChangeData::unresolvedCommentCount,
(cd, field) -> cd.setUnresolvedCommentCount(field));
+ public static final IndexedField<ChangeData, Integer>.SearchSpec UNRESOLVED_COMMENT_COUNT_SPEC =
+ UNRESOLVED_COMMENT_COUNT_FIELD.integerRange(
+ ChangeQueryBuilder.FIELD_UNRESOLVED_COMMENT_COUNT);
+
/** Total number of published inline comments of the change, including robot comments. */
- public static final FieldDef<ChangeData, Integer> TOTAL_COMMENT_COUNT =
- intRange("total_comments")
+ public static final IndexedField<ChangeData, Integer> TOTAL_COMMENT_COUNT_FIELD =
+ IndexedField.<ChangeData>integerBuilder("TotalCommentCount")
+ .stored()
.build(ChangeData::totalCommentCount, (cd, field) -> cd.setTotalCommentCount(field));
+ public static final IndexedField<ChangeData, Integer>.SearchSpec TOTAL_COMMENT_COUNT_SPEC =
+ TOTAL_COMMENT_COUNT_FIELD.integerRange("total_comments");
+
/** Whether the change is mergeable. */
- public static final FieldDef<ChangeData, String> MERGEABLE =
- exact(ChangeQueryBuilder.FIELD_MERGEABLE)
+ public static final IndexedField<ChangeData, String> MERGEABLE_FIELD =
+ IndexedField.<ChangeData>stringBuilder("Mergeable")
.stored()
+ .size(1)
.build(
cd -> {
Boolean m = cd.isMergeable();
@@ -1058,10 +1099,14 @@
},
(cd, field) -> cd.setMergeable(field == null ? false : field.equals("1")));
+ public static final IndexedField<ChangeData, String>.SearchSpec MERGEABLE_SPEC =
+ MERGEABLE_FIELD.exact(ChangeQueryBuilder.FIELD_MERGEABLE);
+
/** Whether the change is a merge commit. */
- public static final FieldDef<ChangeData, String> MERGE =
- exact(ChangeQueryBuilder.FIELD_MERGE)
+ public static final IndexedField<ChangeData, String> MERGE_FIELD =
+ IndexedField.<ChangeData>stringBuilder("Merge")
.stored()
+ .size(1)
.build(
cd -> {
Boolean m = cd.isMerge();
@@ -1071,15 +1116,23 @@
return m ? "1" : "0";
});
+ public static final IndexedField<ChangeData, String>.SearchSpec MERGE_SPEC =
+ MERGE_FIELD.exact(ChangeQueryBuilder.FIELD_MERGE);
+
/** Whether the change is a cherry pick of another change. */
- public static final FieldDef<ChangeData, String> CHERRY_PICK =
- exact(ChangeQueryBuilder.FIELD_CHERRYPICK)
+ public static final IndexedField<ChangeData, String> CHERRY_PICK_FIELD =
+ IndexedField.<ChangeData>stringBuilder("CherryPick")
.stored()
+ .size(1)
.build(cd -> cd.change().getCherryPickOf() != null ? "1" : "0");
+ public static final IndexedField<ChangeData, String>.SearchSpec CHERRY_PICK_SPEC =
+ CHERRY_PICK_FIELD.exact(ChangeQueryBuilder.FIELD_CHERRYPICK);
+
/** The number of inserted lines in this change. */
- public static final FieldDef<ChangeData, Integer> ADDED =
- intRange(ChangeQueryBuilder.FIELD_ADDED)
+ public static final IndexedField<ChangeData, Integer> ADDED_LINES_FIELD =
+ IndexedField.<ChangeData>integerBuilder("AddedLines")
+ .stored()
.build(
cd -> cd.changedLines().isPresent() ? cd.changedLines().get().insertions : null,
(cd, field) -> {
@@ -1088,9 +1141,13 @@
}
});
+ public static final IndexedField<ChangeData, Integer>.SearchSpec ADDED_LINES_SPEC =
+ ADDED_LINES_FIELD.integerRange(ChangeQueryBuilder.FIELD_ADDED);
+
/** The number of deleted lines in this change. */
- public static final FieldDef<ChangeData, Integer> DELETED =
- intRange(ChangeQueryBuilder.FIELD_DELETED)
+ public static final IndexedField<ChangeData, Integer> DELETED_LINES_FIELD =
+ IndexedField.<ChangeData>integerBuilder("DeletedLines")
+ .stored()
.build(
cd -> cd.changedLines().isPresent() ? cd.changedLines().get().deletions : null,
(cd, field) -> {
@@ -1099,28 +1156,49 @@
}
});
+ public static final IndexedField<ChangeData, Integer>.SearchSpec DELETED_LINES_SPEC =
+ DELETED_LINES_FIELD.integerRange(ChangeQueryBuilder.FIELD_DELETED);
+
/** The total number of modified lines in this change. */
- public static final FieldDef<ChangeData, Integer> DELTA =
- intRange(ChangeQueryBuilder.FIELD_DELTA)
+ public static final IndexedField<ChangeData, Integer> DELTA_LINES_FIELD =
+ IndexedField.<ChangeData>integerBuilder("DeltaLines")
+ .stored()
.build(cd -> cd.changedLines().map(c -> c.insertions + c.deletions).orElse(null));
+ public static final IndexedField<ChangeData, Integer>.SearchSpec DELTA_LINES_SPEC =
+ DELTA_LINES_FIELD.integerRange(ChangeQueryBuilder.FIELD_DELTA);
+
/** Determines if this change is private. */
- public static final FieldDef<ChangeData, String> PRIVATE =
- exact(ChangeQueryBuilder.FIELD_PRIVATE).build(cd -> cd.change().isPrivate() ? "1" : "0");
+ public static final IndexedField<ChangeData, String> PRIVATE_FIELD =
+ IndexedField.<ChangeData>stringBuilder("Private")
+ .size(1)
+ .build(cd -> cd.change().isPrivate() ? "1" : "0");
+
+ public static final IndexedField<ChangeData, String>.SearchSpec PRIVATE_SPEC =
+ PRIVATE_FIELD.exact(ChangeQueryBuilder.FIELD_PRIVATE);
/** Determines if this change is work in progress. */
- public static final FieldDef<ChangeData, String> WIP =
- exact(ChangeQueryBuilder.FIELD_WIP).build(cd -> cd.change().isWorkInProgress() ? "1" : "0");
+ public static final IndexedField<ChangeData, String> WIP_FIELD =
+ IndexedField.<ChangeData>stringBuilder("WIP")
+ .size(1)
+ .build(cd -> cd.change().isWorkInProgress() ? "1" : "0");
+
+ public static final IndexedField<ChangeData, String>.SearchSpec WIP_SPEC =
+ WIP_FIELD.exact(ChangeQueryBuilder.FIELD_WIP);
/** Determines if this change has started review. */
- public static final FieldDef<ChangeData, String> STARTED =
- exact(ChangeQueryBuilder.FIELD_STARTED)
+ public static final IndexedField<ChangeData, String> STARTED_FIELD =
+ IndexedField.<ChangeData>stringBuilder("ReviewStarted")
+ .size(1)
.build(cd -> cd.change().hasReviewStarted() ? "1" : "0");
+ public static final IndexedField<ChangeData, String>.SearchSpec STARTED_SPEC =
+ STARTED_FIELD.exact(ChangeQueryBuilder.FIELD_STARTED);
+
/** Users who have commented on this change. */
- public static final FieldDef<ChangeData, Iterable<Integer>> COMMENTBY =
- integer(ChangeQueryBuilder.FIELD_COMMENTBY)
- .buildRepeatable(
+ public static final IndexedField<ChangeData, Iterable<Integer>> COMMENTBY_FIELD =
+ IndexedField.<ChangeData>iterableIntegerBuilder("CommentBy")
+ .build(
cd ->
Stream.concat(
cd.messages().stream().map(ChangeMessage::getAuthor),
@@ -1129,6 +1207,9 @@
.map(Account.Id::get)
.collect(toSet()));
+ public static final IndexedField<ChangeData, Iterable<Integer>>.SearchSpec COMMENTBY_SPEC =
+ COMMENTBY_FIELD.integer(ChangeQueryBuilder.FIELD_COMMENTBY);
+
/** Star labels on this change in the format: <account-id>:<label> */
public static final FieldDef<ChangeData, Iterable<String>> STAR =
exact(ChangeQueryBuilder.FIELD_STAR)
diff --git a/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java b/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java
index 3e43505..2af5d60 100644
--- a/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java
+++ b/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java
@@ -35,52 +35,42 @@
schema(
/* version= */ 74,
ImmutableList.of(
- ChangeField.ADDED,
ChangeField.APPROVAL,
- ChangeField.ATTENTION_SET_FULL,
ChangeField.CHANGE,
- ChangeField.CHERRY_PICK,
- ChangeField.COMMENT,
- ChangeField.COMMENTBY,
- ChangeField.COMMIT,
- ChangeField.COMMIT_MESSAGE,
- ChangeField.DELETED,
- ChangeField.DELTA,
ChangeField.DRAFTBY,
ChangeField.EDITBY,
- ChangeField.EXACT_COMMIT,
ChangeField.GROUP,
ChangeField.ID,
- ChangeField.LABEL,
ChangeField.LEGACY_ID_STR,
- ChangeField.MERGE,
- ChangeField.MERGEABLE,
ChangeField.PATCH_SET,
- ChangeField.PRIVATE,
ChangeField.REF_STATE,
ChangeField.REF_STATE_PATTERN,
ChangeField.REVIEWEDBY,
ChangeField.STAR,
ChangeField.STARBY,
- ChangeField.STARTED,
ChangeField.STORED_SUBMIT_RECORD_LENIENT,
ChangeField.STORED_SUBMIT_RECORD_STRICT,
ChangeField.STORED_SUBMIT_REQUIREMENTS,
ChangeField.SUBMIT_RECORD,
ChangeField.SUBMIT_RULE_RESULT,
- ChangeField.TOTAL_COMMENT_COUNT,
- ChangeField.TR,
- ChangeField.UNRESOLVED_COMMENT_COUNT,
- ChangeField.UPDATED,
- ChangeField.WIP),
+ ChangeField.UPDATED),
ImmutableList.<IndexedField<ChangeData, ?>>of(
+ ChangeField.ADDED_LINES_FIELD,
ChangeField.ASSIGNEE_FIELD,
+ ChangeField.ATTENTION_SET_FULL_FIELD,
ChangeField.ATTENTION_SET_USERS_COUNT_FIELD,
ChangeField.ATTENTION_SET_USERS_FIELD,
ChangeField.AUTHOR_PARTS_FIELD,
+ ChangeField.CHERRY_PICK_FIELD,
ChangeField.CHERRY_PICK_OF_CHANGE_FIELD,
ChangeField.CHERRY_PICK_OF_PATCHSET_FIELD,
+ ChangeField.COMMENTBY_FIELD,
+ ChangeField.COMMENT_FIELD,
ChangeField.COMMITTER_PARTS_FIELD,
+ ChangeField.COMMIT_FIELD,
+ ChangeField.COMMIT_MESSAGE_FIELD,
+ ChangeField.DELETED_LINES_FIELD,
+ ChangeField.DELTA_LINES_FIELD,
ChangeField.DIRECTORY_FIELD,
ChangeField.EXACT_AUTHOR_FIELD,
ChangeField.EXACT_COMMITTER_FIELD,
@@ -91,32 +81,51 @@
ChangeField.HASHTAG_FIELD,
ChangeField.IS_PURE_REVERT_FIELD,
ChangeField.IS_SUBMITTABLE_FIELD,
+ ChangeField.LABEL_FIELD,
+ ChangeField.MERGEABLE_FIELD,
ChangeField.MERGED_ON_FIELD,
+ ChangeField.MERGE_FIELD,
ChangeField.ONLY_EXTENSIONS_FIELD,
ChangeField.OWNER_FIELD,
ChangeField.PATH_FIELD,
ChangeField.PENDING_REVIEWER_BY_EMAIL_FIELD,
ChangeField.PENDING_REVIEWER_FIELD,
+ ChangeField.PRIVATE_FIELD,
ChangeField.PROJECT_FIELD,
ChangeField.REF_FIELD,
ChangeField.REVERT_OF_FIELD,
ChangeField.REVIEWER_BY_EMAIL_FIELD,
ChangeField.REVIEWER_FIELD,
+ ChangeField.STARTED_FIELD,
ChangeField.STATUS_FIELD,
ChangeField.SUBMISSIONID_FIELD,
ChangeField.TOPIC_FIELD,
- ChangeField.UPLOADER_FIELD),
+ ChangeField.TOTAL_COMMENT_COUNT_FIELD,
+ ChangeField.TR_FIELD,
+ ChangeField.UNRESOLVED_COMMENT_COUNT_FIELD,
+ ChangeField.UPLOADER_FIELD,
+ ChangeField.WIP_FIELD),
ImmutableList.<IndexedField<ChangeData, ?>.SearchSpec>of(
+ ChangeField.ADDED_LINES_SPEC,
ChangeField.ASSIGNEE_SPEC,
+ ChangeField.ATTENTION_SET_FULL_SPEC,
ChangeField.ATTENTION_SET_USERS,
ChangeField.ATTENTION_SET_USERS_COUNT,
ChangeField.AUTHOR_PARTS_SPEC,
ChangeField.CHERRY_PICK_OF_CHANGE,
ChangeField.CHERRY_PICK_OF_PATCHSET,
+ ChangeField.CHERRY_PICK_SPEC,
+ ChangeField.COMMENTBY_SPEC,
+ ChangeField.COMMENT_SPEC,
ChangeField.COMMITTER_PARTS_SPEC,
+ ChangeField.COMMIT_MESSAGE,
+ ChangeField.COMMIT_SPEC,
+ ChangeField.DELETED_LINES_SPEC,
+ ChangeField.DELTA_LINES_SPEC,
ChangeField.DIRECTORY_SPEC,
ChangeField.EXACT_AUTHOR_SPEC,
ChangeField.EXACT_COMMITTER_SPEC,
+ ChangeField.EXACT_COMMIT_SPEC,
ChangeField.EXACT_TOPIC,
ChangeField.EXTENSION_SPEC,
ChangeField.FILE_PART_SPEC,
@@ -127,21 +136,30 @@
ChangeField.HASHTAG_SPEC,
ChangeField.IS_PURE_REVERT_SPEC,
ChangeField.IS_SUBMITTABLE_SPEC,
+ ChangeField.LABEL_SPEC,
+ ChangeField.MERGEABLE_SPEC,
ChangeField.MERGED_ON_SPEC,
+ ChangeField.MERGE_SPEC,
ChangeField.ONLY_EXTENSIONS_SPEC,
ChangeField.OWNER_SPEC,
ChangeField.PATH_SPEC,
ChangeField.PENDING_REVIEWER_BY_EMAIL,
ChangeField.PENDING_REVIEWER_SPEC,
+ ChangeField.PRIVATE_SPEC,
ChangeField.PROJECTS_SPEC,
ChangeField.PROJECT_SPEC,
ChangeField.REF_SPEC,
ChangeField.REVERT_OF,
ChangeField.REVIEWER_BY_EMAIL,
ChangeField.REVIEWER_SPEC,
+ ChangeField.STARTED_SPEC,
ChangeField.STATUS_SPEC,
ChangeField.SUBMISSIONID_SPEC,
- ChangeField.UPLOADER_SPEC));
+ ChangeField.TOTAL_COMMENT_COUNT_SPEC,
+ ChangeField.TR_SPEC,
+ ChangeField.UNRESOLVED_COMMENT_COUNT_SPEC,
+ ChangeField.UPLOADER_SPEC,
+ ChangeField.WIP_SPEC));
/**
* Added new field {@link ChangeField#PREFIX_HASHTAG} and {@link ChangeField#PREFIX_TOPIC} to
@@ -167,7 +185,11 @@
/** Added new field {@link ChangeField#COMMIT_MESSAGE_EXACT}. */
@Deprecated
static final Schema<ChangeData> V77 =
- new Schema.Builder<ChangeData>().add(V76).add(ChangeField.COMMIT_MESSAGE_EXACT).build();
+ new Schema.Builder<ChangeData>()
+ .add(V76)
+ .addIndexedFields(ChangeField.COMMIT_MESSAGE_EXACT_FIELD)
+ .addSearchSpecs(ChangeField.COMMIT_MESSAGE_EXACT)
+ .build();
// Upgrade Lucene to 7.x requires reindexing.
@Deprecated static final Schema<ChangeData> V78 = schema(V77);
diff --git a/java/com/google/gerrit/server/project/BooleanProjectConfigTransformations.java b/java/com/google/gerrit/server/project/BooleanProjectConfigTransformations.java
index ae9828a..9ccbf90 100644
--- a/java/com/google/gerrit/server/project/BooleanProjectConfigTransformations.java
+++ b/java/com/google/gerrit/server/project/BooleanProjectConfigTransformations.java
@@ -71,6 +71,11 @@
.put(
BooleanProjectConfig.WORK_IN_PROGRESS_BY_DEFAULT,
new Mapper(i -> i.workInProgressByDefault, (i, v) -> i.workInProgressByDefault = v))
+ .put(
+ BooleanProjectConfig.SKIP_ADDING_AUTHOR_AND_COMMITTER_AS_REVIEWERS,
+ new Mapper(
+ i -> i.skipAddingAuthorAndCommitterAsReviewers,
+ (i, v) -> i.skipAddingAuthorAndCommitterAsReviewers = v))
.build();
static {
diff --git a/java/com/google/gerrit/server/project/CreateProjectArgs.java b/java/com/google/gerrit/server/project/CreateProjectArgs.java
index 196873f..8da0510 100644
--- a/java/com/google/gerrit/server/project/CreateProjectArgs.java
+++ b/java/com/google/gerrit/server/project/CreateProjectArgs.java
@@ -49,7 +49,7 @@
newChangeForAllNotInTarget = InheritableBoolean.INHERIT;
enableSignedPush = InheritableBoolean.INHERIT;
requireSignedPush = InheritableBoolean.INHERIT;
- submitType = SubmitType.MERGE_IF_NECESSARY;
+ submitType = SubmitType.INHERIT;
rejectEmptyCommit = InheritableBoolean.INHERIT;
}
diff --git a/java/com/google/gerrit/server/project/ProjectConfig.java b/java/com/google/gerrit/server/project/ProjectConfig.java
index a964ee1..fc1256e 100644
--- a/java/com/google/gerrit/server/project/ProjectConfig.java
+++ b/java/com/google/gerrit/server/project/ProjectConfig.java
@@ -1143,9 +1143,11 @@
error(
String.format(
"Invalid %s for label \"%s\". Valid names are: %s",
- KEY_FUNCTION, name, Joiner.on(", ").join(LabelFunction.ALL.keySet())));
+ KEY_FUNCTION,
+ name,
+ Joiner.on(", ").join(LabelFunction.ALL_NON_DEPRECATED.keySet())));
}
- label.setFunction(function.orElse(null));
+ function.ifPresent(label::setFunction);
label.setCopyCondition(rc.getString(LABEL, name, KEY_COPY_CONDITION));
if (!values.isEmpty()) {
diff --git a/java/com/google/gerrit/server/project/RefUtil.java b/java/com/google/gerrit/server/project/RefUtil.java
index e86ad41..07f7ba5 100644
--- a/java/com/google/gerrit/server/project/RefUtil.java
+++ b/java/com/google/gerrit/server/project/RefUtil.java
@@ -23,6 +23,7 @@
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
import java.io.IOException;
import java.util.Collections;
+import org.eclipse.jgit.errors.AmbiguousObjectException;
import org.eclipse.jgit.errors.IncorrectObjectTypeException;
import org.eclipse.jgit.errors.MissingObjectException;
import org.eclipse.jgit.errors.RevisionSyntaxException;
@@ -49,6 +50,9 @@
} catch (RevisionSyntaxException e) {
throw new UnprocessableEntityException(
String.format("base revision \"%s\" is invalid", baseRevision), e);
+ } catch (AmbiguousObjectException e) {
+ throw new UnprocessableEntityException(
+ String.format("base revision \"%s\" is ambiguous", baseRevision), e);
}
}
diff --git a/java/com/google/gerrit/server/query/change/AddedPredicate.java b/java/com/google/gerrit/server/query/change/AddedPredicate.java
index 1f526c5..698884c 100644
--- a/java/com/google/gerrit/server/query/change/AddedPredicate.java
+++ b/java/com/google/gerrit/server/query/change/AddedPredicate.java
@@ -19,11 +19,11 @@
public class AddedPredicate extends IntegerRangeChangePredicate {
public AddedPredicate(String value) throws QueryParseException {
- super(ChangeField.ADDED, value);
+ super(ChangeField.ADDED_LINES_SPEC, value);
}
@Override
protected Integer getValueInt(ChangeData changeData) {
- return ChangeField.ADDED.get(changeData);
+ return ChangeField.ADDED_LINES_SPEC.get(changeData);
}
}
diff --git a/java/com/google/gerrit/server/query/change/ChangePredicates.java b/java/com/google/gerrit/server/query/change/ChangePredicates.java
index 70f241c..6b8cdd8 100644
--- a/java/com/google/gerrit/server/query/change/ChangePredicates.java
+++ b/java/com/google/gerrit/server/query/change/ChangePredicates.java
@@ -66,7 +66,7 @@
* com.google.gerrit.entities.Account.Id}.
*/
public static Predicate<ChangeData> commentBy(Account.Id id) {
- return new ChangeIndexPredicate(ChangeField.COMMENTBY, id.toString());
+ return new ChangeIndexPredicate(ChangeField.COMMENTBY_SPEC, id.toString());
}
/**
@@ -268,7 +268,7 @@
/** Returns a predicate that matches changes with the provided {@code trackingId}. */
public static Predicate<ChangeData> trackingId(String trackingId) {
- return new ChangeIndexCardinalPredicate(ChangeField.TR, trackingId, 5);
+ return new ChangeIndexCardinalPredicate(ChangeField.TR_SPEC, trackingId, 5);
}
/** Returns a predicate that matches changes authored by the provided {@code exactAuthor}. */
@@ -319,9 +319,9 @@
*/
public static Predicate<ChangeData> commitPrefix(String commitId) {
if (commitId.length() == ObjectIds.STR_LEN) {
- return new ChangeIndexCardinalPredicate(ChangeField.EXACT_COMMIT, commitId, 5);
+ return new ChangeIndexCardinalPredicate(ChangeField.EXACT_COMMIT_SPEC, commitId, 5);
}
- return new ChangeIndexCardinalPredicate(ChangeField.COMMIT, commitId, 5);
+ return new ChangeIndexCardinalPredicate(ChangeField.COMMIT_SPEC, commitId, 5);
}
/**
@@ -337,7 +337,7 @@
* comment on any patch set of the change. Uses full-text search semantics.
*/
public static Predicate<ChangeData> comment(String comment) {
- return new ChangeIndexPredicate(ChangeField.COMMENT, comment);
+ return new ChangeIndexPredicate(ChangeField.COMMENT_SPEC, comment);
}
/**
diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
index fe4cb5d..b433b25 100644
--- a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
@@ -678,7 +678,8 @@
if ("reviewer".equalsIgnoreCase(value)) {
return Predicate.and(
- Predicate.not(new BooleanPredicate(ChangeField.WIP)), ReviewerPredicate.reviewer(self()));
+ Predicate.not(new BooleanPredicate(ChangeField.WIP_SPEC)),
+ ReviewerPredicate.reviewer(self()));
}
if ("cc".equalsIgnoreCase(value)) {
@@ -689,16 +690,16 @@
if (!args.indexMergeable) {
throw new QueryParseException("'is:mergeable' operator is not supported by server");
}
- return new BooleanPredicate(ChangeField.MERGEABLE);
+ return new BooleanPredicate(ChangeField.MERGEABLE_SPEC);
}
if ("merge".equalsIgnoreCase(value)) {
- checkFieldAvailable(ChangeField.MERGE, "is:merge");
- return new BooleanPredicate(ChangeField.MERGE);
+ checkFieldAvailable(ChangeField.MERGE_SPEC, "is:merge");
+ return new BooleanPredicate(ChangeField.MERGE_SPEC);
}
if ("private".equalsIgnoreCase(value)) {
- return new BooleanPredicate(ChangeField.PRIVATE);
+ return new BooleanPredicate(ChangeField.PRIVATE_SPEC);
}
if ("attention".equalsIgnoreCase(value)) {
@@ -736,17 +737,17 @@
}
if ("started".equalsIgnoreCase(value)) {
- checkFieldAvailable(ChangeField.STARTED, "is:started");
- return new BooleanPredicate(ChangeField.STARTED);
+ checkFieldAvailable(ChangeField.STARTED_SPEC, "is:started");
+ return new BooleanPredicate(ChangeField.STARTED_SPEC);
}
if ("wip".equalsIgnoreCase(value)) {
- return new BooleanPredicate(ChangeField.WIP);
+ return new BooleanPredicate(ChangeField.WIP_SPEC);
}
if ("cherrypick".equalsIgnoreCase(value)) {
- checkFieldAvailable(ChangeField.CHERRY_PICK, "is:cherrypick");
- return new BooleanPredicate(ChangeField.CHERRY_PICK);
+ checkFieldAvailable(ChangeField.CHERRY_PICK_SPEC, "is:cherrypick");
+ return new BooleanPredicate(ChangeField.CHERRY_PICK_SPEC);
}
// for plugins the value will be operandName_pluginName
@@ -1328,7 +1329,7 @@
if (Objects.equals(byState, Predicate.<ChangeData>any())) {
return Predicate.any();
}
- return Predicate.and(Predicate.not(new BooleanPredicate(ChangeField.WIP)), byState);
+ return Predicate.and(Predicate.not(new BooleanPredicate(ChangeField.WIP_SPEC)), byState);
}
@Operator
diff --git a/java/com/google/gerrit/server/query/change/DeletedPredicate.java b/java/com/google/gerrit/server/query/change/DeletedPredicate.java
index d4bdc67..40e4c6e 100644
--- a/java/com/google/gerrit/server/query/change/DeletedPredicate.java
+++ b/java/com/google/gerrit/server/query/change/DeletedPredicate.java
@@ -19,11 +19,11 @@
public class DeletedPredicate extends IntegerRangeChangePredicate {
public DeletedPredicate(String value) throws QueryParseException {
- super(ChangeField.DELETED, value);
+ super(ChangeField.DELETED_LINES_SPEC, value);
}
@Override
protected Integer getValueInt(ChangeData changeData) {
- return ChangeField.DELETED.get(changeData);
+ return ChangeField.DELETED_LINES_SPEC.get(changeData);
}
}
diff --git a/java/com/google/gerrit/server/query/change/DeltaPredicate.java b/java/com/google/gerrit/server/query/change/DeltaPredicate.java
index 821ec94..e9eaa32 100644
--- a/java/com/google/gerrit/server/query/change/DeltaPredicate.java
+++ b/java/com/google/gerrit/server/query/change/DeltaPredicate.java
@@ -19,11 +19,11 @@
public class DeltaPredicate extends IntegerRangeChangePredicate {
public DeltaPredicate(String value) throws QueryParseException {
- super(ChangeField.DELTA, value);
+ super(ChangeField.DELTA_LINES_SPEC, value);
}
@Override
protected Integer getValueInt(ChangeData changeData) {
- return ChangeField.DELTA.get(changeData);
+ return ChangeField.DELTA_LINES_SPEC.get(changeData);
}
}
diff --git a/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java b/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java
index 5e8674e..5662e4d 100644
--- a/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java
+++ b/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java
@@ -59,7 +59,7 @@
int expVal,
Account.Id account,
@Nullable Integer count) {
- super(ChangeField.LABEL, ChangeField.formatLabel(label, expVal, account, count));
+ super(ChangeField.LABEL_SPEC, ChangeField.formatLabel(label, expVal, account, count));
this.permissionBackend = args.permissionBackend;
this.projectCache = args.projectCache;
this.userFactory = args.userFactory;
diff --git a/java/com/google/gerrit/server/query/change/InternalChangeQuery.java b/java/com/google/gerrit/server/query/change/InternalChangeQuery.java
index 99c1ca1..ccd645b 100644
--- a/java/com/google/gerrit/server/query/change/InternalChangeQuery.java
+++ b/java/com/google/gerrit/server/query/change/InternalChangeQuery.java
@@ -271,21 +271,21 @@
return query(ChangePredicates.submissionId(cs));
}
- private static Predicate<ChangeData> byProjectGroupsPredicate(
- IndexConfig indexConfig, Project.NameKey project, Collection<String> groups) {
- int n = indexConfig.maxTerms() - 1;
+ private static Predicate<ChangeData> byBranchGroupsPredicate(
+ IndexConfig indexConfig, BranchNameKey branchAndProject, Collection<String> groups) {
+ int n = indexConfig.maxTerms() - 2;
checkArgument(groups.size() <= n, "cannot exceed %s groups", n);
List<GroupPredicate> groupPredicates = new ArrayList<>(groups.size());
for (String g : groups) {
groupPredicates.add(new GroupPredicate(g));
}
- return and(project(project), or(groupPredicates));
+ return and(project(branchAndProject.project()), ref(branchAndProject), or(groupPredicates));
}
- public static ImmutableList<ChangeData> byProjectGroups(
+ public static ImmutableList<ChangeData> byBranchGroups(
Provider<InternalChangeQuery> queryProvider,
IndexConfig indexConfig,
- Project.NameKey project,
+ BranchNameKey branchAndProject,
Collection<String> groups) {
// These queries may be complex along multiple dimensions:
// * Many groups per change, if there are very many patch sets. This requires partitioning the
@@ -296,16 +296,17 @@
// InternalChangeQuery is single-use.
Supplier<InternalChangeQuery> querySupplier = () -> queryProvider.get().enforceVisibility(true);
- int batchSize = indexConfig.maxTerms() - 1;
+ int batchSize = indexConfig.maxTerms() - 2;
if (groups.size() <= batchSize) {
return queryExhaustively(
- querySupplier, byProjectGroupsPredicate(indexConfig, project, groups));
+ querySupplier, byBranchGroupsPredicate(indexConfig, branchAndProject, groups));
}
Set<Change.Id> seen = new HashSet<>();
ImmutableList.Builder<ChangeData> result = ImmutableList.builder();
for (List<String> part : Iterables.partition(groups, batchSize)) {
for (ChangeData cd :
- queryExhaustively(querySupplier, byProjectGroupsPredicate(indexConfig, project, part))) {
+ queryExhaustively(
+ querySupplier, byBranchGroupsPredicate(indexConfig, branchAndProject, part))) {
if (!seen.add(cd.getId())) {
result.add(cd);
}
diff --git a/java/com/google/gerrit/server/query/change/IsUnresolvedPredicate.java b/java/com/google/gerrit/server/query/change/IsUnresolvedPredicate.java
index 27309af..ffa29ba 100644
--- a/java/com/google/gerrit/server/query/change/IsUnresolvedPredicate.java
+++ b/java/com/google/gerrit/server/query/change/IsUnresolvedPredicate.java
@@ -23,11 +23,11 @@
}
public IsUnresolvedPredicate(String value) throws QueryParseException {
- super(ChangeField.UNRESOLVED_COMMENT_COUNT, value);
+ super(ChangeField.UNRESOLVED_COMMENT_COUNT_SPEC, value);
}
@Override
protected Integer getValueInt(ChangeData changeData) {
- return ChangeField.UNRESOLVED_COMMENT_COUNT.get(changeData);
+ return ChangeField.UNRESOLVED_COMMENT_COUNT_SPEC.get(changeData);
}
}
diff --git a/java/com/google/gerrit/server/query/change/MagicLabelPredicate.java b/java/com/google/gerrit/server/query/change/MagicLabelPredicate.java
index e890066..32a8fdf 100644
--- a/java/com/google/gerrit/server/query/change/MagicLabelPredicate.java
+++ b/java/com/google/gerrit/server/query/change/MagicLabelPredicate.java
@@ -39,7 +39,7 @@
Account.Id account,
@Nullable Integer count) {
super(
- ChangeField.LABEL,
+ ChangeField.LABEL_SPEC,
ChangeField.formatLabel(
magicLabelVote.label(), magicLabelVote.value().name(), account, count));
this.account = account;
diff --git a/java/com/google/gerrit/server/restapi/change/PostReviewOp.java b/java/com/google/gerrit/server/restapi/change/PostReviewOp.java
index 5ff0968..2b0b8c3 100644
--- a/java/com/google/gerrit/server/restapi/change/PostReviewOp.java
+++ b/java/com/google/gerrit/server/restapi/change/PostReviewOp.java
@@ -709,8 +709,9 @@
String msg = Strings.nullToEmpty(in.message).trim();
StringBuilder buf = new StringBuilder();
- for (LabelVote d : labelDelta) {
- buf.append(" ").append(d.format());
+ for (String formattedLabelVote :
+ labelDelta.stream().map(LabelVote::format).sorted().collect(toImmutableList())) {
+ buf.append(" ").append(formattedLabelVote);
}
if (comments.size() == 1) {
buf.append("\n\n(1 comment)");
diff --git a/javatests/com/google/gerrit/acceptance/api/change/CopiedApprovalsInChangeMessageIT.java b/javatests/com/google/gerrit/acceptance/api/change/CopiedApprovalsInChangeMessageIT.java
index f8cf5fd..2b1bef0 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/CopiedApprovalsInChangeMessageIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/CopiedApprovalsInChangeMessageIT.java
@@ -45,12 +45,29 @@
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.server.approval.ApprovalCopier;
import com.google.gerrit.server.approval.ApprovalsUtil;
+import com.google.gerrit.server.group.SystemGroupBackend;
import com.google.gerrit.server.project.testing.TestLabels;
import com.google.gerrit.server.util.AccountTemplateUtil;
import com.google.gerrit.server.util.time.TimeUtil;
import com.google.inject.Inject;
import org.junit.Test;
+/**
+ * Tests to verify that copied/outdated approvals are included into the change message that is
+ * posted on patch set creation. Includes verifying that the copied/outdated approvals in the change
+ * message are correctly formatted.
+ *
+ * <p>Some of the tests only verify the correct formatting of the copied/outdated approvals in the
+ * change message that is done by {@link
+ * ApprovalsUtil#formatApprovalCopierResult(com.google.gerrit.server.approval.ApprovalCopier.Result,
+ * LabelTypes)}. This method does the formatting based on the inputs that it gets, but it doesn't do
+ * any verification of these inputs. This means it's possible to provide inputs that are
+ * inconsistent with the approval copying logic in {@link ApprovalCopier}. E.g. it's possible to
+ * provide "is:MAX" as a passing atom for a "Code-Review-1" vote and have "is:MAX" highlighted as
+ * passing in the message although the "Code-Review-1" vote doesn't match with "is:MAX". For easier
+ * readability the formatting tests avoid using such inconsistent input data, but it's not
+ * impossible that in some cases we made a mistake and the input data is inconsistent.
+ */
public class CopiedApprovalsInChangeMessageIT extends AbstractDaemonTest {
@Inject private ApprovalsUtil approvalsUtil;
@Inject private ProjectOperations projectOperations;
@@ -98,7 +115,11 @@
PatchSetApproval patchSetApproval = createPatchSetApproval(admin, "Code-Review", 1);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
- /* copiedApprovals= */ ImmutableSet.of(patchSetApproval),
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of())),
/* outdatedApprovals= */ ImmutableSet.of());
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue("Copied Votes:\n* Code-Review+1 (label type is missing)\n");
@@ -111,7 +132,11 @@
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
/* copiedApprovals= */ ImmutableSet.of(),
- /* outdatedApprovals= */ ImmutableSet.of(patchSetApproval));
+ /* outdatedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of())));
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue("Outdated Votes:\n* Code-Review+1 (label type is missing)\n");
}
@@ -125,7 +150,11 @@
PatchSetApproval patchSetApproval = createPatchSetApproval(admin, "Code-Review", 1);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
- /* copiedApprovals= */ ImmutableSet.of(patchSetApproval),
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of())),
/* outdatedApprovals= */ ImmutableSet.of());
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue("Copied Votes:\n* Code-Review+1\n");
@@ -141,7 +170,11 @@
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
/* copiedApprovals= */ ImmutableSet.of(),
- /* outdatedApprovals= */ ImmutableSet.of(patchSetApproval));
+ /* outdatedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of())));
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue("Outdated Votes:\n* Code-Review+1\n");
}
@@ -153,13 +186,17 @@
ImmutableList.of(
createLabelType(
/* labelName= */ "Code-Review", /* copyCondition= */ "is:MIN OR is:MAX")));
- PatchSetApproval patchSetApproval = createPatchSetApproval(admin, "Code-Review", 1);
+ PatchSetApproval patchSetApproval = createPatchSetApproval(admin, "Code-Review", -2);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
- /* copiedApprovals= */ ImmutableSet.of(patchSetApproval),
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval,
+ /* passingAtoms= */ ImmutableSet.of("is:MIN"),
+ /* failingAtoms= */ ImmutableSet.of("is:MAX"))),
/* outdatedApprovals= */ ImmutableSet.of());
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
- .hasValue("Copied Votes:\n* Code-Review+1 (copy condition: \"is:MIN OR is:MAX\")\n");
+ .hasValue("Copied Votes:\n* Code-Review-2 (copy condition: \"**is:MIN** OR is:MAX\")\n");
}
@Test
@@ -168,14 +205,21 @@
new LabelTypes(
ImmutableList.of(
createLabelType(
- /* labelName= */ "Code-Review", /* copyCondition= */ "is:MIN OR is:MAX")));
- PatchSetApproval patchSetApproval = createPatchSetApproval(admin, "Code-Review", 1);
+ /* labelName= */ "Code-Review",
+ /* copyCondition= */ "changekind:TRIVIAL_REBASE is:MAX")));
+ PatchSetApproval patchSetApproval = createPatchSetApproval(admin, "Code-Review", 2);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
/* copiedApprovals= */ ImmutableSet.of(),
- /* outdatedApprovals= */ ImmutableSet.of(patchSetApproval));
+ /* outdatedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval,
+ /* passingAtoms= */ ImmutableSet.of("is:MAX"),
+ /* failingAtoms= */ ImmutableSet.of("changekind:TRIVIAL_REBASE"))));
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
- .hasValue("Outdated Votes:\n* Code-Review+1 (copy condition: \"is:MIN OR is:MAX\")\n");
+ .hasValue(
+ "Outdated Votes:\n* Code-Review+2 (copy condition:"
+ + " \"changekind:TRIVIAL_REBASE **is:MAX**\")\n");
}
@Test
@@ -189,7 +233,11 @@
PatchSetApproval patchSetApproval = createPatchSetApproval(admin, "Code-Review", 1);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
- /* copiedApprovals= */ ImmutableSet.of(patchSetApproval),
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of())),
/* outdatedApprovals= */ ImmutableSet.of());
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue(
@@ -208,7 +256,11 @@
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
/* copiedApprovals= */ ImmutableSet.of(),
- /* outdatedApprovals= */ ImmutableSet.of(patchSetApproval));
+ /* outdatedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of())));
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue(
"Outdated Votes:\n* Code-Review+1 (non-parseable copy condition: \"foo bar baz\")\n");
@@ -225,17 +277,22 @@
/* labelName= */ "Code-Review",
/* copyCondition= */ String.format(
"is:MIN OR (is:MAX approverin:%s)", groupUuid))));
- PatchSetApproval patchSetApproval = createPatchSetApproval(admin, "Code-Review", 1);
+ PatchSetApproval patchSetApproval = createPatchSetApproval(admin, "Code-Review", 2);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
- /* copiedApprovals= */ ImmutableSet.of(patchSetApproval),
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval,
+ /* passingAtoms= */ ImmutableSet.of(
+ "is:MAX", String.format("approverin:%s", groupUuid)),
+ /* failingAtoms= */ ImmutableSet.of("is:MIN"))),
/* outdatedApprovals= */ ImmutableSet.of());
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue(
String.format(
"Copied Votes:\n"
- + "* Code-Review+1 by %s"
- + " (copy condition: \"is:MIN OR (is:MAX approverin:%s)\")\n",
+ + "* Code-Review+2 by %s"
+ + " (copy condition: \"is:MIN OR (**is:MAX** **approverin:%s**)\")\n",
AccountTemplateUtil.getAccountTemplate(admin.id()), groupUuid));
}
@@ -254,12 +311,17 @@
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
/* copiedApprovals= */ ImmutableSet.of(),
- /* outdatedApprovals= */ ImmutableSet.of(patchSetApproval));
+ /* outdatedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval,
+ /* passingAtoms= */ ImmutableSet.of(String.format("approverin:%s", groupUuid)),
+ /* failingAtoms= */ ImmutableSet.of("is:MIN", "is:MAX"))));
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue(
String.format(
"Outdated Votes:\n"
- + "* Code-Review+1 by %s (copy condition: \"is:MIN OR (is:MAX approverin:%s)\")\n",
+ + "* Code-Review+1 by %s (copy condition: \"is:MIN"
+ + " OR (is:MAX **approverin:%s**)\")\n",
AccountTemplateUtil.getAccountTemplate(admin.id()), groupUuid));
}
@@ -275,10 +337,15 @@
/* labelName= */ "Code-Review",
/* copyCondition= */ String.format(
"is:MIN OR (is:MAX approverin:%s)", groupUuid))));
- PatchSetApproval patchSetApproval = createPatchSetApproval(admin, "Code-Review", 1);
+ PatchSetApproval patchSetApproval = createPatchSetApproval(admin, "Code-Review", 2);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
- /* copiedApprovals= */ ImmutableSet.of(patchSetApproval),
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval,
+ /* passingAtoms= */ ImmutableSet.of(
+ "is:MAX", String.format("approverin:%s", groupUuid)),
+ /* failingAtoms= */ ImmutableSet.of("is:MIN"))),
/* outdatedApprovals= */ ImmutableSet.of());
// Set 'user' as the current user in the request scope.
@@ -291,8 +358,8 @@
.hasValue(
String.format(
"Copied Votes:\n"
- + "* Code-Review+1 by %s"
- + " (copy condition: \"is:MIN OR (is:MAX approverin:%s)\")\n",
+ + "* Code-Review+2 by %s"
+ + " (copy condition: \"is:MIN OR (**is:MAX** **approverin:%s**)\")\n",
AccountTemplateUtil.getAccountTemplate(admin.id()), groupUuid));
}
@@ -313,7 +380,11 @@
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
/* copiedApprovals= */ ImmutableSet.of(),
- /* outdatedApprovals= */ ImmutableSet.of(patchSetApproval));
+ /* outdatedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval,
+ /* passingAtoms= */ ImmutableSet.of(String.format("approverin:%s", groupUuid)),
+ /* failingAtoms= */ ImmutableSet.of("is:MIN", "is:MAX"))));
// Set 'user' as the current user in the request scope.
// 'user' cannot see the Administrators group that is used in the copy condition.
@@ -325,7 +396,8 @@
.hasValue(
String.format(
"Outdated Votes:\n"
- + "* Code-Review+1 by %s (copy condition: \"is:MIN OR (is:MAX approverin:%s)\")\n",
+ + "* Code-Review+1 by %s (copy condition: \"is:MIN"
+ + " OR (is:MAX **approverin:%s**)\")\n",
AccountTemplateUtil.getAccountTemplate(admin.id()), groupUuid));
}
@@ -344,7 +416,11 @@
PatchSetApproval patchSetApproval = createPatchSetApproval(admin, "Code-Review", 1);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
- /* copiedApprovals= */ ImmutableSet.of(patchSetApproval),
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of())),
/* outdatedApprovals= */ ImmutableSet.of());
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue(
@@ -371,7 +447,11 @@
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
/* copiedApprovals= */ ImmutableSet.of(),
- /* outdatedApprovals= */ ImmutableSet.of(patchSetApproval));
+ /* outdatedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of())));
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue(
String.format(
@@ -388,7 +468,15 @@
PatchSetApproval patchSetApproval2 = createPatchSetApproval(user, "Code-Review", 1);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
- /* copiedApprovals= */ ImmutableSet.of(patchSetApproval1, patchSetApproval2),
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval1,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of()),
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval2,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of())),
/* outdatedApprovals= */ ImmutableSet.of());
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue("Copied Votes:\n* Code-Review+1 (label type is missing)\n");
@@ -401,7 +489,15 @@
PatchSetApproval patchSetApproval2 = createPatchSetApproval(user, "Verified", 1);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
- /* copiedApprovals= */ ImmutableSet.of(patchSetApproval1, patchSetApproval2),
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval1,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of()),
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval2,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of())),
/* outdatedApprovals= */ ImmutableSet.of());
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue(
@@ -417,7 +513,15 @@
PatchSetApproval patchSetApproval2 = createPatchSetApproval(user, "Code-Review", 1);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
- /* copiedApprovals= */ ImmutableSet.of(patchSetApproval1, patchSetApproval2),
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval1,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of()),
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval2,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of())),
/* outdatedApprovals= */ ImmutableSet.of());
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue("Copied Votes:\n* Code-Review+1, Code-Review+2 (label type is missing)\n");
@@ -433,7 +537,15 @@
PatchSetApproval patchSetApproval2 = createPatchSetApproval(user, "Code-Review", 1);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
- /* copiedApprovals= */ ImmutableSet.of(patchSetApproval1, patchSetApproval2),
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval1,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of()),
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval2,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of())),
/* outdatedApprovals= */ ImmutableSet.of());
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue("Copied Votes:\n* Code-Review+1\n");
@@ -450,7 +562,15 @@
PatchSetApproval patchSetApproval2 = createPatchSetApproval(user, "Verified", 1);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
- /* copiedApprovals= */ ImmutableSet.of(patchSetApproval1, patchSetApproval2),
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval1,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of()),
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval2,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of())),
/* outdatedApprovals= */ ImmutableSet.of());
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue("Copied Votes:\n* Code-Review+1\n* Verified+1\n");
@@ -466,7 +586,15 @@
PatchSetApproval patchSetApproval2 = createPatchSetApproval(user, "Code-Review", 1);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
- /* copiedApprovals= */ ImmutableSet.of(patchSetApproval1, patchSetApproval2),
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval1,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of()),
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval2,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of())),
/* outdatedApprovals= */ ImmutableSet.of());
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue("Copied Votes:\n* Code-Review+1, Code-Review+2\n");
@@ -480,14 +608,22 @@
ImmutableList.of(
createLabelType(
/* labelName= */ "Code-Review", /* copyCondition= */ "is:MIN OR is:MAX")));
- PatchSetApproval patchSetApproval1 = createPatchSetApproval(admin, "Code-Review", 1);
- PatchSetApproval patchSetApproval2 = createPatchSetApproval(user, "Code-Review", 1);
+ PatchSetApproval patchSetApproval1 = createPatchSetApproval(admin, "Code-Review", 2);
+ PatchSetApproval patchSetApproval2 = createPatchSetApproval(user, "Code-Review", 2);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
- /* copiedApprovals= */ ImmutableSet.of(patchSetApproval1, patchSetApproval2),
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval1,
+ /* passingAtoms= */ ImmutableSet.of("is:MAX"),
+ /* failingAtoms= */ ImmutableSet.of("is:MIN")),
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval2,
+ /* passingAtoms= */ ImmutableSet.of("is:MAX"),
+ /* failingAtoms= */ ImmutableSet.of("is:MIN"))),
/* outdatedApprovals= */ ImmutableSet.of());
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
- .hasValue("Copied Votes:\n* Code-Review+1 (copy condition: \"is:MIN OR is:MAX\")\n");
+ .hasValue("Copied Votes:\n* Code-Review+2 (copy condition: \"is:MIN OR **is:MAX**\")\n");
}
@Test
@@ -498,39 +634,92 @@
ImmutableList.of(
createLabelType(
/* labelName= */ "Code-Review", /* copyCondition= */ "is:MIN OR is:MAX"),
- createLabelType(
- /* labelName= */ "Verified", /* copyCondition= */ "is:MIN OR is:MAX")));
- PatchSetApproval patchSetApproval1 = createPatchSetApproval(admin, "Code-Review", 1);
+ LabelType.builder(
+ "Verified",
+ ImmutableList.of(
+ LabelValue.create((short) -1, "Fails"),
+ LabelValue.create((short) 0, "No Vote"),
+ LabelValue.create((short) 1, "Succeeds")))
+ .setCopyCondition("is:MIN OR is:MAX")
+ .build()));
+ PatchSetApproval patchSetApproval1 = createPatchSetApproval(admin, "Code-Review", 2);
PatchSetApproval patchSetApproval2 = createPatchSetApproval(user, "Verified", 1);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
- /* copiedApprovals= */ ImmutableSet.of(patchSetApproval1, patchSetApproval2),
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval1,
+ /* passingAtoms= */ ImmutableSet.of("is:MAX"),
+ /* failingAtoms= */ ImmutableSet.of("is:MIN")),
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval2,
+ /* passingAtoms= */ ImmutableSet.of("is:MAX"),
+ /* failingAtoms= */ ImmutableSet.of("is:MIN"))),
/* outdatedApprovals= */ ImmutableSet.of());
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue(
"Copied Votes:\n"
- + "* Code-Review+1 (copy condition: \"is:MIN OR is:MAX\")\n"
- + "* Verified+1 (copy condition: \"is:MIN OR is:MAX\")\n");
+ + "* Code-Review+2 (copy condition: \"is:MIN OR **is:MAX**\")\n"
+ + "* Verified+1 (copy condition: \"is:MIN OR **is:MAX**\")\n");
}
@Test
- public void formatMultipleApprovals_differentValue_withCopyCondition_noUserInPredicate()
- throws Exception {
+ public void
+ formatMultipleApprovals_differentValue_withCopyCondition_noUserInPredicate_samePassingAtoms()
+ throws Exception {
LabelTypes labelTypes =
new LabelTypes(
ImmutableList.of(
createLabelType(
- /* labelName= */ "Code-Review", /* copyCondition= */ "is:MIN OR is:MAX")));
+ /* labelName= */ "Code-Review", /* copyCondition= */ "changekind:REWORK")));
PatchSetApproval patchSetApproval1 = createPatchSetApproval(admin, "Code-Review", 2);
PatchSetApproval patchSetApproval2 = createPatchSetApproval(user, "Code-Review", 1);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
- /* copiedApprovals= */ ImmutableSet.of(patchSetApproval1, patchSetApproval2),
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval1,
+ /* passingAtoms= */ ImmutableSet.of("changekind:REWORK"),
+ /* failingAtoms= */ ImmutableSet.of()),
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval2,
+ /* passingAtoms= */ ImmutableSet.of("changekind:REWORK"),
+ /* failingAtoms= */ ImmutableSet.of())),
/* outdatedApprovals= */ ImmutableSet.of());
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue(
"Copied Votes:\n"
- + "* Code-Review+1, Code-Review+2 (copy condition: \"is:MIN OR is:MAX\")\n");
+ + "* Code-Review+1, Code-Review+2 (copy condition: \"**changekind:REWORK**\")\n");
+ }
+
+ @Test
+ public void
+ formatMultipleApprovals_differentValue_withCopyCondition_noUserInPredicate_differentPassingAtoms()
+ throws Exception {
+ LabelTypes labelTypes =
+ new LabelTypes(
+ ImmutableList.of(
+ createLabelType(
+ /* labelName= */ "Code-Review", /* copyCondition= */ "is:1 OR is:2")));
+ PatchSetApproval patchSetApproval1 = createPatchSetApproval(admin, "Code-Review", 2);
+ PatchSetApproval patchSetApproval2 = createPatchSetApproval(user, "Code-Review", 1);
+ ApprovalCopier.Result approvalCopierResult =
+ ApprovalCopier.Result.create(
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval1,
+ /* passingAtoms= */ ImmutableSet.of("is:2"),
+ /* failingAtoms= */ ImmutableSet.of("is:1")),
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval2,
+ /* passingAtoms= */ ImmutableSet.of("is:1"),
+ /* failingAtoms= */ ImmutableSet.of("is:2"))),
+ /* outdatedApprovals= */ ImmutableSet.of());
+ assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
+ .hasValue(
+ "Copied Votes:\n"
+ + "* Code-Review+1 (copy condition: \"**is:1** OR is:2\")\n"
+ + "* Code-Review+2 (copy condition: \"is:1 OR **is:2**\")\n");
}
@Test
@@ -545,7 +734,15 @@
PatchSetApproval patchSetApproval2 = createPatchSetApproval(user, "Code-Review", 1);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
- /* copiedApprovals= */ ImmutableSet.of(patchSetApproval1, patchSetApproval2),
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval1,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of()),
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval2,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of())),
/* outdatedApprovals= */ ImmutableSet.of());
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue(
@@ -565,7 +762,15 @@
PatchSetApproval patchSetApproval2 = createPatchSetApproval(user, "Verified", 1);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
- /* copiedApprovals= */ ImmutableSet.of(patchSetApproval1, patchSetApproval2),
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval1,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of()),
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval2,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of())),
/* outdatedApprovals= */ ImmutableSet.of());
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue(
@@ -587,7 +792,15 @@
PatchSetApproval patchSetApproval2 = createPatchSetApproval(user, "Code-Review", 1);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
- /* copiedApprovals= */ ImmutableSet.of(patchSetApproval1, patchSetApproval2),
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval1,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of()),
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval2,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of())),
/* outdatedApprovals= */ ImmutableSet.of());
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue(
@@ -597,8 +810,9 @@
}
@Test
- public void formatMultipleApprovals_sameVote_withCopyCondition_withUserInPredicate()
- throws Exception {
+ public void
+ formatMultipleApprovals_sameVote_withCopyCondition_withUserInPredicate_samePassingAtoms()
+ throws Exception {
String groupUuid =
groupCache.get(AccountGroup.nameKey("Administrators")).get().getGroupUUID().get();
LabelTypes labelTypes =
@@ -608,24 +822,86 @@
/* labelName= */ "Code-Review",
/* copyCondition= */ String.format(
"is:MIN OR (is:MAX approverin:%s)", groupUuid))));
- PatchSetApproval patchSetApproval1 = createPatchSetApproval(user, "Code-Review", 1);
- PatchSetApproval patchSetApproval2 = createPatchSetApproval(admin, "Code-Review", 1);
+ PatchSetApproval patchSetApproval1 = createPatchSetApproval(user, "Code-Review", 2);
+ PatchSetApproval patchSetApproval2 = createPatchSetApproval(admin, "Code-Review", 2);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
- /* copiedApprovals= */ ImmutableSet.of(patchSetApproval1, patchSetApproval2),
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval1,
+ /* passingAtoms= */ ImmutableSet.of(
+ "is:MAX", String.format("approverin:%s", groupUuid)),
+ /* failingAtoms= */ ImmutableSet.of("is:MIN")),
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval2,
+ /* passingAtoms= */ ImmutableSet.of(
+ "is:MAX", String.format("approverin:%s", groupUuid)),
+ /* failingAtoms= */ ImmutableSet.of("is:MIN"))),
/* outdatedApprovals= */ ImmutableSet.of());
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue(
String.format(
"Copied Votes:\n"
- + "* Code-Review+1 by %s, %s"
- + " (copy condition: \"is:MIN OR (is:MAX approverin:%s)\")\n",
+ + "* Code-Review+2 by %s, %s"
+ + " (copy condition: \"is:MIN OR (**is:MAX** **approverin:%s**)\")\n",
AccountTemplateUtil.getAccountTemplate(admin.id()),
AccountTemplateUtil.getAccountTemplate(user.id()),
groupUuid));
}
@Test
+ public void
+ formatMultipleApprovals_sameVote_withCopyCondition_withUserInPredicate_differentPassingAtoms()
+ throws Exception {
+ String administratorsGroupUuid =
+ groupCache.get(AccountGroup.nameKey("Administrators")).get().getGroupUUID().get();
+ String registeredUsersGroupUuid = SystemGroupBackend.REGISTERED_USERS.get();
+ LabelTypes labelTypes =
+ new LabelTypes(
+ ImmutableList.of(
+ createLabelType(
+ /* labelName= */ "Code-Review",
+ /* copyCondition= */ String.format(
+ "is:MIN OR (is:MAX approverin:%s) OR (is:MAX approverin:%s)",
+ administratorsGroupUuid, registeredUsersGroupUuid))));
+ PatchSetApproval patchSetApproval1 = createPatchSetApproval(user, "Code-Review", 2);
+ PatchSetApproval patchSetApproval2 = createPatchSetApproval(admin, "Code-Review", 2);
+ ApprovalCopier.Result approvalCopierResult =
+ ApprovalCopier.Result.create(
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval1,
+ /* passingAtoms= */ ImmutableSet.of(
+ "is:MAX", String.format("approverin:%s", registeredUsersGroupUuid)),
+ /* failingAtoms= */ ImmutableSet.of(
+ "is:MIN", String.format("approverin:%s", administratorsGroupUuid))),
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval2,
+ /* passingAtoms= */ ImmutableSet.of(
+ "is:MAX",
+ String.format("approverin:%s", administratorsGroupUuid),
+ String.format("approverin:%s", registeredUsersGroupUuid)),
+ /* failingAtoms= */ ImmutableSet.of("is:MIN"))),
+ /* outdatedApprovals= */ ImmutableSet.of());
+ assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
+ .hasValue(
+ String.format(
+ "Copied Votes:\n"
+ + "* Code-Review+2 by %s"
+ + " (copy condition: \"is:MIN OR (**is:MAX** **approverin:%s**)"
+ + " OR (**is:MAX** **approverin:%s**)\")\n"
+ + "* Code-Review+2 by %s"
+ + " (copy condition: \"is:MIN OR (**is:MAX** approverin:%s)"
+ + " OR (**is:MAX** **approverin:%s**)\")\n",
+ AccountTemplateUtil.getAccountTemplate(admin.id()),
+ administratorsGroupUuid,
+ registeredUsersGroupUuid,
+ AccountTemplateUtil.getAccountTemplate(user.id()),
+ administratorsGroupUuid,
+ registeredUsersGroupUuid));
+ }
+
+ @Test
public void formatMultipleApprovals_differentLabel_withCopyCondition_withUserInPredicate()
throws Exception {
String groupUuid =
@@ -641,18 +917,30 @@
/* labelName= */ "Verified",
/* copyCondition= */ String.format(
"is:MIN OR (is:MAX approverin:%s)", groupUuid))));
- PatchSetApproval patchSetApproval1 = createPatchSetApproval(user, "Code-Review", 1);
+ PatchSetApproval patchSetApproval1 = createPatchSetApproval(user, "Code-Review", -2);
PatchSetApproval patchSetApproval2 = createPatchSetApproval(admin, "Verified", 1);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
- /* copiedApprovals= */ ImmutableSet.of(patchSetApproval1, patchSetApproval2),
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval1,
+ /* passingAtoms= */ ImmutableSet.of("is:MIN"),
+ /* failingAtoms= */ ImmutableSet.of(
+ "is:MAX", String.format("approverin:%s", groupUuid))),
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval2,
+ /* passingAtoms= */ ImmutableSet.of(
+ "is:MAX", String.format("approverin:%s", groupUuid)),
+ /* failingAtoms= */ ImmutableSet.of("is:MIN"))),
/* outdatedApprovals= */ ImmutableSet.of());
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue(
String.format(
"Copied Votes:\n"
- + "* Code-Review+1 by %s (copy condition: \"is:MIN OR (is:MAX approverin:%s)\")\n"
- + "* Verified+1 by %s (copy condition: \"is:MIN OR (is:MAX approverin:%s)\")\n",
+ + "* Code-Review-2 by %s (copy condition: \"**is:MIN**"
+ + " OR (is:MAX approverin:%s)\")\n"
+ + "* Verified+1 by %s (copy condition: \"is:MIN"
+ + " OR (**is:MAX** **approverin:%s**)\")\n",
AccountTemplateUtil.getAccountTemplate(user.id()),
groupUuid,
AccountTemplateUtil.getAccountTemplate(admin.id()),
@@ -660,8 +948,9 @@
}
@Test
- public void formatMultipleApprovals_differentValue_withCopyCondition_withUserInPredicate()
- throws Exception {
+ public void
+ formatMultipleApprovals_differentValue_withCopyCondition_withUserInPredicate_samePassingAtoms()
+ throws Exception {
String groupUuid =
groupCache.get(AccountGroup.nameKey("Administrators")).get().getGroupUUID().get();
LabelTypes labelTypes =
@@ -670,51 +959,122 @@
createLabelType(
/* labelName= */ "Code-Review",
/* copyCondition= */ String.format(
- "is:MIN OR (is:MAX approverin:%s)", groupUuid))));
+ "is:MIN OR (is:ANY approverin:%s)", groupUuid))));
PatchSetApproval patchSetApproval1 = createPatchSetApproval(admin, "Code-Review", 2);
PatchSetApproval patchSetApproval2 = createPatchSetApproval(user, "Code-Review", 1);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
- /* copiedApprovals= */ ImmutableSet.of(patchSetApproval1, patchSetApproval2),
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval1,
+ /* passingAtoms= */ ImmutableSet.of(
+ "is:ANY", String.format("approverin:%s", groupUuid)),
+ /* failingAtoms= */ ImmutableSet.of("is:MIN")),
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval2,
+ /* passingAtoms= */ ImmutableSet.of(
+ "is:ANY", String.format("approverin:%s", groupUuid)),
+ /* failingAtoms= */ ImmutableSet.of("is:MIN"))),
/* outdatedApprovals= */ ImmutableSet.of());
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue(
String.format(
"Copied Votes:\n"
+ "* Code-Review+1 by %s, Code-Review+2 by %s"
- + " (copy condition: \"is:MIN OR (is:MAX approverin:%s)\")\n",
+ + " (copy condition: \"is:MIN OR (**is:ANY** **approverin:%s**)\")\n",
AccountTemplateUtil.getAccountTemplate(user.id()),
AccountTemplateUtil.getAccountTemplate(admin.id()),
groupUuid));
}
@Test
+ public void
+ formatMultipleApprovals_differentValue_withCopyCondition_withUserInPredicate_differentPassingAtoms()
+ throws Exception {
+ String groupUuid =
+ groupCache.get(AccountGroup.nameKey("Administrators")).get().getGroupUUID().get();
+ LabelTypes labelTypes =
+ new LabelTypes(
+ ImmutableList.of(
+ createLabelType(
+ /* labelName= */ "Code-Review",
+ /* copyCondition= */ String.format(
+ "is:MIN OR (is:1 approverin:%s) OR (is:2 approverin:%s)",
+ groupUuid, groupUuid))));
+ PatchSetApproval patchSetApproval1 = createPatchSetApproval(admin, "Code-Review", 2);
+ PatchSetApproval patchSetApproval2 = createPatchSetApproval(user, "Code-Review", 1);
+ ApprovalCopier.Result approvalCopierResult =
+ ApprovalCopier.Result.create(
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval1,
+ /* passingAtoms= */ ImmutableSet.of(
+ "is:2", String.format("approverin:%s", groupUuid)),
+ /* failingAtoms= */ ImmutableSet.of("is:MIN", "is:1")),
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval2,
+ /* passingAtoms= */ ImmutableSet.of(
+ "is:1", String.format("approverin:%s", groupUuid)),
+ /* failingAtoms= */ ImmutableSet.of("is:MIN", "is:2"))),
+ /* outdatedApprovals= */ ImmutableSet.of());
+ assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
+ .hasValue(
+ String.format(
+ "Copied Votes:\n"
+ + "* Code-Review+1 by %s"
+ + " (copy condition: \"is:MIN OR (**is:1** **approverin:%s**)"
+ + " OR (is:2 **approverin:%s**)\")\n"
+ + "* Code-Review+2 by %s"
+ + " (copy condition: \"is:MIN OR (is:1 **approverin:%s**)"
+ + " OR (**is:2** **approverin:%s**)\")\n",
+ AccountTemplateUtil.getAccountTemplate(user.id()),
+ groupUuid,
+ groupUuid,
+ AccountTemplateUtil.getAccountTemplate(admin.id()),
+ groupUuid,
+ groupUuid));
+ }
+
+ @Test
public void formatMultipleApprovals_differentAndSameValue_withCopyCondition_withUserInPredicate()
throws Exception {
TestAccount user2 = accountCreator.user2();
- String groupUuid =
- groupCache.get(AccountGroup.nameKey("Administrators")).get().getGroupUUID().get();
+ String groupUuid = SystemGroupBackend.REGISTERED_USERS.get();
LabelTypes labelTypes =
new LabelTypes(
ImmutableList.of(
createLabelType(
/* labelName= */ "Code-Review",
/* copyCondition= */ String.format(
- "is:MIN OR (is:MAX approverin:%s)", groupUuid))));
+ "is:MIN OR (is:ANY approverin:%s)", groupUuid))));
PatchSetApproval patchSetApproval1 = createPatchSetApproval(admin, "Code-Review", 2);
PatchSetApproval patchSetApproval2 = createPatchSetApproval(user2, "Code-Review", 1);
PatchSetApproval patchSetApproval3 = createPatchSetApproval(user, "Code-Review", 1);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
/* copiedApprovals= */ ImmutableSet.of(
- patchSetApproval1, patchSetApproval2, patchSetApproval3),
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval1,
+ /* passingAtoms= */ ImmutableSet.of(
+ "is:ANY", String.format("approverin:%s", groupUuid)),
+ /* failingAtoms= */ ImmutableSet.of("is:MIN")),
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval2,
+ /* passingAtoms= */ ImmutableSet.of(
+ "is:ANY", String.format("approverin:%s", groupUuid)),
+ /* failingAtoms= */ ImmutableSet.of("is:MIN")),
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval3,
+ /* passingAtoms= */ ImmutableSet.of(
+ "is:ANY", String.format("approverin:%s", groupUuid)),
+ /* failingAtoms= */ ImmutableSet.of("is:MIN"))),
/* outdatedApprovals= */ ImmutableSet.of());
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue(
String.format(
"Copied Votes:\n"
+ "* Code-Review+1 by %s, %s, Code-Review+2 by %s"
- + " (copy condition: \"is:MIN OR (is:MAX approverin:%s)\")\n",
+ + " (copy condition: \"is:MIN OR (**is:ANY** **approverin:%s**)\")\n",
AccountTemplateUtil.getAccountTemplate(user.id()),
AccountTemplateUtil.getAccountTemplate(user2.id()),
AccountTemplateUtil.getAccountTemplate(admin.id()),
@@ -737,7 +1097,15 @@
PatchSetApproval patchSetApproval2 = createPatchSetApproval(user, "Code-Review", 1);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
- /* copiedApprovals= */ ImmutableSet.of(patchSetApproval1, patchSetApproval2),
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval1,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of()),
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval2,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of())),
/* outdatedApprovals= */ ImmutableSet.of());
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue(
@@ -769,7 +1137,15 @@
PatchSetApproval patchSetApproval2 = createPatchSetApproval(user, "Verified", 1);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
- /* copiedApprovals= */ ImmutableSet.of(patchSetApproval1, patchSetApproval2),
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval1,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of()),
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval2,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of())),
/* outdatedApprovals= */ ImmutableSet.of());
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue(
@@ -799,7 +1175,15 @@
PatchSetApproval patchSetApproval2 = createPatchSetApproval(user, "Code-Review", 1);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
- /* copiedApprovals= */ ImmutableSet.of(patchSetApproval1, patchSetApproval2),
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval1,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of()),
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval2,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of())),
/* outdatedApprovals= */ ImmutableSet.of());
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue(
@@ -849,7 +1233,7 @@
+ "\n"
+ "Copied Votes:\n"
+ "* Code-Review-2 (copy condition: \"changekind:NO_CHANGE"
- + " OR changekind:TRIVIAL_REBASE OR is:MIN\")\n"
+ + " OR changekind:TRIVIAL_REBASE OR **is:MIN**\")\n"
+ "\n"
+ "Outdated Votes:\n"
+ "* Verified+1\n");
@@ -900,7 +1284,7 @@
+ "\n"
+ "Copied Votes:\n"
+ "* Code-Review-2 (copy condition: \"changekind:NO_CHANGE"
- + " OR changekind:TRIVIAL_REBASE OR is:MIN\")\n"
+ + " OR changekind:TRIVIAL_REBASE OR **is:MIN**\")\n"
+ "\n"
+ "Outdated Votes:\n"
+ "* Verified+1\n");
@@ -946,7 +1330,7 @@
+ "\n"
+ "Copied Votes:\n"
+ "* Code-Review-2 (copy condition: \"changekind:NO_CHANGE"
- + " OR changekind:TRIVIAL_REBASE OR is:MIN\")\n"
+ + " OR changekind:TRIVIAL_REBASE OR **is:MIN**\")\n"
+ "\n"
+ "Outdated Votes:\n"
+ "* Verified+1\n");
diff --git a/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java b/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java
index 9e7a693..bb8f3f3 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java
@@ -16,6 +16,10 @@
import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowLabel;
+import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
+import static com.google.gerrit.server.project.testing.TestLabels.labelBuilder;
+import static com.google.gerrit.server.project.testing.TestLabels.value;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import static java.util.stream.Collectors.toList;
import static org.mockito.ArgumentMatchers.any;
@@ -36,13 +40,16 @@
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.acceptance.config.GerritConfig;
+import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.AttentionSetUpdate;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.LabelId;
+import com.google.gerrit.entities.LabelType;
import com.google.gerrit.entities.PatchSet;
+import com.google.gerrit.entities.RefNames;
import com.google.gerrit.entities.SubmitRecord;
import com.google.gerrit.extensions.annotations.Exports;
import com.google.gerrit.extensions.api.changes.DraftInput;
@@ -100,6 +107,7 @@
@Inject private TestCommentHelper testCommentHelper;
@Inject private RequestScopeOperations requestScopeOperations;
@Inject private ExtensionRegistry extensionRegistry;
+ @Inject private ProjectOperations projectOperations;
private static final String COMMENT_TEXT = "The comment text";
private static final CommentForValidation FILE_COMMENT_FOR_VALIDATION =
@@ -978,6 +986,36 @@
user.fullName()));
}
+ @Test
+ public void votesInChangeMessageAreSorted() throws Exception {
+ // Create Verify label and allow voting on it.
+ try (ProjectConfigUpdate u = updateProject(project)) {
+ LabelType.Builder verified =
+ labelBuilder(
+ LabelId.VERIFIED, value(1, "Passes"), value(0, "No score"), value(-1, "Failed"));
+ u.getConfig().upsertLabelType(verified.build());
+ u.save();
+ }
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(
+ allowLabel(LabelId.VERIFIED)
+ .ref(RefNames.REFS_HEADS + "*")
+ .group(REGISTERED_USERS)
+ .range(-1, 1))
+ .update();
+
+ PushOneCommit.Result r = createChange();
+
+ ReviewInput input = new ReviewInput().label(LabelId.CODE_REVIEW, 2).label(LabelId.VERIFIED, 1);
+ gApi.changes().id(r.getChangeId()).current().review(input);
+
+ Collection<ChangeMessageInfo> messages = gApi.changes().id(r.getChangeId()).get().messages;
+ assertThat(Iterables.getLast(messages).message)
+ .isEqualTo(String.format("Patch Set 1: Code-Review+2 Verified+1"));
+ }
+
private static class TestListener implements CommentAddedListener {
public CommentAddedListener.Event lastCommentAddedEvent;
diff --git a/javatests/com/google/gerrit/acceptance/api/project/ProjectConfigIT.java b/javatests/com/google/gerrit/acceptance/api/project/ProjectConfigIT.java
index 52207db..5fd2159 100644
--- a/javatests/com/google/gerrit/acceptance/api/project/ProjectConfigIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/project/ProjectConfigIT.java
@@ -134,6 +134,27 @@
}
@Test
+ public void rejectCreatingLabelWithInvalidFunction() throws Exception {
+ fetchRefsMetaConfig();
+ PushOneCommit push =
+ pushFactory.create(
+ admin.newIdent(),
+ testRepo,
+ "Test Change",
+ ProjectConfig.PROJECT_CONFIG,
+ "[label \"Foo\"]\n function = INVALID");
+ PushOneCommit.Result r = push.to(RefNames.REFS_CONFIG);
+ r.assertErrorStatus(
+ String.format("commit %s: invalid project configuration", abbreviateName(r.getCommit())));
+ r.assertMessage(
+ String.format(
+ "ERROR: commit %s: invalid project configuration:\n"
+ + "ERROR: commit %s: project.config: Invalid function for label \"foo\"."
+ + " Valid names are: NoBlock, NoOp, PatchSetLock",
+ abbreviateName(r.getCommit()), abbreviateName(r.getCommit())));
+ }
+
+ @Test
public void rejectSettingCopyMinScore() throws Exception {
testRejectSettingLabelFlag(
LabelConfigValidator.KEY_COPY_MIN_SCORE, /* value= */ true, "is:MIN");
diff --git a/javatests/com/google/gerrit/acceptance/edit/ChangeEditIT.java b/javatests/com/google/gerrit/acceptance/edit/ChangeEditIT.java
index edfb577..cc72924 100644
--- a/javatests/com/google/gerrit/acceptance/edit/ChangeEditIT.java
+++ b/javatests/com/google/gerrit/acceptance/edit/ChangeEditIT.java
@@ -108,6 +108,7 @@
"Uploading to an edit worked!".getBytes(UTF_8);
private static final String CONTENT_BINARY_ENCODED_NEW3 =
"data:text/plain,VXBsb2FkaW5nIHRvIGFuIGVkaXQgd29ya2VkIQ==";
+ private static final String CONTENT_BINARY_ENCODED_EMPTY = "data:text/plain;base64,";
@Inject private ProjectOperations projectOperations;
@Inject private RequestScopeOperations requestScopeOperations;
@@ -697,6 +698,16 @@
}
@Test
+ public void changeEditModifyFileSetEmptyContentModeRest() throws Exception {
+ createEmptyEditFor(changeId);
+ FileContentInput in = new FileContentInput();
+ in.binary_content = CONTENT_BINARY_ENCODED_EMPTY;
+ in.fileMode = FILE_MODE;
+ adminRestSession.put(urlEditFile(changeId, FILE_NAME), in).assertNoContent();
+ ensureSameBytes(getFileContentOfEdit(changeId, FILE_NAME), "".getBytes(UTF_8));
+ }
+
+ @Test
public void createAndUploadBinaryInChangeEditOneRequestRest() throws Exception {
FileContentInput in = new FileContentInput();
in.binary_content = CONTENT_BINARY_ENCODED_NEW;
diff --git a/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java b/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
index 08f65da..b738324 100644
--- a/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
+++ b/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
@@ -1169,6 +1169,27 @@
}
@Test
+ public void pushForMasterWithForgedAuthorAndCommitter_skipAddingAuthorAndCommitterAsReviewers()
+ throws Exception {
+ setSkipAddingAuthorAndCommitterAsReviewers(InheritableBoolean.TRUE);
+ TestAccount user2 = accountCreator.user2();
+ // Create a commit with different forged author and committer.
+ RevCommit c =
+ commitBuilder()
+ .author(user.newIdent())
+ .committer(user2.newIdent())
+ .add(PushOneCommit.FILE_NAME, PushOneCommit.FILE_CONTENT)
+ .message(PushOneCommit.SUBJECT)
+ .create();
+ // Push commit as "Administrator".
+ pushHead(testRepo, "refs/for/master");
+
+ String changeId = GitUtil.getChangeId(testRepo, c).get();
+ assertThat(getOwnerEmail(changeId)).isEqualTo(admin.email());
+ assertThat(getReviewerEmails(changeId, ReviewerState.CC)).isEmpty();
+ }
+
+ @Test
public void pushForMasterWithNonExistingForgedAuthorAndCommitter() throws Exception {
// Create a commit with different forged author and committer.
RevCommit c =
@@ -3061,6 +3082,12 @@
assertThat(r.getChange().attentionSet()).isEmpty();
}
+ @Test
+ public void pushWithInvalidBaseIsRejected() throws Exception {
+ PushOneCommit.Result r = pushTo("refs/for/master%base=invalid");
+ r.assertErrorStatus("expected SHA1 for option --base: invalid");
+ }
+
private DraftInput newDraft(String path, int line, String message) {
DraftInput d = new DraftInput();
d.path = path;
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java b/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java
index 67c784b..079f84e 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java
@@ -57,8 +57,10 @@
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.api.changes.RevisionApi;
import com.google.gerrit.extensions.api.projects.BranchInput;
+import com.google.gerrit.extensions.api.projects.ConfigInput;
import com.google.gerrit.extensions.client.ChangeStatus;
import com.google.gerrit.extensions.client.GeneralPreferencesInfo;
+import com.google.gerrit.extensions.client.InheritableBoolean;
import com.google.gerrit.extensions.client.ReviewerState;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.ChangeInput;
@@ -494,6 +496,20 @@
}
@Test
+ public void createAuthorNotAddedAsCcWithAvoidAddingOriginalAuthorAsReviewer() throws Exception {
+ ConfigInput config = new ConfigInput();
+ config.skipAddingAuthorAndCommitterAsReviewers = InheritableBoolean.TRUE;
+ gApi.projects().name(project.get()).config(config);
+ ChangeInput input = newChangeInput(ChangeStatus.NEW);
+ input.author = new AccountInput();
+ input.author.email = user.email();
+ input.author.name = user.fullName();
+
+ ChangeInfo info = assertCreateSucceeds(input);
+ assertThat(info.reviewers).isEmpty();
+ }
+
+ @Test
public void createNewWorkInProgressChange() throws Exception {
ChangeInput input = newChangeInput(ChangeStatus.NEW);
input.workInProgress = true;
diff --git a/javatests/com/google/gerrit/acceptance/server/change/ApprovalCopierIT.java b/javatests/com/google/gerrit/acceptance/server/change/ApprovalCopierIT.java
index 0d06946..379a712 100644
--- a/javatests/com/google/gerrit/acceptance/server/change/ApprovalCopierIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/change/ApprovalCopierIT.java
@@ -17,18 +17,23 @@
import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.common.truth.Truth.assertAbout;
import static com.google.common.truth.Truth.assertThat;
-import static com.google.gerrit.acceptance.server.change.ApprovalCopierIT.PatchSetApprovalSubject.assertThatList;
-import static com.google.gerrit.acceptance.server.change.ApprovalCopierIT.PatchSetApprovalSubject.hasTestId;
+import static com.google.gerrit.acceptance.server.change.ApprovalCopierIT.ApprovalDataSubject.assertThat;
+import static com.google.gerrit.acceptance.server.change.ApprovalCopierIT.ApprovalDataSubject.assertThatList;
+import static com.google.gerrit.acceptance.server.change.ApprovalCopierIT.ApprovalDataSubject.hasTestId;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowLabel;
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
import static com.google.gerrit.server.project.testing.TestLabels.labelBuilder;
import static com.google.gerrit.server.project.testing.TestLabels.value;
+import static com.google.gerrit.truth.ListSubject.elements;
import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableSet;
import com.google.common.truth.Correspondence;
import com.google.common.truth.FailureMetadata;
+import com.google.common.truth.StandardSubjectBuilder;
+import com.google.common.truth.StringSubject;
import com.google.common.truth.Subject;
+import com.google.common.truth.Truth8;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.acceptance.PushOneCommit;
@@ -43,6 +48,7 @@
import com.google.gerrit.entities.PatchSetApproval;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.extensions.api.changes.ReviewInput;
+import com.google.gerrit.extensions.client.ChangeKind;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.server.approval.ApprovalCopier;
import com.google.gerrit.server.query.change.ChangeData;
@@ -50,6 +56,7 @@
import com.google.gerrit.truth.NullAwareCorrespondence;
import com.google.inject.Inject;
import java.io.IOException;
+import java.util.Optional;
import java.util.Set;
import java.util.function.Predicate;
import org.eclipse.jgit.lib.Repository;
@@ -71,6 +78,24 @@
@Before
public void setup() throws Exception {
+ // Overwrite "Code-Review" label that is inherited from All-Projects.
+ try (ProjectConfigUpdate u = updateProject(project)) {
+ LabelType.Builder codeReview =
+ labelBuilder(
+ LabelId.CODE_REVIEW,
+ value(2, "Looks good to me, approved"),
+ value(1, "Looks good to me, but someone else must approve"),
+ value(0, "No score"),
+ value(-1, "I would prefer this is not submitted as is"),
+ value(-2, "This shall not be submitted"))
+ .setCopyCondition(
+ String.format(
+ "changekind:%s OR changekind:%s OR is:MIN",
+ ChangeKind.NO_CHANGE, ChangeKind.TRIVIAL_REBASE.name()));
+ u.getConfig().upsertLabelType(codeReview.build());
+ u.save();
+ }
+
// Add Verified label.
try (ProjectConfigUpdate u = updateProject(project)) {
LabelType.Builder verified =
@@ -153,6 +178,18 @@
.containsExactly(
PatchSetApprovalTestId.create(patchSet1Id, admin.id(), LabelId.CODE_REVIEW, 2),
PatchSetApprovalTestId.create(patchSet1Id, user.id(), LabelId.VERIFIED, 1));
+
+ ApprovalDataSubject codeReviewApprovalSubject =
+ assertThat(approvalCopierResult.outdatedApprovals(), LabelId.CODE_REVIEW, admin.id());
+ codeReviewApprovalSubject.hasPassingAtomsThat().isEmpty();
+ codeReviewApprovalSubject
+ .hasFailingAtomsThat()
+ .containsExactly("changekind:NO_CHANGE", "changekind:TRIVIAL_REBASE", "is:MIN");
+
+ ApprovalDataSubject verifiedApprovalSubject =
+ assertThat(approvalCopierResult.outdatedApprovals(), LabelId.VERIFIED, user.id());
+ verifiedApprovalSubject.hasPassingAtomsThat().isEmpty();
+ verifiedApprovalSubject.hasFailingAtomsThat().containsExactly("is:MIN");
}
@Test
@@ -176,6 +213,18 @@
PatchSetApprovalTestId.create(patchSet2Id, admin.id(), LabelId.CODE_REVIEW, -2),
PatchSetApprovalTestId.create(patchSet2Id, user.id(), LabelId.VERIFIED, -1));
assertThatList(approvalCopierResult.outdatedApprovals()).isEmpty();
+
+ ApprovalDataSubject codeReviewApprovalSubject =
+ assertThat(approvalCopierResult.copiedApprovals(), LabelId.CODE_REVIEW, admin.id());
+ codeReviewApprovalSubject.hasPassingAtomsThat().containsExactly("is:MIN");
+ codeReviewApprovalSubject
+ .hasFailingAtomsThat()
+ .containsExactly("changekind:NO_CHANGE", "changekind:TRIVIAL_REBASE");
+
+ ApprovalDataSubject verifiedApprovalSubject =
+ assertThat(approvalCopierResult.copiedApprovals(), LabelId.VERIFIED, user.id());
+ verifiedApprovalSubject.hasPassingAtomsThat().containsExactly("is:MIN");
+ verifiedApprovalSubject.hasFailingAtomsThat().isEmpty();
}
@Test
@@ -230,6 +279,30 @@
.containsExactly(
PatchSetApprovalTestId.create(patchSet1Id, user.id(), LabelId.CODE_REVIEW, 1),
PatchSetApprovalTestId.create(patchSet1Id, admin.id(), LabelId.VERIFIED, 1));
+
+ ApprovalDataSubject copiedCodeReviewApprovalSubject =
+ assertThat(approvalCopierResult.copiedApprovals(), LabelId.CODE_REVIEW, admin.id());
+ copiedCodeReviewApprovalSubject.hasPassingAtomsThat().containsExactly("is:MIN");
+ copiedCodeReviewApprovalSubject
+ .hasFailingAtomsThat()
+ .containsExactly("changekind:NO_CHANGE", "changekind:TRIVIAL_REBASE");
+
+ ApprovalDataSubject copiedVerifiedApprovalSubject =
+ assertThat(approvalCopierResult.copiedApprovals(), LabelId.VERIFIED, user.id());
+ copiedVerifiedApprovalSubject.hasPassingAtomsThat().containsExactly("is:MIN");
+ copiedVerifiedApprovalSubject.hasFailingAtomsThat().isEmpty();
+
+ ApprovalDataSubject outdatedCodeReviewApprovalSubject1 =
+ assertThat(approvalCopierResult.outdatedApprovals(), LabelId.CODE_REVIEW, user.id());
+ outdatedCodeReviewApprovalSubject1.hasPassingAtomsThat().isEmpty();
+ outdatedCodeReviewApprovalSubject1
+ .hasFailingAtomsThat()
+ .containsExactly("changekind:NO_CHANGE", "changekind:TRIVIAL_REBASE", "is:MIN");
+
+ ApprovalDataSubject outdatedVerifiedApprovalSubject1 =
+ assertThat(approvalCopierResult.outdatedApprovals(), LabelId.VERIFIED, admin.id());
+ outdatedVerifiedApprovalSubject1.hasPassingAtomsThat().isEmpty();
+ outdatedVerifiedApprovalSubject1.hasFailingAtomsThat().containsExactly("is:MIN");
}
@Test
@@ -275,6 +348,11 @@
.comparingElementsUsing(hasTestId())
.containsExactly(
PatchSetApprovalTestId.create(patchSet1Id, admin.id(), LabelId.CODE_REVIEW, -2));
+
+ ApprovalDataSubject codeReviewApprovalSubject1 =
+ assertThat(approvalCopierResult.outdatedApprovals(), LabelId.CODE_REVIEW, admin.id());
+ codeReviewApprovalSubject1.hasPassingAtomsThat().isEmpty();
+ codeReviewApprovalSubject1.hasFailingAtomsThat().isEmpty();
}
@Test
@@ -347,12 +425,14 @@
ApprovalCopier.Result approvalCopierResult =
invokeApprovalCopierForCurrentPatchSet(
r.getChange().getId(), /* expectedCurrentPatchSetNum= */ 2);
- ImmutableSet<PatchSetApproval> copiedApprovals = approvalCopierResult.copiedApprovals();
- assertThatList(filter(copiedApprovals, PatchSetApproval::copied))
+ ImmutableSet<ApprovalCopier.Result.PatchSetApprovalData> copiedApprovals =
+ approvalCopierResult.copiedApprovals();
+ assertThatList(filter(copiedApprovals, approval -> approval.patchSetApproval().copied()))
.comparingElementsUsing(hasTestId())
.containsExactly(
PatchSetApprovalTestId.create(patchSet2Id, user.id(), LabelId.VERIFIED, -1));
- assertThatList(filter(copiedApprovals, psa -> !psa.copied())).isEmpty();
+ assertThatList(filter(copiedApprovals, approval -> !approval.patchSetApproval().copied()))
+ .isEmpty();
}
private void vote(String changeId, TestAccount testAccount, String label, int value)
@@ -362,8 +442,9 @@
requestScopeOperations.setApiUser(admin.id());
}
- private ImmutableSet<PatchSetApproval> filter(
- Set<PatchSetApproval> approvals, Predicate<PatchSetApproval> filter) {
+ private ImmutableSet<ApprovalCopier.Result.PatchSetApprovalData> filter(
+ Set<ApprovalCopier.Result.PatchSetApprovalData> approvals,
+ Predicate<ApprovalCopier.Result.PatchSetApprovalData> filter) {
return approvals.stream().filter(filter).collect(toImmutableSet());
}
@@ -378,20 +459,75 @@
}
}
- public static class PatchSetApprovalSubject extends Subject {
- public static Correspondence<PatchSetApproval, PatchSetApprovalTestId> hasTestId() {
- return NullAwareCorrespondence.transforming(PatchSetApprovalTestId::create, "has test ID");
+ public static class ApprovalDataSubject extends Subject {
+ public static Correspondence<ApprovalCopier.Result.PatchSetApprovalData, PatchSetApprovalTestId>
+ hasTestId() {
+ return NullAwareCorrespondence.transforming(
+ approvalData -> PatchSetApprovalTestId.create(approvalData.patchSetApproval()),
+ "has test ID");
}
+ public static ApprovalDataSubject assertThat(
+ ApprovalCopier.Result.PatchSetApprovalData approvalData) {
+ return assertAbout(approvalDatas()).that(approvalData);
+ }
+
+ public static ApprovalDataSubject assertThat(
+ ImmutableSet<ApprovalCopier.Result.PatchSetApprovalData> approvalDatas,
+ String labelId,
+ Account.Id accountId) {
+ Optional<ApprovalCopier.Result.PatchSetApprovalData> approvalDataForLabelAndAccount =
+ approvalDatas.stream()
+ .filter(
+ approvalData ->
+ approvalData.patchSetApproval().label().equals(labelId)
+ && approvalData.patchSetApproval().accountId().equals(accountId))
+ .findAny();
+ Truth8.assertThat(approvalDataForLabelAndAccount).isPresent();
+ return assertAbout(approvalDatas()).that(approvalDataForLabelAndAccount.get());
+ }
+
+ public static ListSubject<ApprovalDataSubject, ApprovalCopier.Result.PatchSetApprovalData>
+ assertThatList(ImmutableSet<ApprovalCopier.Result.PatchSetApprovalData> approvalDatas) {
+ return ListSubject.assertThat(approvalDatas.asList(), approvalDatas());
+ }
+
+ private static Factory<ApprovalDataSubject, ApprovalCopier.Result.PatchSetApprovalData>
+ approvalDatas() {
+ return ApprovalDataSubject::new;
+ }
+
+ private final ApprovalCopier.Result.PatchSetApprovalData approvalData;
+
+ private ApprovalDataSubject(
+ FailureMetadata metadata, ApprovalCopier.Result.PatchSetApprovalData approvalData) {
+ super(metadata, approvalData);
+ this.approvalData = approvalData;
+ }
+
+ public ListSubject<StringSubject, String> hasPassingAtomsThat() {
+ return check("passingAtoms()")
+ .about(elements())
+ .that(approvalData().passingAtoms().asList(), StandardSubjectBuilder::that);
+ }
+
+ public ListSubject<StringSubject, String> hasFailingAtomsThat() {
+ return check("failingAtoms()")
+ .about(elements())
+ .that(approvalData().failingAtoms().asList(), StandardSubjectBuilder::that);
+ }
+
+ private ApprovalCopier.Result.PatchSetApprovalData approvalData() {
+ isNotNull();
+ return approvalData;
+ }
+ }
+
+ public static class PatchSetApprovalSubject extends Subject {
public static PatchSetApprovalSubject assertThat(PatchSetApproval patchSetApproval) {
return assertAbout(patchSetApprovals()).that(patchSetApproval);
}
- public static ListSubject<PatchSetApprovalSubject, PatchSetApproval> assertThatList(
- ImmutableSet<PatchSetApproval> patchSetApprovals) {
- return ListSubject.assertThat(patchSetApprovals.asList(), patchSetApprovals());
- }
-
private static Factory<PatchSetApprovalSubject, PatchSetApproval> patchSetApprovals() {
return PatchSetApprovalSubject::new;
}
diff --git a/javatests/com/google/gerrit/acceptance/server/change/GetRelatedIT.java b/javatests/com/google/gerrit/acceptance/server/change/GetRelatedIT.java
index 70b5701..21db45c 100644
--- a/javatests/com/google/gerrit/acceptance/server/change/GetRelatedIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/change/GetRelatedIT.java
@@ -39,11 +39,13 @@
import com.google.gerrit.common.data.GlobalCapability;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.AccountGroup;
+import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.extensions.api.changes.GetRelatedOption;
import com.google.gerrit.extensions.api.changes.RelatedChangeAndCommitInfo;
import com.google.gerrit.extensions.api.changes.ReviewInput;
+import com.google.gerrit.extensions.api.projects.BranchInput;
import com.google.gerrit.extensions.common.CommitInfo;
import com.google.gerrit.extensions.common.EditInfo;
import com.google.gerrit.index.IndexConfig;
@@ -67,6 +69,8 @@
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.transport.PushResult;
+import org.eclipse.jgit.transport.RemoteRefUpdate;
import org.junit.Test;
@NoHttpd
@@ -664,6 +668,71 @@
}
}
+ @Test
+ public void getRelatedLinearSameCommitPushedTwice() throws Exception {
+ RevCommit base = projectOperations.project(project).getHead("master");
+
+ // 1,1---2,1 on master
+ PushOneCommit.Result r1 =
+ createChange(
+ testRepo,
+ "master",
+ "subject: 1",
+ "a.txt",
+ "1",
+ /** topic= */
+ null);
+ RevCommit c1_1 = r1.getCommit();
+ PatchSet.Id ps1_1 = r1.getPatchSetId();
+
+ PushOneCommit.Result r2 =
+ createChange(
+ testRepo,
+ "master",
+ "subject: 2",
+ "b.txt",
+ "2",
+ /** topic= */
+ null);
+ RevCommit c2_1 = r2.getCommit();
+ PatchSet.Id ps2_1 = r2.getPatchSetId();
+
+ // 3,1---4,1 on stable
+ gApi.projects().name(project.get()).branch("stable").create(new BranchInput());
+ testRepo.reset(c1_1);
+ PushResult r3 = pushHead(testRepo, "refs/for/stable%base=" + base.getName());
+ assertThat(r3.getRemoteUpdate("refs/for/stable%base=" + base.getName()).getStatus())
+ .isEqualTo(RemoteRefUpdate.Status.OK);
+ ChangeData change3 =
+ Iterables.getOnlyElement(
+ queryProvider
+ .get()
+ .byBranchCommit(BranchNameKey.create(project, "stable"), c1_1.getName()));
+ assertThat(change3.currentPatchSet().commitId()).isEqualTo(c1_1);
+ RevCommit c3_1 = c1_1;
+ PatchSet.Id ps3_1 = change3.currentPatchSet().id();
+
+ PushOneCommit.Result r4 =
+ createChange(
+ testRepo,
+ "stable",
+ "subject: 4",
+ "d.txt",
+ "4",
+ /** topic= */
+ null);
+ RevCommit c4_1 = r4.getCommit();
+ PatchSet.Id ps4_1 = r4.getPatchSetId();
+
+ for (PatchSet.Id ps : ImmutableList.of(ps2_1, ps1_1)) {
+ assertRelated(ps, changeAndCommit(ps2_1, c2_1, 1), changeAndCommit(ps1_1, c1_1, 1));
+ }
+
+ for (PatchSet.Id ps : ImmutableList.of(ps4_1, ps3_1)) {
+ assertRelated(ps, changeAndCommit(ps4_1, c4_1, 1), changeAndCommit(ps3_1, c3_1, 1));
+ }
+ }
+
private static Correspondence<RelatedChangeAndCommitInfo, String>
getRelatedChangeToStatusCorrespondence() {
return Correspondence.transforming(
diff --git a/javatests/com/google/gerrit/acceptance/server/event/CommentAddedEventIT.java b/javatests/com/google/gerrit/acceptance/server/event/CommentAddedEventIT.java
index fb3259f..f2184de 100644
--- a/javatests/com/google/gerrit/acceptance/server/event/CommentAddedEventIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/event/CommentAddedEventIT.java
@@ -213,7 +213,7 @@
@Test
@GerritConfig(name = "event.comment-added.publishPatchSetLevelComment", value = "false")
- public void publishPatchSetLevelComment() throws Exception {
+ public void publishPatchSetLevelComment_disabled() throws Exception {
PushOneCommit.Result r = createChange();
TestListener listener = new TestListener();
try (Registration registration = extensionRegistry.newRegistration().add(listener)) {
@@ -225,6 +225,20 @@
}
@Test
+ @GerritConfig(name = "event.comment-added.publishPatchSetLevelComment", value = "true")
+ public void publishPatchSetLevelComment_enabled() throws Exception {
+ PushOneCommit.Result r = createChange();
+ TestListener listener = new TestListener();
+ try (Registration registration = extensionRegistry.newRegistration().add(listener)) {
+ String patchSetLevelComment = "a patch set level comment";
+ ReviewInput reviewInput = new ReviewInput().patchSetLevelComment(patchSetLevelComment);
+ revision(r).review(reviewInput);
+ assertThat(listener.getLastCommentAddedEvent().getComment())
+ .isEqualTo(String.format("Patch Set 1:\n\n%s", patchSetLevelComment));
+ }
+ }
+
+ @Test
public void reviewChange_MultipleVotes() throws Exception {
TestListener listener = new TestListener();
try (Registration registration = extensionRegistry.newRegistration().add(listener)) {
diff --git a/javatests/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImplTest.java b/javatests/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImplTest.java
index 7543ba8..fed22f2 100644
--- a/javatests/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImplTest.java
+++ b/javatests/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImplTest.java
@@ -160,7 +160,8 @@
Project.NameKey key = projectOperations.newProject().create();
Config config = projectOperations.project(key).getConfig();
assertThat(config).isNotInstanceOf(StoredConfig.class);
- assertThat(config).text().isEmpty();
+ assertThat(config).sections().containsExactly("submit");
+ assertThat(config).sectionValues("submit").containsExactly("action", "inherit");
ConfigInput input = new ConfigInput();
input.description = "my fancy project";
@@ -168,7 +169,7 @@
config = projectOperations.project(key).getConfig();
assertThat(config).isNotInstanceOf(StoredConfig.class);
- assertThat(config).sections().containsExactly("project");
+ assertThat(config).sections().containsExactly("project", "submit");
assertThat(config).subsections("project").isEmpty();
assertThat(config).sectionValues("project").containsExactly("description", "my fancy project");
}
@@ -193,7 +194,7 @@
.update();
Config config = projectOperations.project(key).getConfig();
- assertThat(config).sections().containsExactly("access");
+ assertThat(config).sections().containsExactly("access", "submit");
assertThat(config).subsections("access").containsExactly("refs/foo");
assertThat(config)
.subsectionValues("access", "refs/foo")
@@ -210,7 +211,7 @@
.update();
Config config = projectOperations.project(key).getConfig();
- assertThat(config).sections().containsExactly("access");
+ assertThat(config).sections().containsExactly("access", "submit");
assertThat(config).subsections("access").containsExactly("refs/foo");
assertThat(config)
.subsectionValues("access", "refs/foo")
@@ -227,7 +228,7 @@
.update();
Config config = projectOperations.project(key).getConfig();
- assertThat(config).sections().containsExactly("access");
+ assertThat(config).sections().containsExactly("access", "submit");
assertThat(config).subsections("access").containsExactly("refs/foo");
assertThat(config)
.subsectionValues("access", "refs/foo")
@@ -244,7 +245,7 @@
.update();
Config config = projectOperations.project(key).getConfig();
- assertThat(config).sections().containsExactly("access");
+ assertThat(config).sections().containsExactly("access", "submit");
assertThat(config).subsections("access").containsExactly("refs/foo");
assertThat(config)
.subsectionValues("access", "refs/foo")
@@ -262,7 +263,7 @@
.update();
Config config = projectOperations.project(key).getConfig();
- assertThat(config).sections().containsExactly("access");
+ assertThat(config).sections().containsExactly("access", "submit");
assertThat(config).subsections("access").containsExactly("refs/foo");
assertThat(config)
.subsectionValues("access", "refs/foo")
@@ -277,7 +278,7 @@
.update();
config = projectOperations.project(key).getConfig();
- assertThat(config).sections().containsExactly("access");
+ assertThat(config).sections().containsExactly("access", "submit");
assertThat(config).subsections("access").containsExactly("refs/foo");
assertThat(config)
.subsectionValues("access", "refs/foo")
@@ -318,7 +319,7 @@
.update();
Config config = projectOperations.project(key).getConfig();
- assertThat(config).sections().containsExactly("access");
+ assertThat(config).sections().containsExactly("access", "submit");
assertThat(config).subsections("access").containsExactly("refs/foo");
assertThat(config)
.subsectionValues("access", "refs/foo")
@@ -335,7 +336,7 @@
projectOperations.project(key).forUpdate().add(permission).add(permission).update();
Config config = projectOperations.project(key).getConfig();
- assertThat(config).sections().containsExactly("access");
+ assertThat(config).sections().contains("access");
assertThat(config).subsections("access").containsExactly("refs/foo");
assertThat(config)
.subsectionValues("access", "refs/foo")
@@ -345,7 +346,7 @@
projectOperations.project(key).forUpdate().add(permission).update();
config = projectOperations.project(key).getConfig();
- assertThat(config).sections().containsExactly("access");
+ assertThat(config).sections().containsExactly("access", "submit");
assertThat(config).subsections("access").containsExactly("refs/foo");
assertThat(config)
.subsectionValues("access", "refs/foo")
@@ -365,7 +366,7 @@
.update();
Config config = projectOperations.project(key).getConfig();
- assertThat(config).sections().containsExactly("access");
+ assertThat(config).sections().containsExactly("access", "submit");
assertThat(config).subsections("access").containsExactly("refs/foo");
assertThat(config)
.subsectionValues("access", "refs/foo")
@@ -382,7 +383,7 @@
.update();
Config config = projectOperations.project(key).getConfig();
- assertThat(config).sections().containsExactly("access");
+ assertThat(config).sections().containsExactly("access", "submit");
assertThat(config).subsections("access").containsExactly("refs/foo");
assertThat(config)
.subsectionValues("access", "refs/foo")
@@ -400,7 +401,7 @@
.update();
Config config = projectOperations.project(key).getConfig();
- assertThat(config).sections().containsExactly("access");
+ assertThat(config).sections().containsExactly("access", "submit");
assertThat(config).subsections("access").containsExactly("refs/foo");
assertThat(config)
.subsectionValues("access", "refs/foo")
@@ -415,7 +416,7 @@
.update();
config = projectOperations.project(key).getConfig();
- assertThat(config).sections().containsExactly("access");
+ assertThat(config).sections().containsExactly("access", "submit");
assertThat(config).subsections("access").containsExactly("refs/foo");
assertThat(config)
.subsectionValues("access", "refs/foo")
@@ -437,7 +438,7 @@
.update();
Config config = projectOperations.project(key).getConfig();
- assertThat(config).sections().containsExactly("access");
+ assertThat(config).sections().containsExactly("access", "submit");
assertThat(config).subsections("access").containsExactly("refs/foo");
assertThat(config)
.subsectionValues("access", "refs/foo")
diff --git a/javatests/com/google/gerrit/server/cache/serialize/entities/ProjectSerializerTest.java b/javatests/com/google/gerrit/server/cache/serialize/entities/ProjectSerializerTest.java
index 29fd5ed..1f725f8 100644
--- a/javatests/com/google/gerrit/server/cache/serialize/entities/ProjectSerializerTest.java
+++ b/javatests/com/google/gerrit/server/cache/serialize/entities/ProjectSerializerTest.java
@@ -40,6 +40,9 @@
.setBooleanConfig(
BooleanProjectConfig.CREATE_NEW_CHANGE_FOR_ALL_NOT_IN_TARGET,
InheritableBoolean.INHERIT)
+ .setBooleanConfig(
+ BooleanProjectConfig.SKIP_ADDING_AUTHOR_AND_COMMITTER_AS_REVIEWERS,
+ InheritableBoolean.TRUE)
.build();
@Test
diff --git a/javatests/com/google/gerrit/server/index/change/ChangeFieldTest.java b/javatests/com/google/gerrit/server/index/change/ChangeFieldTest.java
index 0bdf5cd..35077db 100644
--- a/javatests/com/google/gerrit/server/index/change/ChangeFieldTest.java
+++ b/javatests/com/google/gerrit/server/index/change/ChangeFieldTest.java
@@ -158,14 +158,15 @@
public void tolerateNullValuesForInsertion() {
Project.NameKey project = Project.nameKey("project");
ChangeData cd = ChangeData.createForTest(project, Change.id(1), 1, ObjectId.zeroId());
- assertThat(ChangeField.ADDED.setIfPossible(cd, new FakeStoredValue(null))).isTrue();
+ assertThat(ChangeField.ADDED_LINES_SPEC.setIfPossible(cd, new FakeStoredValue(null))).isTrue();
}
@Test
public void tolerateNullValuesForDeletion() {
Project.NameKey project = Project.nameKey("project");
ChangeData cd = ChangeData.createForTest(project, Change.id(1), 1, ObjectId.zeroId());
- assertThat(ChangeField.DELETED.setIfPossible(cd, new FakeStoredValue(null))).isTrue();
+ assertThat(ChangeField.DELETED_LINES_SPEC.setIfPossible(cd, new FakeStoredValue(null)))
+ .isTrue();
}
@Test
diff --git a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
index 28149d8..d7ec030 100644
--- a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
+++ b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
@@ -2755,7 +2755,7 @@
@Test
public void cherrypick() throws Exception {
- assume().that(getSchema().hasField(ChangeField.CHERRY_PICK)).isTrue();
+ assume().that(getSchema().hasField(ChangeField.CHERRY_PICK_SPEC)).isTrue();
TestRepository<Repo> repo = createProject("repo");
Change change1 = insert(repo, newChange(repo));
Change change2 = insert(repo, newCherryPickChange(repo, "foo", change1.currentPatchSetId()));
@@ -2766,7 +2766,7 @@
@Test
public void merge() throws Exception {
- assume().that(getSchema().hasField(ChangeField.MERGE)).isTrue();
+ assume().that(getSchema().hasField(ChangeField.MERGE_SPEC)).isTrue();
TestRepository<Repo> repo = createProject("repo");
RevCommit commit1 = repo.parseBody(repo.commit().add("file1", "contents1").create());
RevCommit commit2 = repo.parseBody(repo.commit().add("file1", "contents2").create());
diff --git a/polygerrit-ui/app/elements/admin/gr-admin-view/gr-admin-view.ts b/polygerrit-ui/app/elements/admin/gr-admin-view/gr-admin-view.ts
index 1c62114..af4e87e 100644
--- a/polygerrit-ui/app/elements/admin/gr-admin-view/gr-admin-view.ts
+++ b/polygerrit-ui/app/elements/admin/gr-admin-view/gr-admin-view.ts
@@ -418,7 +418,10 @@
return html`
<div class="main breadcrumbs">
- <gr-repo-commands .repo=${this.repoViewState.repo}></gr-repo-commands>
+ <gr-repo-commands
+ .repo=${this.repoViewState.repo}
+ .createEdit=${this.repoViewState.createEdit}
+ ></gr-repo-commands>
</div>
`;
}
diff --git a/polygerrit-ui/app/elements/admin/gr-create-change-dialog/gr-create-file-edit-dialog.ts b/polygerrit-ui/app/elements/admin/gr-create-change-dialog/gr-create-file-edit-dialog.ts
new file mode 100644
index 0000000..c3f79c7
--- /dev/null
+++ b/polygerrit-ui/app/elements/admin/gr-create-change-dialog/gr-create-file-edit-dialog.ts
@@ -0,0 +1,170 @@
+/**
+ * @license
+ * Copyright 2022 Google LLC
+ * SPDX-License-Identifier: Apache-2.0
+ */
+import {navigationToken} from '../../core/gr-navigation/gr-navigation';
+import {
+ RepoName,
+ BranchName,
+ ChangeInfo,
+ PatchSetNumber,
+} from '../../../types/common';
+import {getAppContext} from '../../../services/app-context';
+import {LitElement, html, nothing} from 'lit';
+import {customElement, property, query, state} from 'lit/decorators.js';
+import {resolve} from '../../../models/dependency';
+import {createEditUrl} from '../../../models/views/edit';
+import {modalStyles} from '../../../styles/gr-modal-styles';
+import {assertIsDefined} from '../../../utils/common-util';
+import {when} from 'lit/directives/when.js';
+import {GrDialog} from '../../shared/gr-dialog/gr-dialog';
+
+declare global {
+ interface HTMLElementTagNameMap {
+ 'gr-create-file-edit-dialog': GrCreateFileEditDialog;
+ }
+}
+
+@customElement('gr-create-file-edit-dialog')
+export class GrCreateFileEditDialog extends LitElement {
+ @query('dialog')
+ modal?: HTMLDialogElement;
+
+ @query('gr-dialog')
+ grDialog?: GrDialog;
+
+ @property({type: String})
+ repo?: RepoName;
+
+ @property({type: String})
+ branch?: BranchName;
+
+ @property({type: String})
+ path?: string;
+
+ /**
+ * If this is set, then we show this message replacing all other content.
+ */
+ @state()
+ errorMessage?: string;
+
+ /**
+ * Triggers showing the dialog and kicks off creating a change.
+ */
+ @state()
+ active = false;
+
+ /**
+ * Indicates whether the REST API call for creating a change is in progress.
+ */
+ @state()
+ loading = false;
+
+ private readonly restApiService = getAppContext().restApiService;
+
+ private readonly getNavigation = resolve(this, navigationToken);
+
+ static override get styles() {
+ return [modalStyles];
+ }
+
+ override render() {
+ if (!this.active) return nothing;
+ return html`
+ <dialog tabindex="-1">
+ <gr-dialog
+ disabled
+ ?loading=${this.loading}
+ .loadingLabel=${'Creating change ...'}
+ @cancel=${() => this.deactivate()}
+ .confirmLabel=${this.loading ? 'Please wait ...' : 'Failed'}
+ .cancelLabel=${'Cancel'}
+ >
+ <div slot="header">
+ <span class="main-heading">Create Change from URL</span>
+ </div>
+ <div slot="main">
+ ${when(
+ this.errorMessage,
+ () => this.renderError(),
+ () => this.renderCreating()
+ )}
+ </div>
+ </gr-dialog>
+ </dialog>
+ `;
+ }
+
+ async activate() {
+ this.active = true;
+ this.createChange();
+ await this.updateComplete;
+ if (this.active && this.modal?.open === false) this.modal.showModal();
+ }
+
+ deactivate() {
+ this.active = false;
+ this.modal?.close();
+ }
+
+ private renderCreating() {
+ return html`
+ <div>
+ <span>
+ Creating a change in repository <b>${this.repo}</b> on branch
+ <b>${this.branch}</b>.
+ </span>
+ </div>
+ <div>
+ <span>
+ The page will then redirect to the file editor for
+ <b>${this.path}</b>
+ in the newly created change.
+ </span>
+ </div>
+ `;
+ }
+
+ private renderError() {
+ return html`<div>Error: ${this.errorMessage}</div>`;
+ }
+
+ private createChange() {
+ if (!this.repo || !this.branch || !this.path) {
+ this.errorMessage = 'repo, branch and path must be set';
+ return;
+ }
+ if (this.loading || this.errorMessage) return;
+ this.loading = true;
+ this.restApiService
+ .createChange(this.repo, this.branch, `Edit ${this.path}`)
+ .then(change => {
+ if (!this.active) return;
+ if (change) {
+ this.loading = false;
+ this.redirectToFileEdit(change);
+ this.deactivate();
+ } else {
+ this.errorMessage = 'Creating the change failed.';
+ }
+ })
+ .catch(() => {
+ this.errorMessage = 'Creating the change failed.';
+ })
+ .finally(() => {
+ this.loading = false;
+ });
+ }
+
+ private redirectToFileEdit(change: ChangeInfo) {
+ assertIsDefined(this.path, 'path');
+ const url = createEditUrl({
+ changeNum: change._number,
+ repo: change.project,
+ path: this.path,
+ patchNum: 1 as PatchSetNumber,
+ });
+ this.getNavigation().setUrl(url);
+ }
+}
diff --git a/polygerrit-ui/app/elements/admin/gr-create-change-dialog/gr-create-file-edit-dialog_test.ts b/polygerrit-ui/app/elements/admin/gr-create-change-dialog/gr-create-file-edit-dialog_test.ts
new file mode 100644
index 0000000..7621fac
--- /dev/null
+++ b/polygerrit-ui/app/elements/admin/gr-create-change-dialog/gr-create-file-edit-dialog_test.ts
@@ -0,0 +1,99 @@
+/**
+ * @license
+ * Copyright 2022 Google LLC
+ * SPDX-License-Identifier: Apache-2.0
+ */
+import '../../../test/common-test-setup';
+import './gr-create-file-edit-dialog';
+import {createChange} from '../../../test/test-data-generators';
+import {fixture, html, assert} from '@open-wc/testing';
+import {GrCreateFileEditDialog} from './gr-create-file-edit-dialog';
+import {stubRestApi, waitUntilCalled} from '../../../test/test-utils';
+import {BranchName, RepoName} from '../../../api/rest-api';
+import {SinonStub} from 'sinon';
+import {testResolver} from '../../../test/common-test-setup';
+import {navigationToken} from '../../core/gr-navigation/gr-navigation';
+
+suite('gr-create-file-edit-dialog', () => {
+ let element: GrCreateFileEditDialog;
+ let createChangeStub: SinonStub;
+ let setUrlStub: SinonStub;
+
+ setup(async () => {
+ createChangeStub = stubRestApi('createChange').resolves(createChange());
+ setUrlStub = sinon.stub(testResolver(navigationToken), 'setUrl');
+
+ element = await fixture(
+ html`<gr-create-file-edit-dialog></gr-create-file-edit-dialog>`
+ );
+ element.repo = 'test-repo' as RepoName;
+ element.branch = 'test-branch' as BranchName;
+ element.path = 'test-path';
+ await element.updateComplete;
+ });
+
+ test('render', async () => {
+ element.activate();
+ await element.updateComplete;
+ assert.shadowDom.equal(
+ element,
+ /* HTML */ `
+ <dialog tabindex="-1">
+ <gr-dialog disabled loading>
+ <div slot="header">
+ <span class="main-heading"> Create Change from URL </span>
+ </div>
+ <div slot="main">
+ <div>
+ <span>
+ Creating a change in repository
+ <b> test-repo </b>
+ on branch
+ <b> test-branch </b>
+ .
+ </span>
+ </div>
+ <div>
+ <span>
+ The page will then redirect to the file editor for
+ <b> test-path </b> in the newly created change.
+ </span>
+ </div>
+ </div>
+ </gr-dialog>
+ </dialog>
+ `
+ );
+ });
+
+ test('render error', async () => {
+ element.activate();
+ element.errorMessage = 'Failed.';
+ await element.updateComplete;
+ assert.shadowDom.equal(
+ element,
+ /* HTML */ `
+ <dialog tabindex="-1">
+ <gr-dialog disabled loading>
+ <div slot="header">
+ <span class="main-heading"> Create Change from URL </span>
+ </div>
+ <div slot="main">
+ <div>Error: Failed.</div>
+ </div>
+ </gr-dialog>
+ </dialog>
+ `
+ );
+ });
+
+ test('creates change', async () => {
+ element.activate();
+ await element.updateComplete;
+
+ assert.isTrue(createChangeStub.calledOnce);
+ await waitUntilCalled(setUrlStub, 'setUrl');
+ await element.updateComplete;
+ assert.shadowDom.equal(element, '');
+ });
+});
diff --git a/polygerrit-ui/app/elements/admin/gr-repo-commands/gr-repo-commands.ts b/polygerrit-ui/app/elements/admin/gr-repo-commands/gr-repo-commands.ts
index bb1e926..e13609e 100644
--- a/polygerrit-ui/app/elements/admin/gr-repo-commands/gr-repo-commands.ts
+++ b/polygerrit-ui/app/elements/admin/gr-repo-commands/gr-repo-commands.ts
@@ -9,6 +9,7 @@
import '../../shared/gr-dialog/gr-dialog';
import '../../shared/gr-overlay/gr-overlay';
import '../gr-create-change-dialog/gr-create-change-dialog';
+import '../gr-create-change-dialog/gr-create-file-edit-dialog';
import {navigationToken} from '../../core/gr-navigation/gr-navigation';
import {
BranchName,
@@ -34,6 +35,7 @@
import {createEditUrl} from '../../../models/views/edit';
import {resolve} from '../../../models/dependency';
import {modalStyles} from '../../../styles/gr-modal-styles';
+import {GrCreateFileEditDialog} from '../gr-create-change-dialog/gr-create-file-edit-dialog';
const GC_MESSAGE = 'Garbage collection completed successfully.';
const CONFIG_BRANCH = 'refs/meta/config' as BranchName;
@@ -57,9 +59,18 @@
@query('#createNewChangeModal')
private readonly createNewChangeModal?: GrCreateChangeDialog;
+ @query('#createFileEditDialog')
+ private readonly createFileEditDialog?: GrCreateFileEditDialog;
+
@property({type: String})
repo?: RepoName;
+ @property({type: Object})
+ createEdit?: {
+ branch: BranchName;
+ path: string;
+ };
+
@state() private loading = true;
@state() private repoConfig?: ConfigInfo;
@@ -76,6 +87,9 @@
private readonly getNavigation = resolve(this, navigationToken);
+ /** Make sure that this dialog is only activated once. */
+ private createFileEditDialogWasActivated = false;
+
override connectedCallback() {
super.connectedCallback();
fireTitleChange(this, 'Repo Commands');
@@ -181,6 +195,12 @@
</div>
</gr-dialog>
</dialog>
+ <gr-create-file-edit-dialog
+ id="createFileEditDialog"
+ .repo=${this.repo}
+ .branch=${this.createEdit?.branch}
+ .path=${this.createEdit?.path}
+ ></gr-create-file-edit-dialog>
`;
}
@@ -200,6 +220,15 @@
`;
}
+ override updated(changedProperties: PropertyValues) {
+ if (changedProperties.has('createEdit')) {
+ if (!this.createFileEditDialogWasActivated) {
+ this.createFileEditDialog?.activate();
+ this.createFileEditDialogWasActivated = true;
+ }
+ }
+ }
+
override willUpdate(changedProperties: PropertyValues) {
if (changedProperties.has('repo')) {
this.loadRepo();
diff --git a/polygerrit-ui/app/elements/admin/gr-repo-commands/gr-repo-commands_test.ts b/polygerrit-ui/app/elements/admin/gr-repo-commands/gr-repo-commands_test.ts
index 01140c6..dab2706 100644
--- a/polygerrit-ui/app/elements/admin/gr-repo-commands/gr-repo-commands_test.ts
+++ b/polygerrit-ui/app/elements/admin/gr-repo-commands/gr-repo-commands_test.ts
@@ -94,6 +94,8 @@
</div>
</gr-dialog>
</dialog>
+ <gr-create-file-edit-dialog id="createFileEditDialog">
+ </gr-create-file-edit-dialog>
`,
{ignoreTags: ['p']}
);
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view.ts b/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view.ts
index cd5cb96..7abfce9 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view.ts
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view.ts
@@ -294,7 +294,13 @@
// private but used in test
computePage() {
if (this.offset === undefined || this.changesPerPage === undefined) return;
- return this.offset / this.changesPerPage + 1;
+ // We use Math.ceil in case the offset is not divisible by changesPerPage.
+ // If we did not do this, you'd have page '1.2' and then when pressing left
+ // arrow 'Page 1'. This way page '1.2' becomes page '2'.
+ return (
+ Math.ceil(this.offset / this.limitFor(this.query, this.changesPerPage)) +
+ 1
+ );
}
private async handleToggleStar(e: CustomEvent<ChangeStarToggleStarDetail>) {
diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
index 329102a..adc3bd7 100644
--- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
@@ -1570,13 +1570,10 @@
handleRebaseConfirm(e: CustomEvent<ConfirmRebaseEventDetail>) {
assertIsDefined(this.confirmRebase, 'confirmRebase');
assertIsDefined(this.actionsModal, 'actionsModal');
- const el = this.confirmRebase;
const payload = {
base: e.detail.base,
allow_conflicts: e.detail.allowConflicts,
};
- this.actionsModal.close();
- el.hidden = true;
this.fireAction(
'/rebase',
assertUIActionInfo(this.revisionActions.rebase),
@@ -1841,6 +1838,7 @@
if (!response) {
return;
}
+ // response is guaranteed to be ok (due to semantics of rest-api methods)
return this.restApiService.getResponseObject(response).then(obj => {
switch (action.__key) {
case ChangeActions.REVERT: {
@@ -1874,6 +1872,9 @@
case ChangeActions.REBASE_EDIT:
case ChangeActions.REBASE:
case ChangeActions.SUBMIT:
+ // Hide rebase dialog only if the action succeeds
+ this.actionsModal?.close();
+ this.hideAllDialogs();
fireReload(this, true);
break;
case ChangeActions.REVERT_SUBMISSION: {
diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts
index 1d10128..a1021a0 100644
--- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts
@@ -1422,6 +1422,39 @@
await element.reload();
});
+ test('revert change payload', async () => {
+ await element.updateComplete;
+ queryAndAssert<GrButton>(
+ element,
+ 'gr-button[data-action-key="revert"]'
+ ).click();
+ const revertAction = {
+ __key: 'revert',
+ __type: 'change',
+ __primary: false,
+ method: HttpMethod.POST,
+ label: 'Revert',
+ title: 'Revert the change',
+ enabled: true,
+ };
+ queryAndAssert(element, 'gr-confirm-revert-dialog').dispatchEvent(
+ new CustomEvent('confirm', {
+ detail: {
+ message: 'foo message',
+ revertType: 1,
+ },
+ })
+ );
+ assert.deepEqual(fireActionStub.lastCall.args, [
+ '/revert',
+ assertUIActionInfo(revertAction),
+ false,
+ {
+ message: 'foo message',
+ },
+ ]);
+ });
+
test('revert change with plugin hook', async () => {
const newRevertMsg = 'Modified revert msg';
sinon
diff --git a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.ts b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.ts
index 6a74dce..f32bb8d 100644
--- a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.ts
@@ -279,7 +279,7 @@
${this.renderNonOwner(ChangeRole.AUTHOR)}
${this.renderNonOwner(ChangeRole.COMMITTER)} ${this.renderReviewers()}
${this.renderCCs()} ${this.renderProjectBranch()} ${this.renderParent()}
- ${this.renderMergedAs()} ${this.renderShowReverCreatedAs()}
+ ${this.renderMergedAs()} ${this.renderShowRevertCreatedAs()}
${this.renderTopic()} ${this.renderCherryPickOf()}
${this.renderStrategy()} ${this.renderHashTags()}
${this.renderSubmitRequirements()} ${this.renderWeblinks()}
@@ -561,7 +561,7 @@
</section>`;
}
- private renderShowReverCreatedAs() {
+ private renderShowRevertCreatedAs() {
if (!this.showRevertCreatedAs()) return nothing;
return html`<section
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
index c4c37fa..7ce0c65 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
@@ -876,6 +876,11 @@
sharedStyles,
modalStyles,
css`
+ .header,
+ #fileListHeader {
+ position: sticky;
+ top: 0; /* Necessary to ensure it sticks to the top. */
+ }
.container:not(.loading) {
background-color: var(--background-color-tertiary);
}
@@ -923,6 +928,7 @@
}
.showCopyLinkDialogButton {
--gr-button-padding: 0 0 0 var(--spacing-s);
+ --background-color: transparent;
margin-left: var(--spacing-s);
}
#replyBtn {
@@ -1236,14 +1242,20 @@
`;
}
- private renderChangeInfoSection() {
- return html`<section class="changeInfoSection">
+ private renderHeader() {
+ return html`
<div class=${this.computeHeaderClass()}>
<h1 class="assistive-tech-only">
Change ${this.change?._number}: ${this.change?.subject}
</h1>
${this.renderHeaderTitle()} ${this.renderCommitActions()}
</div>
+ `;
+ }
+
+ private renderChangeInfoSection() {
+ return html`<section class="changeInfoSection">
+ ${this.renderHeader()}
<h2 class="assistive-tech-only">Change metadata</h2>
${this.renderChangeInfo()}
</section>`;
diff --git a/polygerrit-ui/app/elements/change/gr-confirm-submit-dialog/gr-confirm-submit-dialog.ts b/polygerrit-ui/app/elements/change/gr-confirm-submit-dialog/gr-confirm-submit-dialog.ts
index cf66ecf..1c48a42 100644
--- a/polygerrit-ui/app/elements/change/gr-confirm-submit-dialog/gr-confirm-submit-dialog.ts
+++ b/polygerrit-ui/app/elements/change/gr-confirm-submit-dialog/gr-confirm-submit-dialog.ts
@@ -127,7 +127,7 @@
return html`
<gr-icon icon="warning" filled class="warningBeforeSubmit"></gr-icon>
Your unpublished edit will not be submitted. Did you forget to click
- <b>PUBLISH</b>
+ <b>PUBLISH</b> after pressing <b>EDIT</b>?
`;
}
diff --git a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.ts b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.ts
index 9bac7c2..b114831 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.ts
+++ b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.ts
@@ -149,6 +149,9 @@
static override styles = [
sharedStyles,
css`
+ :host {
+ display: block;
+ }
.prefsButton {
float: right;
}
@@ -159,6 +162,9 @@
align-items: center;
display: flex;
padding: var(--spacing-s) var(--spacing-l);
+ /* Necessary to avoid that the header is transparent and shows the files */
+ background-color: var(--background-color-primary);
+ border-bottom: 1px solid var(--border-color);
}
.patchInfo-left {
align-items: baseline;
@@ -268,10 +274,6 @@
this.allPatchSets
);
const expandedClass = this.computeExpandedClass(this.filesExpanded);
- const prefsButtonHidden = this.computePrefsButtonHidden(
- this.diffPrefs,
- this.loggedIn
- );
return html`
<div class="patchInfo-header ${editModeClass} ${patchInfoClass}">
<div class="patchInfo-left">
@@ -309,28 +311,29 @@
</span>
`
)}
- <div class="fileViewActions">
- <span class="fileViewActionsLabel">Diff view:</span>
- <gr-diff-mode-selector
- id="modeSelect"
- .saveOnChange=${this.loggedIn ?? false}
- ></gr-diff-mode-selector>
- <span
- id="diffPrefsContainer"
- class="hideOnEdit"
- ?hidden=${prefsButtonHidden}
- >
- <gr-tooltip-content has-tooltip title="Diff preferences">
- <gr-button
- link
- class="prefsButton desktop"
- @click=${this.handlePrefsTap}
- ><gr-icon icon="settings" filled></gr-icon
- ></gr-button>
- </gr-tooltip-content>
- </span>
- <span class="separator"></span>
- </div>
+ ${when(
+ this.loggedIn && this.diffPrefs,
+ () => html`
+ <div class="fileViewActions">
+ <span class="fileViewActionsLabel">Diff view:</span>
+ <gr-diff-mode-selector
+ id="modeSelect"
+ .saveOnChange=${true}
+ ></gr-diff-mode-selector>
+ <span id="diffPrefsContainer" class="hideOnEdit">
+ <gr-tooltip-content has-tooltip title="Diff preferences">
+ <gr-button
+ link
+ class="prefsButton desktop"
+ @click=${this.handlePrefsTap}
+ ><gr-icon icon="settings" filled></gr-icon
+ ></gr-button>
+ </gr-tooltip-content>
+ </span>
+ <span class="separator"></span>
+ </div>
+ `
+ )}
<span class="downloadContainer desktop">
<gr-tooltip-content
has-tooltip
@@ -402,13 +405,6 @@
return classes.join(' ');
}
- private computePrefsButtonHidden(
- prefs: DiffPreferencesInfo,
- loggedIn?: boolean
- ) {
- return !loggedIn || !prefs;
- }
-
private fileListActionsVisible(
shownFileCount: number,
maxFilesForBulkActions: number
diff --git a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header_test.ts b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header_test.ts
index 48cef64..23534a0 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header_test.ts
@@ -53,6 +53,7 @@
.shownFileCount=${3}
></gr-file-list-header>`
);
+ element.loggedIn = true;
element.diffPrefs = createDefaultDiffPrefs();
await element.updateComplete;
});
@@ -77,7 +78,7 @@
<div class="fileViewActions">
<span class="fileViewActionsLabel"> Diff view: </span>
<gr-diff-mode-selector id="modeSelect"> </gr-diff-mode-selector>
- <span class="hideOnEdit" hidden="" id="diffPrefsContainer">
+ <span class="hideOnEdit" id="diffPrefsContainer">
<gr-tooltip-content has-tooltip="" title="Diff preferences">
<gr-button
aria-disabled="false"
@@ -110,7 +111,7 @@
</span>
<gr-tooltip-content
has-tooltip=""
- title="Show/hide all inline diffs (shortcut: I)"
+ title="Show/hide all inline diffs (shortcut: Shift+i)"
>
<gr-button
aria-disabled="false"
@@ -124,7 +125,7 @@
</gr-tooltip-content>
<gr-tooltip-content
has-tooltip=""
- title="Show/hide all inline diffs (shortcut: I)"
+ title="Show/hide all inline diffs (shortcut: Shift+i)"
>
<gr-button
aria-disabled="false"
@@ -143,17 +144,13 @@
});
test('Diff preferences hidden when no prefs', async () => {
- assert.isTrue(
- queryAndAssert<HTMLElement>(element, '#diffPrefsContainer').hidden
- );
+ assert.isOk(query<HTMLElement>(element, '#diffPrefsContainer'));
- element.diffPrefs = createDefaultDiffPrefs();
+ element.diffPrefs = undefined;
element.loggedIn = true;
await element.updateComplete;
- assert.isFalse(
- queryAndAssert<HTMLElement>(element, '#diffPrefsContainer').hidden
- );
+ assert.isNotOk(query<HTMLElement>(element, '#diffPrefsContainer'));
});
test('expandAllDiffs called when expand button clicked', async () => {
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts
index 00d5a8c..2dcfd8c 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts
@@ -337,6 +337,9 @@
min-height: calc(var(--line-height-normal) + 2 * var(--spacing-s));
padding: var(--spacing-xs) var(--spacing-l);
}
+ .header-row.row {
+ border-top: none;
+ }
/* The class defines a content visible only to screen readers */
.noCommentsScreenReaderText {
opacity: 0;
diff --git a/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-change.ts b/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-change.ts
index 69fd142..a2beee2 100644
--- a/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-change.ts
+++ b/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-change.ts
@@ -99,7 +99,7 @@
override render() {
const change = this.change;
if (!change) throw new Error('Missing change');
- const linkClass = this._computeLinkClass(change);
+ const linkClass = this.computeLinkClass(change);
return html`
<div class="changeContainer">
<a
@@ -118,16 +118,16 @@
>✓</span
>`
: ''}
- ${this.showChangeStatus && !isChangeInfo(change)
- ? html`<span class=${this._computeChangeStatusClass(change)}>
- (${this._computeChangeStatus(change)})
+ ${this.showChangeStatus
+ ? html`<span class=${this.computeChangeStatusClass(change)}>
+ (${this.computeChangeStatus(change)})
</span>`
: ''}
</div>
`;
}
- _computeLinkClass(change: ChangeInfo | RelatedChangeAndCommitInfo) {
+ private computeLinkClass(change: ChangeInfo | RelatedChangeAndCommitInfo) {
const statuses = [];
if (change.status === ChangeStatus.ABANDONED) {
statuses.push('strikethrough');
@@ -138,11 +138,16 @@
return statuses.join(' ');
}
- _computeChangeStatusClass(change: RelatedChangeAndCommitInfo) {
+ private computeChangeStatusClass(
+ change: RelatedChangeAndCommitInfo | ChangeInfo
+ ) {
const classes = ['status'];
- if (change._revision_number !== change._current_revision_number) {
+ if (
+ !isChangeInfo(change) &&
+ change._revision_number !== change._current_revision_number
+ ) {
classes.push('notCurrent');
- } else if (this._isIndirectAncestor(change)) {
+ } else if (!isChangeInfo(change) && this.isIndirectAncestor(change)) {
classes.push('indirectAncestor');
} else if (change.submittable) {
classes.push('submittable');
@@ -152,16 +157,19 @@
return classes.join(' ');
}
- _computeChangeStatus(change: RelatedChangeAndCommitInfo) {
+ private computeChangeStatus(change: RelatedChangeAndCommitInfo | ChangeInfo) {
switch (change.status) {
case ChangeStatus.MERGED:
return 'Merged';
case ChangeStatus.ABANDONED:
return 'Abandoned';
}
- if (change._revision_number !== change._current_revision_number) {
+ if (
+ !isChangeInfo(change) &&
+ change._revision_number !== change._current_revision_number
+ ) {
return 'Not current';
- } else if (this._isIndirectAncestor(change)) {
+ } else if (!isChangeInfo(change) && this.isIndirectAncestor(change)) {
return 'Indirect ancestor';
} else if (change.submittable) {
return 'Submittable';
@@ -169,7 +177,7 @@
return '';
}
- _isIndirectAncestor(change: RelatedChangeAndCommitInfo) {
+ private isIndirectAncestor(change: RelatedChangeAndCommitInfo) {
return (
this.connectedRevisions &&
!this.connectedRevisions.includes(change.commit.commit)
diff --git a/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.ts b/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.ts
index bdb95ea..cac9ae5 100644
--- a/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.ts
+++ b/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.ts
@@ -418,6 +418,7 @@
)}<gr-related-change
.change=${change}
.href=${createChangeUrl({change, usp: 'cherry-pick'})}
+ show-change-status
>${change.branch}: ${change.subject}</gr-related-change
>
</div>`
diff --git a/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list_test.ts b/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list_test.ts
index 676a468..3e90145 100644
--- a/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list_test.ts
@@ -249,7 +249,7 @@
<gr-related-collapse title="Cherry picks">
<div class="relatedChangeLine show-when-collapsed">
<span class="marker space"> </span>
- <gr-related-change>
+ <gr-related-change show-change-status="">
test-branch: Test subject
</gr-related-change>
</div>
diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
index 29bd1d9..933d06f 100644
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
@@ -75,6 +75,7 @@
} from '../../../utils/common-util';
import {
CommentThread,
+ createPatchsetLevelUnsavedDraft,
DraftInfo,
getFirstComment,
isDraft,
@@ -111,7 +112,7 @@
LabelNameToValuesMap,
PatchSetNumber,
} from '../../../api/rest-api';
-import {css, html, PropertyValues, LitElement} from 'lit';
+import {css, html, PropertyValues, LitElement, nothing} from 'lit';
import {sharedStyles} from '../../../styles/shared-styles';
import {when} from 'lit/directives/when.js';
import {classMap} from 'lit/directives/class-map.js';
@@ -606,6 +607,16 @@
.patchsetLevelContainer.unresolved {
background-color: var(--unresolved-comment-background-color);
}
+ .privateVisiblityInfo {
+ display: flex;
+ justify-content: center;
+ background-color: var(--info-background);
+ padding: var(--spacing-s) 0;
+ }
+ .privateVisiblityInfo gr-icon {
+ margin-right: var(--spacing-m);
+ color: var(--info-foreground);
+ }
`,
];
@@ -791,6 +802,7 @@
<gr-endpoint-slot name="below"></gr-endpoint-slot>
</gr-endpoint-decorator>
${this.renderCCList()} ${this.renderReviewConfirmation()}
+ ${this.renderPrivateVisiblityInfo()}
</section>
<section class="labelsContainer">${this.renderLabels()}</section>
<section class="newReplyDialog textareaContainer">
@@ -893,6 +905,22 @@
`;
}
+ private renderPrivateVisiblityInfo() {
+ const addedAccounts = [
+ ...(this.reviewersList?.additions() ?? []),
+ ...(this.ccsList?.additions() ?? []),
+ ];
+ if (!this.change?.is_private || !addedAccounts.length) return nothing;
+ return html`
+ <div class="privateVisiblityInfo">
+ <gr-icon icon="info"></gr-icon>
+ <div>
+ Adding a reviewer/CC will make this private change visible to them
+ </div>
+ </div>
+ `;
+ }
+
private renderLabels() {
if (!this.change || !this.account || !this.permittedLabels) return;
return html`
@@ -913,20 +941,13 @@
`;
}
- // TODO: move to comment-util
- private createDraft(): UnsavedInfo {
- return {
- patch_set: this.latestPatchNum,
- message: this.patchsetLevelDraftMessage,
- unresolved: !this.patchsetLevelDraftIsResolved,
- path: SpecialFilePath.PATCHSET_LEVEL_COMMENTS,
- __unsaved: true,
- };
- }
-
private renderPatchsetLevelComment() {
if (!this.patchsetLevelComment)
- this.patchsetLevelComment = this.createDraft();
+ this.patchsetLevelComment = createPatchsetLevelUnsavedDraft(
+ this.latestPatchNum,
+ this.patchsetLevelDraftMessage,
+ !this.patchsetLevelDraftIsResolved
+ );
return html`
<gr-comment
id="patchsetLevelComment"
@@ -1386,6 +1407,14 @@
if (startReview) {
reviewInput.ready = true;
+ } else {
+ const addedAccounts = [
+ ...(this.reviewersList?.additions() ?? []),
+ ...(this.ccsList?.additions() ?? []),
+ ];
+ if (addedAccounts.length > 0) {
+ fireAlert(this, 'Reviewers are not notified for WIP changes');
+ }
}
this.disabled = true;
diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.ts b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.ts
index 633c5b0..52ea252 100644
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.ts
@@ -211,10 +211,7 @@
<div class="peopleListLabel">CC</div>
<gr-account-list allow-any-input="" id="ccs"> </gr-account-list>
</div>
- <dialog
- tabindex="-1"
- id="reviewerConfirmationModal"
- >
+ <dialog tabindex="-1" id="reviewerConfirmationModal">
<div class="reviewerConfirmation">
Group
<span class="groupName"> </span>
@@ -232,7 +229,7 @@
No
</gr-button>
</div>
- </gr-overlay>
+ </dialog>
</section>
<section class="labelsContainer">
<gr-endpoint-decorator name="reply-label-scores">
@@ -330,6 +327,123 @@
);
});
+ test('renders private change info when reviewer is added', async () => {
+ element.change!.is_private = true;
+ element.requestUpdate();
+ await element.updateComplete;
+ const peopleContainer = queryAndAssert<HTMLDivElement>(
+ element,
+ '.peopleContainer'
+ );
+
+ // Info is rendered only if reviewer is added
+ assert.dom.equal(
+ peopleContainer,
+ `
+ <section class="peopleContainer">
+ <gr-endpoint-decorator name="reply-reviewers">
+ <gr-endpoint-param name="change"> </gr-endpoint-param>
+ <gr-endpoint-param name="reviewers"> </gr-endpoint-param>
+ <div class="peopleList">
+ <div class="peopleListLabel">Reviewers</div>
+ <gr-account-list id="reviewers"> </gr-account-list>
+ <gr-endpoint-slot name="right"> </gr-endpoint-slot>
+ </div>
+ <gr-endpoint-slot name="below"> </gr-endpoint-slot>
+ </gr-endpoint-decorator>
+ <div class="peopleList">
+ <div class="peopleListLabel">CC</div>
+ <gr-account-list allow-any-input="" id="ccs"> </gr-account-list>
+ </div>
+ <dialog
+ tabindex="-1"
+ id="reviewerConfirmationModal"
+ >
+ <div class="reviewerConfirmation">
+ Group
+ <span class="groupName"> </span>
+ has
+ <span class="groupSize"> </span>
+ members.
+ <br />
+ Are you sure you want to add them all?
+ </div>
+ <div class="reviewerConfirmationButtons">
+ <gr-button aria-disabled="false" role="button" tabindex="0">
+ Yes
+ </gr-button>
+ <gr-button aria-disabled="false" role="button" tabindex="0">
+ No
+ </gr-button>
+ </div>
+ </dialog>
+ </section>
+ `
+ );
+
+ const account = createAccountWithId(22);
+ element.reviewersList!.accounts = [];
+ element.reviewersList!.addAccountItem({account, count: 1});
+ element.reviewersList!.dispatchEvent(
+ new CustomEvent('account-added', {
+ detail: {account},
+ })
+ );
+ element.requestUpdate();
+ await element.updateComplete;
+
+ assert.dom.equal(
+ peopleContainer,
+ `
+ <section class="peopleContainer">
+ <gr-endpoint-decorator name="reply-reviewers">
+ <gr-endpoint-param name="change"> </gr-endpoint-param>
+ <gr-endpoint-param name="reviewers"> </gr-endpoint-param>
+ <div class="peopleList">
+ <div class="peopleListLabel">Reviewers</div>
+ <gr-account-list id="reviewers"> </gr-account-list>
+ <gr-endpoint-slot name="right"> </gr-endpoint-slot>
+ </div>
+ <gr-endpoint-slot name="below"> </gr-endpoint-slot>
+ </gr-endpoint-decorator>
+ <div class="peopleList">
+ <div class="peopleListLabel">CC</div>
+ <gr-account-list allow-any-input="" id="ccs"> </gr-account-list>
+ </div>
+ <dialog
+ tabindex="-1"
+ id="reviewerConfirmationModal"
+ >
+ <div class="reviewerConfirmation">
+ Group
+ <span class="groupName"> </span>
+ has
+ <span class="groupSize"> </span>
+ members.
+ <br />
+ Are you sure you want to add them all?
+ </div>
+ <div class="reviewerConfirmationButtons">
+ <gr-button aria-disabled="false" role="button" tabindex="0">
+ Yes
+ </gr-button>
+ <gr-button aria-disabled="false" role="button" tabindex="0">
+ No
+ </gr-button>
+ </div>
+ </dialog>
+ <div class="privateVisiblityInfo">
+ <gr-icon icon="info">
+ </gr-icon>
+ <div>
+ Adding a reviewer/CC will make this private change visible to them
+ </div>
+ </div>
+ </section>
+ `
+ );
+ });
+
test('default to publishing draft comments with reply', async () => {
// Async tick is needed because iron-selector content is distributed and
// distributed content requires an observer to be set up.
diff --git a/polygerrit-ui/app/elements/checks/gr-checks-results.ts b/polygerrit-ui/app/elements/checks/gr-checks-results.ts
index f7e3542..3fdb1c0 100644
--- a/polygerrit-ui/app/elements/checks/gr-checks-results.ts
+++ b/polygerrit-ui/app/elements/checks/gr-checks-results.ts
@@ -323,10 +323,18 @@
override updated(changedProperties: PropertyValues) {
if (changedProperties.has('result')) {
- this.isExpandable = !!this.result?.summary && !!this.result?.message;
+ this.isExpandable = this.computeIsExpandable();
}
}
+ private computeIsExpandable() {
+ const hasSummary = !!this.result?.summary;
+ const hasMessage = !!this.result?.message;
+ const hasLinks = (this.result?.links ?? []).length > 0;
+ const hasPointers = (this.result?.codePointers ?? []).length > 0;
+ return hasSummary && (hasMessage || hasLinks || hasPointers);
+ }
+
override focus() {
if (this.nameEl) this.nameEl.focus();
}
diff --git a/polygerrit-ui/app/elements/checks/gr-checks-results_test.ts b/polygerrit-ui/app/elements/checks/gr-checks-results_test.ts
index 934e958..113470c 100644
--- a/polygerrit-ui/app/elements/checks/gr-checks-results_test.ts
+++ b/polygerrit-ui/app/elements/checks/gr-checks-results_test.ts
@@ -117,7 +117,6 @@
aria-checked="false"
aria-label="Expand result row"
class="show-hide"
- hidden=""
role="switch"
tabindex="0"
>
@@ -262,6 +261,7 @@
</h3>
<gr-result-row
class="FAKEErrorFinderFinderFinderFinderFinderFinderFinder"
+ isexpandable
>
</gr-result-row>
<gr-result-row
diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
index f547cc7..6caa000 100644
--- a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
+++ b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
@@ -22,6 +22,7 @@
UrlEncodedCommentId,
PARENT,
PatchSetNumber,
+ BranchName,
} from '../../../types/common';
import {AppElement, AppElementParams} from '../../gr-app-types';
import {LocationChangeEventDetail} from '../../../types/events';
@@ -141,6 +142,10 @@
// Matches /admin/repos/<repo>,commands.
REPO_COMMANDS: /^\/admin\/repos\/(.+),commands$/,
+ // For creating a change, and going directly into editing mode for one file.
+ REPO_EDIT_FILE:
+ /^\/admin\/repos\/edit\/repo\/(.+)\/branch\/(.+)\/file\/(.+)$/,
+
REPO_GENERAL: /^\/admin\/repos\/(.+),general$/,
// Matches /admin/repos/<repos>,access.
@@ -521,6 +526,20 @@
this.redirect(url);
}
+ private dispatchLocationChangeEvent() {
+ const detail: LocationChangeEventDetail = {
+ hash: window.location.hash,
+ pathname: window.location.pathname,
+ };
+ document.dispatchEvent(
+ new CustomEvent('location-change', {
+ detail,
+ composed: true,
+ bubbles: true,
+ })
+ );
+ }
+
startRouter() {
const base = getBaseUrl();
if (base) {
@@ -568,17 +587,7 @@
// Fire asynchronously so that the URL is changed by the time the event
// is processed.
setTimeout(() => {
- const detail: LocationChangeEventDetail = {
- hash: window.location.hash,
- pathname: window.location.pathname,
- };
- document.dispatchEvent(
- new CustomEvent('location-change', {
- detail,
- composed: true,
- bubbles: true,
- })
- );
+ this.dispatchLocationChangeEvent();
}, 1);
next();
});
@@ -676,6 +685,13 @@
true
);
+ this.mapRoute(
+ RoutePattern.REPO_EDIT_FILE,
+ 'handleRepoEditFileRoute',
+ ctx => this.handleRepoEditFileRoute(ctx),
+ true
+ );
+
this.mapRoute(RoutePattern.REPO_GENERAL, 'handleRepoGeneralRoute', ctx =>
this.handleRepoGeneralRoute(ctx)
);
@@ -1151,6 +1167,22 @@
this.reporting.setRepoName(repo);
}
+ handleRepoEditFileRoute(ctx: PageContext) {
+ const repo = ctx.params[0] as RepoName;
+ const branch = ctx.params[1] as BranchName;
+ const path = ctx.params[2];
+ const state: RepoViewState = {
+ view: GerritView.REPO,
+ detail: RepoDetailView.COMMANDS,
+ repo,
+ createEdit: {branch, path},
+ };
+ // Note that router model view must be updated before view models.
+ this.setState(state);
+ this.repoViewModel.setState(state);
+ this.reporting.setRepoName(repo);
+ }
+
handleRepoGeneralRoute(ctx: PageContext) {
const repo = ctx.params[0] as RepoName;
const state: RepoViewState = {
diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-router_test.ts b/polygerrit-ui/app/elements/core/gr-router/gr-router_test.ts
index 597c7c0..b8f68e6 100644
--- a/polygerrit-ui/app/elements/core/gr-router/gr-router_test.ts
+++ b/polygerrit-ui/app/elements/core/gr-router/gr-router_test.ts
@@ -139,6 +139,7 @@
'handlePluginListOffsetRoute',
'handlePluginListRoute',
'handleRepoCommandsRoute',
+ 'handleRepoEditFileRoute',
'handleSettingsLegacyRoute',
'handleSettingsRoute',
];
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
index 36c9397..b68adf1 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
@@ -961,18 +961,18 @@
)}
`
)}
- <span class="separator"></span>
- <div class="diffModeSelector ${diffModeSelectorClass}">
- <span>Diff view:</span>
- <gr-diff-mode-selector
- id="modeSelect"
- .saveOnChange=${this.loggedIn}
- show-tooltip-below
- ></gr-diff-mode-selector>
- </div>
${when(
this.loggedIn && this.prefs,
() => html`
+ <span class="separator"></span>
+ <div class="diffModeSelector ${diffModeSelectorClass}">
+ <span>Diff view:</span>
+ <gr-diff-mode-selector
+ id="modeSelect"
+ .saveOnChange=${this.loggedIn}
+ show-tooltip-below
+ ></gr-diff-mode-selector>
+ </div>
<span id="diffPrefsContainer">
<span class="preferences desktop">
<gr-tooltip-content
diff --git a/polygerrit-ui/app/elements/gr-app-element.ts b/polygerrit-ui/app/elements/gr-app-element.ts
index 2a5bcf5..c4533de 100644
--- a/polygerrit-ui/app/elements/gr-app-element.ts
+++ b/polygerrit-ui/app/elements/gr-app-element.ts
@@ -181,7 +181,7 @@
this.addEventListener(EventType.DIALOG_CHANGE, e => {
this.handleDialogChange(e as CustomEvent<DialogChangeEventDetail>);
});
- this.addEventListener(EventType.LOCATION_CHANGE, () =>
+ document.addEventListener(EventType.LOCATION_CHANGE, () =>
this.handleLocationChange()
);
this.addEventListener(EventType.RECREATE_CHANGE_VIEW, () =>
diff --git a/polygerrit-ui/app/elements/gr-app_test.ts b/polygerrit-ui/app/elements/gr-app_test.ts
index 730548c..05ee9ed 100644
--- a/polygerrit-ui/app/elements/gr-app_test.ts
+++ b/polygerrit-ui/app/elements/gr-app_test.ts
@@ -20,6 +20,26 @@
import {resolve} from '../models/dependency';
import {removeRequestDependencyListener} from '../test/common-test-setup';
+suite('gr-app callback tests', () => {
+ const handleLocationChangeSpy = sinon.spy(
+ GrAppElement.prototype,
+ <any>'handleLocationChange'
+ );
+ const dispatchLocationChangeEventSpy = sinon.spy(
+ GrRouter.prototype,
+ <any>'dispatchLocationChangeEvent'
+ );
+
+ setup(async () => {
+ await fixture<GrApp>(html`<gr-app id="app"></gr-app>`);
+ });
+
+ test("handleLocationChange in gr-app-element is called after dispatching 'location-change' event in gr-router", () => {
+ dispatchLocationChangeEventSpy();
+ assert.isTrue(handleLocationChangeSpy.calledOnce);
+ });
+});
+
suite('gr-app tests', () => {
let grApp: GrApp;
const config = createServerInfo();
diff --git a/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.ts b/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.ts
index b589915..6cdd808 100644
--- a/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.ts
+++ b/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.ts
@@ -880,7 +880,13 @@
private renderBrowserNotifications() {
if (!this.flagsService.isEnabled(KnownExperimentId.PUSH_NOTIFICATIONS))
return nothing;
- if (!areNotificationsEnabled(this.account)) return nothing;
+ if (
+ !this.flagsService.isEnabled(
+ KnownExperimentId.PUSH_NOTIFICATIONS_DEVELOPER
+ ) &&
+ !areNotificationsEnabled(this.account)
+ )
+ return nothing;
return html`
<section id="allowBrowserNotificationsSection">
<label class="title" for="allowBrowserNotifications"
diff --git a/polygerrit-ui/app/elements/shared/gr-autocomplete-dropdown/gr-autocomplete-dropdown.ts b/polygerrit-ui/app/elements/shared/gr-autocomplete-dropdown/gr-autocomplete-dropdown.ts
index 4005bb3..26c8a50 100644
--- a/polygerrit-ui/app/elements/shared/gr-autocomplete-dropdown/gr-autocomplete-dropdown.ts
+++ b/polygerrit-ui/app/elements/shared/gr-autocomplete-dropdown/gr-autocomplete-dropdown.ts
@@ -130,8 +130,12 @@
super();
this.cursor.cursorTargetClass = 'selected';
this.cursor.focusOnMove = true;
- this.shortcuts.addLocal({key: Key.UP}, () => this.handleUp());
- this.shortcuts.addLocal({key: Key.DOWN}, () => this.handleDown());
+ this.shortcuts.addLocal({key: Key.UP, allowRepeat: true}, () =>
+ this.handleUp()
+ );
+ this.shortcuts.addLocal({key: Key.DOWN, allowRepeat: true}, () =>
+ this.handleDown()
+ );
this.shortcuts.addLocal({key: Key.ENTER}, () => this.handleEnter());
this.shortcuts.addLocal({key: Key.ESC}, () => this.handleEscape());
this.shortcuts.addLocal({key: Key.TAB}, () => this.handleTab());
@@ -165,12 +169,7 @@
override render() {
return html`
- <div
- class="dropdown-content"
- slot="dropdown-content"
- id="suggestions"
- role="listbox"
- >
+ <div class="dropdown-content" id="suggestions" role="listbox">
<ul>
${repeat(
this.suggestions,
@@ -209,7 +208,7 @@
}
setPositionTarget(target: HTMLElement) {
- this.fitController?.setPositionTarget(target);
+ this.fitController.setPositionTarget(target);
}
private handleUp() {
@@ -300,7 +299,7 @@
} else {
this.cursor.stops = [];
}
- this.fitController?.refit();
+ this.fitController.refit();
}
private setIndex() {
diff --git a/polygerrit-ui/app/elements/shared/gr-autocomplete-dropdown/gr-autocomplete-dropdown_test.ts b/polygerrit-ui/app/elements/shared/gr-autocomplete-dropdown/gr-autocomplete-dropdown_test.ts
index 641dd2d..cb83396 100644
--- a/polygerrit-ui/app/elements/shared/gr-autocomplete-dropdown/gr-autocomplete-dropdown_test.ts
+++ b/polygerrit-ui/app/elements/shared/gr-autocomplete-dropdown/gr-autocomplete-dropdown_test.ts
@@ -42,12 +42,7 @@
assert.shadowDom.equal(
element,
/* HTML */ `
- <div
- class="dropdown-content"
- id="suggestions"
- role="listbox"
- slot="dropdown-content"
- >
+ <div class="dropdown-content" id="suggestions" role="listbox">
<ul>
<li
aria-label="test name 1"
diff --git a/polygerrit-ui/app/elements/shared/gr-dialog/gr-dialog.ts b/polygerrit-ui/app/elements/shared/gr-dialog/gr-dialog.ts
index dbf75f4..04d5923 100644
--- a/polygerrit-ui/app/elements/shared/gr-dialog/gr-dialog.ts
+++ b/polygerrit-ui/app/elements/shared/gr-dialog/gr-dialog.ts
@@ -31,6 +31,9 @@
* @event cancel
*/
+ @query('#cancel')
+ cancelButton?: GrButton;
+
@query('#confirm')
confirmButton?: GrButton;
@@ -102,6 +105,7 @@
display: flex;
flex-shrink: 0;
padding-top: var(--spacing-xl);
+ align-items: center;
}
.flex-space {
flex-grow: 1;
@@ -221,6 +225,10 @@
}
resetFocus() {
- this.confirmButton!.focus();
+ if (this.disabled && this.cancelLabel) {
+ this.cancelButton!.focus();
+ } else {
+ this.confirmButton!.focus();
+ }
}
}
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-binary.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-binary.ts
index c095ffb..5267f307 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-binary.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-binary.ts
@@ -5,9 +5,12 @@
*/
import {GrDiffBuilderUnified} from './gr-diff-builder-unified';
import {DiffInfo, DiffPreferencesInfo} from '../../../types/diff';
-import {GrDiffLine, GrDiffLineType} from '../gr-diff/gr-diff-line';
-import {queryAndAssert} from '../../../utils/common-util';
import {createElementDiff} from '../gr-diff/gr-diff-utils';
+import {GrDiffGroup} from '../gr-diff/gr-diff-group';
+import {GrDiffLine} from '../gr-diff/gr-diff-line';
+import {GrDiffLineType} from '../../../api/diff';
+import {queryAndAssert} from '../../../utils/common-util';
+import {html, render} from 'lit';
export class GrDiffBuilderBinary extends GrDiffBuilderUnified {
constructor(
@@ -18,12 +21,20 @@
super(diff, prefs, outputEl);
}
- override buildSectionElement(): HTMLElement {
+ override buildSectionElement(group: GrDiffGroup): HTMLElement {
const section = createElementDiff('tbody', 'binary-diff');
+ // Do not create a diff row for 'LOST'.
+ if (group.lines[0].beforeNumber !== 'FILE') return section;
+
const line = new GrDiffLine(GrDiffLineType.BOTH, 'FILE', 'FILE');
const fileRow = this.createRow(line);
- const contentTd = queryAndAssert(fileRow, 'td.both.file')!;
- contentTd.textContent = ' Difference in binary files';
+ const contentTd = queryAndAssert<HTMLTableCellElement>(
+ fileRow,
+ 'td.both.file'
+ )!;
+ const div = document.createElement('div');
+ render(html`<span>Difference in binary files</span>`, div);
+ contentTd.insertBefore(div, contentTd.firstChild);
section.appendChild(fileRow);
return section;
}
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element.ts
index 6cd3cb0..804d101 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element.ts
@@ -7,7 +7,12 @@
import '../../../elements/shared/gr-hovercard/gr-hovercard';
import './gr-diff-builder-side-by-side';
import {GrAnnotation} from '../gr-diff-highlight/gr-annotation';
-import {DiffBuilder, DiffContextExpandedEventDetail} from './gr-diff-builder';
+import {
+ DiffBuilder,
+ ImageDiffBuilder,
+ DiffContextExpandedEventDetail,
+ isImageDiffBuilder,
+} from './gr-diff-builder';
import {GrDiffBuilderSideBySide} from './gr-diff-builder-side-by-side';
import {GrDiffBuilderImage} from './gr-diff-builder-image';
import {GrDiffBuilderUnified} from './gr-diff-builder-unified';
@@ -114,7 +119,7 @@
layers: DiffLayer[] = [];
// visible for testing
- builder?: DiffBuilder;
+ builder?: DiffBuilder | ImageDiffBuilder;
/**
* All layers, both from the outside and the default ones. See `layers` for
@@ -206,8 +211,8 @@
return (
this.cancelableRenderPromise
.then(async () => {
- if (this.isImageDiff) {
- (this.builder as GrDiffBuilderImage).renderDiff();
+ if (isImageDiffBuilder(this.builder)) {
+ this.builder.renderImageDiff();
}
await this.untilGroupsRendered();
this.fireDiffEvent('render-content');
@@ -518,7 +523,7 @@
// If endIndex isn't present, continue to the end of the line.
const endIndex =
highlight.endIndex === undefined
- ? line.text.length
+ ? GrAnnotation.getStringLength(line.text)
: highlight.endIndex;
GrAnnotation.annotateElement(
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element_test.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element_test.ts
index 5eabc59..0f02d71 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element_test.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element_test.ts
@@ -271,10 +271,11 @@
const str0 = slice(str, 0, 6);
const str1 = slice(str, 6);
+ const numHighlightedChars = GrAnnotation.getStringLength(str1);
layer.annotate(el, lineNumberEl, l, Side.LEFT);
- assert.isTrue(annotateElementSpy.called);
+ assert.isTrue(annotateElementSpy.calledWith(el, 6, numHighlightedChars));
assert.equal(el.childNodes.length, 2);
assert.instanceOf(el.childNodes[0], Text);
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-image.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-image.ts
index 75ee088..096d32e 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-image.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-image.ts
@@ -3,228 +3,265 @@
* Copyright 2016 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
-
import {GrDiffBuilderSideBySide} from './gr-diff-builder-side-by-side';
import {ImageInfo} from '../../../types/common';
import {DiffInfo, DiffPreferencesInfo} from '../../../types/diff';
-import {GrEndpointParam} from '../../../elements/plugins/gr-endpoint-param/gr-endpoint-param';
-import {RenderPreferences} from '../../../api/diff';
+import {RenderPreferences, Side} from '../../../api/diff';
import '../gr-diff-image-viewer/gr-image-viewer';
-import {GrImageViewer} from '../gr-diff-image-viewer/gr-image-viewer';
-import {createElementDiff} from '../gr-diff/gr-diff-utils';
+import {ImageDiffBuilder} from './gr-diff-builder';
+import {html, LitElement, nothing, render} from 'lit';
+import {customElement, property, query, state} from 'lit/decorators.js';
// MIME types for images we allow showing. Do not include SVG, it can contain
// arbitrary JavaScript.
const IMAGE_MIME_PATTERN = /^image\/(bmp|gif|x-icon|jpeg|jpg|png|tiff|webp)$/;
-export class GrDiffBuilderImage extends GrDiffBuilderSideBySide {
+export class GrDiffBuilderImage
+ extends GrDiffBuilderSideBySide
+ implements ImageDiffBuilder
+{
constructor(
diff: DiffInfo,
prefs: DiffPreferencesInfo,
outputEl: HTMLElement,
- private readonly _baseImage: ImageInfo | null,
- private readonly _revisionImage: ImageInfo | null,
+ private readonly baseImage: ImageInfo | null,
+ private readonly revisionImage: ImageInfo | null,
renderPrefs?: RenderPreferences,
- private readonly _useNewImageDiffUi: boolean = false
+ private readonly useNewImageDiffUi: boolean = false
) {
super(diff, prefs, outputEl, [], renderPrefs);
}
- public renderDiff() {
- const section = createElementDiff('tbody', 'image-diff');
-
- if (this._useNewImageDiffUi) {
- this._emitImageViewer(section);
-
- this.outputEl.appendChild(section);
- } else {
- this._emitImagePair(section);
- this._emitImageLabels(section);
-
- this.outputEl.appendChild(section);
- this.outputEl.appendChild(this._createEndpoint());
- }
- }
-
- private _createEndpoint() {
- const tbody = createElementDiff('tbody');
- const tr = createElementDiff('tr');
- const td = createElementDiff('td');
-
- // TODO(kaspern): Support blame for image diffs and remove the hardcoded 4
- // column limit.
- td.setAttribute('colspan', '4');
- const endpointDomApi = createElementDiff('gr-endpoint-decorator');
- endpointDomApi.setAttribute('name', 'image-diff');
- endpointDomApi.appendChild(
- this._createEndpointParam('baseImage', this._baseImage)
+ public renderImageDiff() {
+ render(
+ html`
+ ${this.useNewImageDiffUi
+ ? html`
+ <gr-diff-image-new
+ .automaticBlink=${this.autoBlink()}
+ .baseImage=${this.baseImage ?? undefined}
+ .revisionImage=${this.revisionImage ?? undefined}
+ ></gr-diff-image-new>
+ `
+ : html`
+ <gr-diff-image-old
+ .baseImage=${this.baseImage ?? undefined}
+ .revisionImage=${this.revisionImage ?? undefined}
+ ></gr-diff-image-old>
+ `}
+ `,
+ this.outputEl
);
- endpointDomApi.appendChild(
- this._createEndpointParam('revisionImage', this._revisionImage)
- );
- td.appendChild(endpointDomApi);
- tr.appendChild(td);
- tbody.appendChild(tr);
- return tbody;
}
- private _createEndpointParam(name: string, value: ImageInfo | null) {
- const endpointParam = createElementDiff(
- 'gr-endpoint-param'
- ) as GrEndpointParam;
- endpointParam.name = name;
- endpointParam.value = value;
- return endpointParam;
- }
-
- private _emitImageViewer(section: HTMLElement) {
- const tr = createElementDiff('tr');
- const td = createElementDiff('td');
- // TODO(hermannloose): Support blame for image diffs, see above.
- td.setAttribute('colspan', '4');
- const imageViewer = createElementDiff('gr-image-viewer') as GrImageViewer;
-
- imageViewer.baseUrl = this._getImageSrc(this._baseImage);
- imageViewer.revisionUrl = this._getImageSrc(this._revisionImage);
- imageViewer.automaticBlink =
- !!this.renderPrefs?.image_diff_prefs?.automatic_blink;
-
- td.appendChild(imageViewer);
- tr.appendChild(td);
- section.appendChild(tr);
- }
-
- private _getImageSrc(image: ImageInfo | null): string {
- return image && IMAGE_MIME_PATTERN.test(image.type)
- ? `data:${image.type};base64,${image.body}`
- : '';
- }
-
- private _emitImagePair(section: HTMLElement) {
- const tr = createElementDiff('tr');
-
- tr.appendChild(createElementDiff('td', 'left lineNum blank'));
- tr.appendChild(this._createImageCell(this._baseImage, 'left', section));
-
- tr.appendChild(createElementDiff('td', 'right lineNum blank'));
- tr.appendChild(
- this._createImageCell(this._revisionImage, 'right', section)
- );
-
- section.appendChild(tr);
- }
-
- private _createImageCell(
- image: ImageInfo | null,
- className: string,
- section: HTMLElement
- ) {
- const td = createElementDiff('td', className);
- const src = this._getImageSrc(image);
- if (image && src) {
- const imageEl = createElementDiff('img') as HTMLImageElement;
- imageEl.onload = () => {
- image._height = imageEl.naturalHeight;
- image._width = imageEl.naturalWidth;
- this._updateImageLabel(section, className, image);
- };
- imageEl.addEventListener('error', (e: Event) => {
- imageEl.remove();
- td.textContent = '[Image failed to load] ' + e.type;
- });
- imageEl.setAttribute('src', src);
- td.appendChild(imageEl);
- }
- return td;
- }
-
- private _updateImageLabel(
- section: HTMLElement,
- className: string,
- image: ImageInfo
- ) {
- const label = section.querySelector(
- '.' + className + ' span.label'
- ) as HTMLElement;
- this._setLabelText(label, image);
- }
-
- private _setLabelText(label: HTMLElement, image: ImageInfo | null) {
- label.textContent = getImageLabel(image);
- }
-
- private _emitImageLabels(section: HTMLElement) {
- const tr = createElementDiff('tr');
-
- let addNamesInLabel = false;
-
- if (
- this._baseImage &&
- this._revisionImage &&
- this._baseImage._name !== this._revisionImage._name
- ) {
- addNamesInLabel = true;
- }
-
- tr.appendChild(createElementDiff('td', 'left lineNum blank'));
- let td = createElementDiff('td', 'left');
- let label = createElementDiff('label');
- let nameSpan;
- let labelSpan = createElementDiff('span', 'label');
-
- if (addNamesInLabel) {
- nameSpan = createElementDiff('span', 'name');
- nameSpan.textContent = this._baseImage?._name ?? '';
- label.appendChild(nameSpan);
- label.appendChild(createElementDiff('br'));
- }
-
- this._setLabelText(labelSpan, this._baseImage);
-
- label.appendChild(labelSpan);
- td.appendChild(label);
- tr.appendChild(td);
-
- tr.appendChild(createElementDiff('td', 'right lineNum blank'));
- td = createElementDiff('td', 'right');
- label = createElementDiff('label');
- labelSpan = createElementDiff('span', 'label');
-
- if (addNamesInLabel) {
- nameSpan = createElementDiff('span', 'name');
- nameSpan.textContent = this._revisionImage?._name ?? '';
- label.appendChild(nameSpan);
- label.appendChild(createElementDiff('br'));
- }
-
- this._setLabelText(labelSpan, this._revisionImage);
-
- label.appendChild(labelSpan);
- td.appendChild(label);
- tr.appendChild(td);
-
- section.appendChild(tr);
+ private autoBlink(): boolean {
+ return !!this.renderPrefs?.image_diff_prefs?.automatic_blink;
}
override updateRenderPrefs(renderPrefs: RenderPreferences) {
- const imageViewer = this.outputEl.querySelector(
- 'gr-image-viewer'
- ) as GrImageViewer;
- if (this._useNewImageDiffUi && imageViewer) {
- imageViewer.automaticBlink =
- !!renderPrefs?.image_diff_prefs?.automatic_blink;
- }
+ this.renderPrefs = renderPrefs;
+
+ // We have to update `imageDiff.automaticBlink` manually, because `this` is
+ // not a LitElement.
+ const imageDiff = this.outputEl.querySelector(
+ 'gr-diff-image-new'
+ ) as GrDiffImageNew;
+ if (imageDiff) imageDiff.automaticBlink = this.autoBlink();
}
}
-function getImageLabel(image: ImageInfo | null) {
- if (image) {
- const type = image.type ?? image._expectedType;
- if (image._width && image._height) {
- return `${image._width}×${image._height} ${type}`;
- } else {
- return type;
- }
+@customElement('gr-diff-image-new')
+class GrDiffImageNew extends LitElement {
+ @property() baseImage?: ImageInfo;
+
+ @property() revisionImage?: ImageInfo;
+
+ @property() automaticBlink = false;
+
+ /**
+ * The browser API for handling selection does not (yet) work for selection
+ * across multiple shadow DOM elements. So we are rendering gr-diff components
+ * into the light DOM instead of the shadow DOM by overriding this method,
+ * which was the recommended workaround by the lit team.
+ * See also https://github.com/WICG/webcomponents/issues/79.
+ */
+ override createRenderRoot() {
+ return this;
}
- return 'No image';
+
+ override render() {
+ return html`
+ <tbody class="gr-diff image-diff">
+ <tr class="gr-diff">
+ <td class="gr-diff" colspan="4">
+ <gr-image-viewer
+ class="gr-diff"
+ .baseUrl=${imageSrc(this.baseImage)}
+ .revisionUrl=${imageSrc(this.revisionImage)}
+ .automaticBlink=${this.automaticBlink}
+ >
+ </gr-image-viewer>
+ </td>
+ </tr>
+ </tbody>
+ `;
+ }
+}
+
+@customElement('gr-diff-image-old')
+class GrDiffImageOld extends LitElement {
+ @property() baseImage?: ImageInfo;
+
+ @property() revisionImage?: ImageInfo;
+
+ @query('img.left') baseImageEl?: HTMLImageElement;
+
+ @query('img.right') revisionImageEl?: HTMLImageElement;
+
+ @state() baseError?: string;
+
+ @state() revisionError?: string;
+
+ /**
+ * The browser API for handling selection does not (yet) work for selection
+ * across multiple shadow DOM elements. So we are rendering gr-diff components
+ * into the light DOM instead of the shadow DOM by overriding this method,
+ * which was the recommended workaround by the lit team.
+ * See also https://github.com/WICG/webcomponents/issues/79.
+ */
+ override createRenderRoot() {
+ return this;
+ }
+
+ override render() {
+ return html`
+ <tbody class="gr-diff image-diff">
+ ${this.renderImagePairRow()} ${this.renderImageLabelRow()}
+ </tbody>
+ ${this.renderEndpoint()}
+ `;
+ }
+
+ private renderEndpoint() {
+ return html`
+ <tbody class="gr-diff endpoint">
+ <tr class="gr-diff">
+ <td class="gr-diff" colspan="4">
+ <gr-endpoint-decorator class="gr-diff" name="image-diff">
+ ${this.renderEndpointParam('baseImage', this.baseImage)}
+ ${this.renderEndpointParam('revisionImage', this.revisionImage)}
+ </gr-endpoint-decorator>
+ </td>
+ </tr>
+ </tbody>
+ `;
+ }
+
+ private renderEndpointParam(name: string, value: unknown) {
+ if (!value) return nothing;
+ return html`
+ <gr-endpoint-param class="gr-diff" name=${name} .value=${value}>
+ </gr-endpoint-param>
+ `;
+ }
+
+ private renderImagePairRow() {
+ return html`
+ <tr class="gr-diff">
+ <td class="gr-diff left lineNum blank"></td>
+ <td class="gr-diff left">${this.renderImage(Side.LEFT)}</td>
+ <td class="gr-diff right lineNum blank"></td>
+ <td class="gr-diff right">${this.renderImage(Side.RIGHT)}</td>
+ </tr>
+ `;
+ }
+
+ private renderImage(side: Side) {
+ const image = side === Side.LEFT ? this.baseImage : this.revisionImage;
+ if (!image) return nothing;
+ const error = side === Side.LEFT ? this.baseError : this.revisionError;
+ if (error) return error;
+ const src = imageSrc(image);
+ if (!src) return nothing;
+
+ return html`
+ <img
+ class="gr-diff ${side}"
+ src=${src}
+ @load=${this.handleLoad}
+ @error=${(e: Event) => this.handleError(e, side)}
+ >
+ </img>
+ `;
+ }
+
+ private handleLoad() {
+ this.requestUpdate();
+ }
+
+ private handleError(e: Event, side: Side) {
+ const msg = `[Image failed to load] ${e.type}`;
+ if (side === Side.LEFT) this.baseError = msg;
+ if (side === Side.RIGHT) this.revisionError = msg;
+ }
+
+ private renderImageLabelRow() {
+ return html`
+ <tr class="gr-diff">
+ <td class="gr-diff left lineNum blank"></td>
+ <td class="gr-diff left">
+ <label class="gr-diff">
+ ${this.renderName(this.baseImage?._name ?? '')}
+ <span class="gr-diff label">${this.imageLabel(Side.LEFT)}</span>
+ </label>
+ </td>
+ <td class="gr-diff right lineNum blank"></td>
+ <td class="gr-diff right">
+ <label class="gr-diff">
+ ${this.renderName(this.revisionImage?._name ?? '')}
+ <span class="gr-diff label"> ${this.imageLabel(Side.RIGHT)} </span>
+ </label>
+ </td>
+ </tr>
+ `;
+ }
+
+ private renderName(name?: string) {
+ const addNamesInLabel =
+ this.baseImage &&
+ this.revisionImage &&
+ this.baseImage._name !== this.revisionImage._name;
+ if (!addNamesInLabel) return nothing;
+ return html`
+ <span class="gr-diff name">${name}</span><br class="gr-diff" />
+ `;
+ }
+
+ private imageLabel(side: Side) {
+ const image = side === Side.LEFT ? this.baseImage : this.revisionImage;
+ const imageEl =
+ side === Side.LEFT ? this.baseImageEl : this.revisionImageEl;
+ if (image) {
+ const type = image.type ?? image._expectedType;
+ if (imageEl?.naturalWidth && imageEl.naturalHeight) {
+ return `${imageEl?.naturalWidth}×${imageEl.naturalHeight} ${type}`;
+ } else {
+ return type;
+ }
+ }
+ return 'No image';
+ }
+}
+
+function imageSrc(image?: ImageInfo): string {
+ return image && IMAGE_MIME_PATTERN.test(image.type)
+ ? `data:${image.type};base64,${image.body}`
+ : '';
+}
+
+declare global {
+ interface HTMLElementTagNameMap {
+ 'gr-diff-image-new': GrDiffImageNew;
+ 'gr-diff-image-old': GrDiffImageOld;
+ }
}
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-legacy.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-legacy.ts
index 2c9f210..090d125 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-legacy.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-legacy.ts
@@ -226,6 +226,7 @@
}
const cell = createElementDiff('td', 'dividerCell');
+ // Note that <td> table cells that have `display: none` don't count!
const colspan = this.renderPrefs?.show_sign_col ? '5' : '3';
cell.setAttribute('colspan', colspan);
row.appendChild(cell);
@@ -363,23 +364,26 @@
}
td.classList.add(line.type);
- const {beforeNumber, afterNumber} = line;
- if (beforeNumber !== 'FILE' && beforeNumber !== 'LOST') {
+ const lineNumber = side ? line.lineNumber(side) : 0;
+ if (lineNumber === 'FILE') {
+ td.classList.add('file');
+ } else if (lineNumber === 'LOST') {
+ td.classList.add('lost');
+ } else {
const responsiveMode = getResponsiveMode(this._prefs, this.renderPrefs);
+ const contentId =
+ side && lineNumber > 0 ? `${side}-content-${lineNumber}` : '';
const contentText = formatText(
line.text,
responsiveMode,
this._prefs.tab_size,
this._prefs.line_length,
- side === Side.LEFT
- ? `left-content-${beforeNumber}`
- : `right-content-${afterNumber}`
+ contentId
);
if (side) {
contentText.setAttribute('data-side', side);
- const number = side === Side.LEFT ? beforeNumber : afterNumber;
- this.addLineNumberMouseEvents(td, number, side);
+ this.addLineNumberMouseEvents(td, lineNumber, side);
}
if (lineNumberEl && side) {
@@ -393,11 +397,9 @@
}
td.appendChild(contentText);
- } else if (line.beforeNumber === 'FILE') td.classList.add('file');
- else if (line.beforeNumber === 'LOST') td.classList.add('lost');
+ }
- if (side && line.lineNumber(side)) {
- const lineNumber = line.lineNumber(side);
+ if (side && lineNumber) {
const threadGroupEl = document.createElement('div');
threadGroupEl.className = 'thread-group';
threadGroupEl.setAttribute('data-side', side);
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-side-by-side.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-side-by-side.ts
index 10a947d..6ae7d4cd 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-side-by-side.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-side-by-side.ts
@@ -83,14 +83,14 @@
colgroup.appendChild(createElementDiff('col', 'left'));
// Add right-side line number.
- col = document.createElement('col');
+ col = createElementDiff('col', 'right');
col.setAttribute('width', lineNumberWidth.toString());
colgroup.appendChild(col);
colgroup.appendChild(createElementDiff('col', 'sign right'));
// Add right-side content.
- colgroup.appendChild(document.createElement('col'));
+ colgroup.appendChild(createElementDiff('col', 'right'));
outputEl.appendChild(colgroup);
}
@@ -108,24 +108,17 @@
// pushing browser to compute aria even for tr. This can be removed, once
// browsers will again compute a11y label even for tr when it is focused.
// TODO: Remove when Chrome 102 is out of date for 1 year.
- if (
- leftLine.beforeNumber !== 'FILE' &&
- leftLine.beforeNumber !== 'LOST' &&
- rightLine.beforeNumber !== 'FILE' &&
- rightLine.beforeNumber !== 'LOST'
- ) {
- row.setAttribute(
- 'aria-labelledby',
- [
- leftLine.beforeNumber ? `left-button-${leftLine.beforeNumber}` : '',
- leftLine.beforeNumber ? `left-content-${leftLine.beforeNumber}` : '',
- rightLine.afterNumber ? `right-button-${rightLine.afterNumber}` : '',
- rightLine.afterNumber ? `right-content-${rightLine.afterNumber}` : '',
- ]
- .join(' ')
- .trim()
- );
- }
+ row.setAttribute(
+ 'aria-labelledby',
+ [
+ leftLine.beforeNumber ? `left-button-${leftLine.beforeNumber}` : '',
+ leftLine.beforeNumber ? `left-content-${leftLine.beforeNumber}` : '',
+ rightLine.afterNumber ? `right-button-${rightLine.afterNumber}` : '',
+ rightLine.afterNumber ? `right-content-${rightLine.afterNumber}` : '',
+ ]
+ .join(' ')
+ .trim()
+ );
row.appendChild(this.createBlameCell(leftLine.beforeNumber));
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder.ts
index f3c88a9..5ca5197 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder.ts
@@ -64,6 +64,16 @@
updateRenderPrefs(renderPrefs: RenderPreferences): void;
}
+export interface ImageDiffBuilder extends DiffBuilder {
+ renderImageDiff(): void;
+}
+
+export function isImageDiffBuilder(
+ x: DiffBuilder | ImageDiffBuilder | undefined
+): x is ImageDiffBuilder {
+ return !!x && !!(x as ImageDiffBuilder).renderImageDiff;
+}
+
/**
* Base class for different diff builders, like side-by-side, unified etc.
*
@@ -82,7 +92,7 @@
// visible for testing
readonly _prefs: DiffPreferencesInfo;
- protected readonly renderPrefs?: RenderPreferences;
+ protected renderPrefs?: RenderPreferences;
protected readonly outputEl: HTMLElement;
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-row.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-row.ts
index 4783042..f1ce0e3 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-row.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-row.ts
@@ -60,14 +60,6 @@
layers: DiffLayer[] = [];
/**
- * While not visible we are trying to optimize rendering performance by
- * rendering a simpler version of the diff. Once this has become true it
- * cannot be set back to false.
- */
- @state()
- isVisible = false;
-
- /**
* Semantic DOM diff testing does not work with just table fragments, so when
* running such tests the render() method has to wrap the DOM in a proper
* <table> element.
@@ -120,7 +112,6 @@
* `this.layersApplied = true`.
*/
private async updateLayers(side: Side) {
- if (!this.isVisible) return;
const line = this.line(side);
const contentEl = this.contentRef(side).value;
const lineNumberEl = this.lineNumberRef(side).value;
@@ -139,25 +130,8 @@
this.layersApplied = true;
}
- private renderInvisible() {
- return html`
- <tr>
- <td class="gr-diff blame"></td>
- <td class="gr-diff left"></td>
- <td class="gr-diff left content">
- <div>${this.left?.text ?? ''}</div>
- </td>
- <td class="gr-diff right"></td>
- <td class="gr-diff right content">
- <div>${this.right?.text ?? ''}</div>
- </td>
- </tr>
- `;
- }
-
override render() {
if (!this.left || !this.right) return;
- if (!this.isVisible) return this.renderInvisible();
const row = html`
<tr
${ref(this.tableRowRef)}
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-row_test.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-row_test.ts
index 4e8bb62..da3230f 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-row_test.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-row_test.ts
@@ -15,7 +15,6 @@
setup(async () => {
element = await fixture<GrDiffRow>(html`<gr-diff-row></gr-diff-row>`);
- element.isVisible = true;
element.addTableWrapperForTesting = true;
await element.updateComplete;
});
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-section.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-section.ts
index 16c7cb3..e0dd6bc 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-section.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-section.ts
@@ -22,7 +22,6 @@
import '../gr-context-controls/gr-context-controls';
import '../gr-range-header/gr-range-header';
import './gr-diff-row';
-import {whenVisible} from '../../../utils/dom-util';
@customElement('gr-diff-section')
export class GrDiffSection extends LitElement {
@@ -42,13 +41,6 @@
layers: DiffLayer[] = [];
/**
- * While not visible we are trying to optimize rendering performance by
- * rendering a simpler version of the diff.
- */
- @state()
- isVisible = false;
-
- /**
* Semantic DOM diff testing does not work with just table fragments, so when
* running such tests the render() method has to wrap the DOM in a proper
* <table> element.
@@ -67,12 +59,6 @@
return this;
}
- override connectedCallback() {
- super.connectedCallback();
- // TODO: Refine this obviously simplistic approach to optimized rendering.
- whenVisible(this.parentElement!, () => (this.isVisible = true), 1000);
- }
-
override render() {
if (!this.group) return;
const extras: string[] = [];
@@ -100,7 +86,6 @@
.layers=${this.layers}
.lineLength=${this.diffPrefs?.line_length ?? 80}
.tabSize=${this.diffPrefs?.tab_size ?? 2}
- .isVisible=${this.isVisible}
>
</gr-diff-row>
`;
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-section_test.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-section_test.ts
index 363b001..14efa22 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-section_test.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-section_test.ts
@@ -19,7 +19,6 @@
html`<gr-diff-section></gr-diff-section>`
);
element.addTableWrapperForTesting = true;
- element.isVisible = true;
await element.updateComplete;
});
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-cursor/gr-diff-cursor.ts b/polygerrit-ui/app/embed/diff/gr-diff-cursor/gr-diff-cursor.ts
index 35439d6..e5fce12 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-cursor/gr-diff-cursor.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-cursor/gr-diff-cursor.ts
@@ -423,7 +423,10 @@
}
_rowHasThread(row: HTMLElement): boolean {
- return !!row.querySelector('.thread-group');
+ const slots = [
+ ...row.querySelectorAll<HTMLSlotElement>('.thread-group > slot'),
+ ];
+ return slots.some(slot => slot.assignedElements().length > 0);
}
/**
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-highlight/gr-annotation.ts b/polygerrit-ui/app/embed/diff/gr-diff-highlight/gr-annotation.ts
index cc7cd49..92175eb 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-highlight/gr-annotation.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-highlight/gr-annotation.ts
@@ -22,8 +22,14 @@
return this.getStringLength(node.textContent || '');
},
+ /**
+ * Returns the number of Unicode code points in the given string
+ *
+ * This is not necessarily the same as the number of visible symbols.
+ * See https://mathiasbynens.be/notes/javascript-unicode for more details.
+ */
getStringLength(str: string) {
- return str.replace(REGEX_ASTRAL_SYMBOL, '_').length;
+ return [...str].length;
},
/**
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-highlight/gr-annotation_test.ts b/polygerrit-ui/app/embed/diff/gr-diff-highlight/gr-annotation_test.ts
index 6c45f20..fdf1785 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-highlight/gr-annotation_test.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-highlight/gr-annotation_test.ts
@@ -327,4 +327,20 @@
assert.equal(el.getAttribute('class'), 'hello world');
});
});
+
+ suite('getStringLength', () => {
+ test('ASCII characters are counted correctly', () => {
+ assert.equal(GrAnnotation.getStringLength('ASCII'), 5);
+ });
+
+ test('Unicode surrogate pairs count as one symbol', () => {
+ assert.equal(GrAnnotation.getStringLength('Unic💢de'), 7);
+ assert.equal(GrAnnotation.getStringLength('💢💢'), 2);
+ });
+
+ test('Grapheme clusters count as multiple symbols', () => {
+ assert.equal(GrAnnotation.getStringLength('man\u0303ana'), 7); // mañana
+ assert.equal(GrAnnotation.getStringLength('q\u0307\u0323'), 3); // q̣̇
+ });
+ });
});
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor.ts b/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor.ts
index 9f874b8..22a71a5 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor.ts
@@ -21,6 +21,7 @@
import {debounce, DelayedTask} from '../../../utils/async-util';
import {RenderPreferences} from '../../../api/diff';
import {assertIsDefined} from '../../../utils/common-util';
+import {GrAnnotation} from '../gr-diff-highlight/gr-annotation';
const WHOLE_FILE = -1;
@@ -639,16 +640,19 @@
rows: string[],
intralineInfos: number[][]
): Highlights[] {
+ // +1 to account for the \n that is not part of the rows passed here
+ const lineLengths = rows.map(r => GrAnnotation.getStringLength(r) + 1);
+
let rowIndex = 0;
let idx = 0;
const normalized = [];
for (const [skipLength, markLength] of intralineInfos) {
- let line = rows[rowIndex] + '\n';
+ let lineLength = lineLengths[rowIndex];
let j = 0;
while (j < skipLength) {
- if (idx === line.length) {
+ if (idx === lineLength) {
idx = 0;
- line = rows[++rowIndex] + '\n';
+ lineLength = lineLengths[++rowIndex];
continue;
}
idx++;
@@ -660,10 +664,10 @@
};
j = 0;
- while (line && j < markLength) {
- if (idx === line.length) {
+ while (lineLength && j < markLength) {
+ if (idx === lineLength) {
idx = 0;
- line = rows[++rowIndex] + '\n';
+ lineLength = lineLengths[++rowIndex];
normalized.push(lineHighlight);
lineHighlight = {
contentIndex: rowIndex,
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor_test.ts b/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor_test.ts
index 6caeb62..f6f7052 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor_test.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor_test.ts
@@ -723,6 +723,25 @@
endIndex: 41,
},
]);
+
+ content = ['🙈 a', '🙉 b', '🙊 c'];
+ highlights = [[2, 7]];
+ results = element.convertIntralineInfos(content, highlights);
+ assert.deepEqual(results, [
+ {
+ contentIndex: 0,
+ startIndex: 2,
+ },
+ {
+ contentIndex: 1,
+ startIndex: 0,
+ },
+ {
+ contentIndex: 2,
+ startIndex: 0,
+ endIndex: 1,
+ },
+ ]);
});
test('scrolling pauses rendering', () => {
diff --git a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff.ts b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff.ts
index f9a31b4..569de48 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff.ts
@@ -973,6 +973,8 @@
z-index: 10;
}
+ gr-diff-image-new,
+ gr-diff-image-old,
gr-diff-section,
gr-context-controls-section,
gr-diff-row {
@@ -1004,6 +1006,7 @@
if (this.diffTable && this.diffBuilder) {
this.highlights.init(this.diffTable, this.diffBuilder);
}
+ this.diffBuilder.init();
}
override disconnectedCallback() {
diff --git a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff_test.ts b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff_test.ts
index 1db3945..0dc1e12 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff_test.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff_test.ts
@@ -93,12 +93,13 @@
<col class="gr-diff left" width="48" />
<col class="gr-diff left sign" />
<col class="gr-diff left" />
- <col width="48" />
+ <col class="gr-diff right" width="48" />
<col class="gr-diff right sign" />
- <col />
+ <col class="gr-diff right" />
</colgroup>
<tbody class="both gr-diff section">
<tr
+ aria-labelledby="left-button-LOST left-content-LOST right-button-LOST right-content-LOST"
class="diff-row gr-diff side-by-side"
left-type="both"
right-type="both"
@@ -119,6 +120,7 @@
</tbody>
<tbody class="both gr-diff section">
<tr
+ aria-labelledby="left-button-FILE left-content-FILE right-button-FILE right-content-FILE"
class="diff-row gr-diff side-by-side"
left-type="both"
right-type="both"
@@ -367,11 +369,7 @@
<td class="blankLineNum gr-diff left"></td>
<td class="blank gr-diff left no-intraline-info sign"></td>
<td class="blank gr-diff left no-intraline-info">
- <div
- class="contentText gr-diff"
- data-side="left"
- id="left-content-0"
- ></div>
+ <div class="contentText gr-diff" data-side="left"></div>
</td>
<td class="gr-diff lineNum right" data-value="5">
<button
@@ -405,11 +403,7 @@
<td class="blankLineNum gr-diff left"></td>
<td class="blank gr-diff left no-intraline-info sign"></td>
<td class="blank gr-diff left no-intraline-info">
- <div
- class="contentText gr-diff"
- data-side="left"
- id="left-content-0"
- ></div>
+ <div class="contentText gr-diff" data-side="left"></div>
</td>
<td class="gr-diff lineNum right" data-value="6">
<button
@@ -443,11 +437,7 @@
<td class="blankLineNum gr-diff left"></td>
<td class="blank gr-diff left no-intraline-info sign"></td>
<td class="blank gr-diff left no-intraline-info">
- <div
- class="contentText gr-diff"
- data-side="left"
- id="left-content-0"
- ></div>
+ <div class="contentText gr-diff" data-side="left"></div>
</td>
<td class="gr-diff lineNum right" data-value="7">
<button
@@ -750,11 +740,7 @@
<td class="blankLineNum gr-diff right"></td>
<td class="blank gr-diff no-intraline-info right sign"></td>
<td class="blank gr-diff no-intraline-info right">
- <div
- class="contentText gr-diff"
- data-side="right"
- id="right-content-0"
- ></div>
+ <div class="contentText gr-diff" data-side="right"></div>
</td>
</tr>
<tr
@@ -788,11 +774,7 @@
<td class="blankLineNum gr-diff right"></td>
<td class="blank gr-diff no-intraline-info right sign"></td>
<td class="blank gr-diff no-intraline-info right">
- <div
- class="contentText gr-diff"
- data-side="right"
- id="right-content-0"
- ></div>
+ <div class="contentText gr-diff" data-side="right"></div>
</td>
</tr>
<tr
@@ -826,11 +808,7 @@
<td class="blankLineNum gr-diff right"></td>
<td class="blank gr-diff no-intraline-info right sign"></td>
<td class="blank gr-diff no-intraline-info right">
- <div
- class="contentText gr-diff"
- data-side="right"
- id="right-content-0"
- ></div>
+ <div class="contentText gr-diff" data-side="right"></div>
</td>
</tr>
<tr
@@ -864,11 +842,7 @@
<td class="blankLineNum gr-diff right"></td>
<td class="blank gr-diff no-intraline-info right sign"></td>
<td class="blank gr-diff no-intraline-info right">
- <div
- class="contentText gr-diff"
- data-side="right"
- id="right-content-0"
- ></div>
+ <div class="contentText gr-diff" data-side="right"></div>
</td>
</tr>
</tbody>
@@ -1371,11 +1345,7 @@
<td class="blankLineNum gr-diff left"></td>
<td class="blank gr-diff left no-intraline-info sign"></td>
<td class="blank gr-diff left no-intraline-info">
- <div
- class="contentText gr-diff"
- data-side="left"
- id="left-content-0"
- ></div>
+ <div class="contentText gr-diff" data-side="left"></div>
</td>
<td class="gr-diff lineNum right" data-value="40">
<button
@@ -1409,11 +1379,7 @@
<td class="blankLineNum gr-diff left"></td>
<td class="blank gr-diff left no-intraline-info sign"></td>
<td class="blank gr-diff left no-intraline-info">
- <div
- class="contentText gr-diff"
- data-side="left"
- id="left-content-0"
- ></div>
+ <div class="contentText gr-diff" data-side="left"></div>
</td>
<td class="gr-diff lineNum right" data-value="41">
<button
@@ -1447,11 +1413,7 @@
<td class="blankLineNum gr-diff left"></td>
<td class="blank gr-diff left no-intraline-info sign"></td>
<td class="blank gr-diff left no-intraline-info">
- <div
- class="contentText gr-diff"
- data-side="left"
- id="left-content-0"
- ></div>
+ <div class="contentText gr-diff" data-side="left"></div>
</td>
<td class="gr-diff lineNum right" data-value="42">
<button
@@ -1485,11 +1447,7 @@
<td class="blankLineNum gr-diff left"></td>
<td class="blank gr-diff left no-intraline-info sign"></td>
<td class="blank gr-diff left no-intraline-info">
- <div
- class="contentText gr-diff"
- data-side="left"
- id="left-content-0"
- ></div>
+ <div class="contentText gr-diff" data-side="left"></div>
</td>
<td class="gr-diff lineNum right" data-value="43">
<button
@@ -1912,6 +1870,78 @@
assert.isTrue(container.classList.contains('displayLine'));
});
+ suite('binary diffs', () => {
+ test('render binary diff', async () => {
+ element.prefs = {
+ ...MINIMAL_PREFS,
+ };
+ element.diff = {
+ meta_a: {name: 'carrot.exe', content_type: 'binary', lines: 0},
+ meta_b: {name: 'carrot.exe', content_type: 'binary', lines: 0},
+ change_type: 'MODIFIED',
+ intraline_status: 'OK',
+ diff_header: [],
+ content: [],
+ binary: true,
+ };
+ await waitForEventOnce(element, 'render');
+
+ assert.shadowDom.equal(
+ element,
+ /* HTML */ `
+ <div class="diffContainer sideBySide">
+ <table class="selected-right" id="diffTable">
+ <colgroup>
+ <col class="blame gr-diff" />
+ <col width="48" />
+ <col width="48" />
+ <col />
+ </colgroup>
+ <tbody class="binary-diff gr-diff"></tbody>
+ <tbody class="binary-diff gr-diff">
+ <tr class="both diff-row gr-diff unified" tabindex="-1">
+ <td class="blame gr-diff" data-line-number="FILE"></td>
+ <td class="gr-diff left lineNum" data-value="FILE">
+ <button
+ aria-label="Add file comment"
+ class="gr-diff left lineNumButton"
+ data-value="FILE"
+ id="left-button-FILE"
+ tabindex="-1"
+ >
+ File
+ </button>
+ </td>
+ <td class="gr-diff lineNum right" data-value="FILE">
+ <button
+ aria-label="Add file comment"
+ class="gr-diff lineNumButton right"
+ data-value="FILE"
+ id="right-button-FILE"
+ tabindex="-1"
+ >
+ File
+ </button>
+ </td>
+ <td
+ class="both content file gr-diff no-intraline-info right"
+ >
+ <div>
+ <span> Difference in binary files </span>
+ </div>
+ <div class="thread-group" data-side="right">
+ <slot name="right-FILE"> </slot>
+ </div>
+ </td>
+ </tr>
+ </tbody>
+ </table>
+ </div>
+ `
+ );
+ });
+ });
+
suite('image diffs', () => {
let mockFile1: ImageInfo;
let mockFile2: ImageInfo;
@@ -1974,14 +2004,14 @@
<td class="blank gr-diff left lineNum"></td>
<td class="gr-diff left">
<img
- class="gr-diff"
+ class="gr-diff left"
src="data:image/bmp;base64,${mockFile1.body}"
/>
</td>
<td class="blank gr-diff lineNum right"></td>
<td class="gr-diff right">
<img
- class="gr-diff"
+ class="gr-diff right"
src="data:image/bmp;base64,${mockFile2.body}"
/>
</td>
@@ -2003,6 +2033,22 @@
</tbody>
`
);
+ const endpoint = queryAndAssert(element, 'tbody.endpoint');
+ assert.dom.equal(
+ endpoint,
+ /* HTML */ `
+ <tbody class="gr-diff endpoint">
+ <tr class="gr-diff">
+ <gr-endpoint-decorator class="gr-diff" name="image-diff">
+ <gr-endpoint-param class="gr-diff" name="baseImage">
+ </gr-endpoint-param>
+ <gr-endpoint-param class="gr-diff" name="revisionImage">
+ </gr-endpoint-param>
+ </gr-endpoint-decorator>
+ </tr>
+ </tbody>
+ `
+ );
});
test('renders image diffs with a different file name', async () => {
@@ -2081,7 +2127,7 @@
rightImage,
/* HTML */ `
<img
- class="gr-diff"
+ class="gr-diff right"
src="data:image/bmp;base64,${mockFile2.body}"
/>
`
@@ -2115,7 +2161,7 @@
leftImage,
/* HTML */ `
<img
- class="gr-diff"
+ class="gr-diff left"
src="data:image/bmp;base64,${mockFile1.body}"
/>
`
diff --git a/polygerrit-ui/app/models/views/repo.ts b/polygerrit-ui/app/models/views/repo.ts
index 02fd17d..ec65ca1 100644
--- a/polygerrit-ui/app/models/views/repo.ts
+++ b/polygerrit-ui/app/models/views/repo.ts
@@ -4,7 +4,7 @@
* SPDX-License-Identifier: Apache-2.0
*/
import {GerritView} from '../../services/router/router-model';
-import {RepoName} from '../../types/common';
+import {BranchName, RepoName} from '../../types/common';
import {encodeURL, getBaseUrl} from '../../utils/url-util';
import {define} from '../dependency';
import {Model} from '../model';
@@ -25,6 +25,14 @@
repo?: RepoName;
filter?: string | null;
offset?: number | string;
+ /**
+ * This is for creating a change from the URL and then redirecting to a file
+ * editing page.
+ */
+ createEdit?: {
+ branch: BranchName;
+ path: string;
+ };
}
export function createRepoUrl(state: Omit<RepoViewState, 'view'>) {
diff --git a/polygerrit-ui/app/services/shortcuts/shortcuts-service.ts b/polygerrit-ui/app/services/shortcuts/shortcuts-service.ts
index dac9b92..756c209 100644
--- a/polygerrit-ui/app/services/shortcuts/shortcuts-service.ts
+++ b/polygerrit-ui/app/services/shortcuts/shortcuts-service.ts
@@ -25,6 +25,7 @@
import {Finalizable} from '../registry';
import {UserModel} from '../../models/user/user-model';
import {define} from '../../models/dependency';
+import {isCharacterLetter, isUpperCase} from '../../utils/string-util';
export {Shortcut, ShortcutSection};
@@ -365,7 +366,10 @@
if (binding.combo === ComboKey.V) {
description.push('v');
}
- if (binding.modifiers?.includes(Modifier.SHIFT_KEY)) {
+ if (
+ binding.modifiers?.includes(Modifier.SHIFT_KEY) ||
+ (isCharacterLetter(binding.key) && isUpperCase(binding.key))
+ ) {
description.push('Shift');
}
if (binding.modifiers?.includes(Modifier.ALT_KEY)) {
@@ -377,6 +381,12 @@
if (binding.modifiers?.includes(Modifier.META_KEY)) {
description.push('Meta/Cmd');
}
- description.push(describeKey(binding.key));
+
+ let key = describeKey(binding.key);
+ if (isCharacterLetter(key)) {
+ key = key.toLowerCase();
+ }
+ description.push(key);
+
return description;
}
diff --git a/polygerrit-ui/app/services/shortcuts/shortcuts-service_test.ts b/polygerrit-ui/app/services/shortcuts/shortcuts-service_test.ts
index 0a5e0a4..164000a 100644
--- a/polygerrit-ui/app/services/shortcuts/shortcuts-service_test.ts
+++ b/polygerrit-ui/app/services/shortcuts/shortcuts-service_test.ts
@@ -32,7 +32,7 @@
test('getShortcut', () => {
assert.equal(service.getShortcut(Shortcut.NEXT_FILE), ']');
- assert.equal(service.getShortcut(Shortcut.TOGGLE_LEFT_PANE), 'A');
+ assert.equal(service.getShortcut(Shortcut.TOGGLE_LEFT_PANE), 'Shift+a');
});
suite('addShortcut()', () => {
diff --git a/polygerrit-ui/app/styles/gr-form-styles.ts b/polygerrit-ui/app/styles/gr-form-styles.ts
index cc89c3c..120b0bd 100644
--- a/polygerrit-ui/app/styles/gr-form-styles.ts
+++ b/polygerrit-ui/app/styles/gr-form-styles.ts
@@ -9,6 +9,7 @@
.gr-form-styles input {
background-color: var(--view-background-color);
color: var(--primary-text-color);
+ font: inherit;
}
.gr-form-styles select {
background-color: var(--select-background-color);
diff --git a/polygerrit-ui/app/types/common.ts b/polygerrit-ui/app/types/common.ts
index 93a3f2a..7370c96 100644
--- a/polygerrit-ui/app/types/common.ts
+++ b/polygerrit-ui/app/types/common.ts
@@ -750,8 +750,6 @@
type: string;
_name?: string;
_expectedType?: string;
- _width?: number;
- _height?: number;
}
/**
diff --git a/polygerrit-ui/app/utils/comment-util.ts b/polygerrit-ui/app/utils/comment-util.ts
index f531698..8af5beb 100644
--- a/polygerrit-ui/app/utils/comment-util.ts
+++ b/polygerrit-ui/app/utils/comment-util.ts
@@ -22,6 +22,7 @@
VotingRangeInfo,
FixSuggestionInfo,
FixId,
+ PatchSetNumber,
} from '../types/common';
import {CommentSide, SpecialFilePath} from '../constants/constants';
import {parseDate} from './date-util';
@@ -136,6 +137,20 @@
};
}
+export function createPatchsetLevelUnsavedDraft(
+ patchNum?: PatchSetNumber,
+ message?: string,
+ unresolved?: boolean
+): UnsavedInfo {
+ return {
+ patch_set: patchNum,
+ message,
+ unresolved,
+ path: SpecialFilePath.PATCHSET_LEVEL_COMMENTS,
+ __unsaved: true,
+ };
+}
+
export function createUnsavedReply(
replyingTo: CommentInfo,
message: string,
diff --git a/polygerrit-ui/app/utils/string-util.ts b/polygerrit-ui/app/utils/string-util.ts
index 659fc20..9955f68 100644
--- a/polygerrit-ui/app/utils/string-util.ts
+++ b/polygerrit-ui/app/utils/string-util.ts
@@ -18,6 +18,14 @@
return s.replace(/[^a-zA-Z]+/g, '');
}
+export function isCharacterLetter(ch: string): boolean {
+ return ch.length === 1 && ch.toLowerCase() !== ch.toUpperCase();
+}
+
+export function isUpperCase(ch: string): boolean {
+ return ch === ch.toUpperCase();
+}
+
export function ordinal(n?: number): string {
if (n === undefined) return '';
if (n % 10 === 1 && n % 100 !== 11) return `${n}st`;
diff --git a/tools/nongoogle.bzl b/tools/nongoogle.bzl
index cf50499..7f26ef3 100644
--- a/tools/nongoogle.bzl
+++ b/tools/nongoogle.bzl
@@ -67,18 +67,18 @@
sha1 = "cb2f351bf4463751201f43bb99865235d5ba07ca",
)
- SSHD_VERS = "2.9.1"
+ SSHD_VERS = "2.9.2"
maven_jar(
name = "sshd-osgi",
artifact = "org.apache.sshd:sshd-osgi:" + SSHD_VERS,
- sha1 = "9ed1a653da98a1aabe3ae092ee8310299718e914",
+ sha1 = "bac0415734519b2fe433fea196017acf7ed32660",
)
maven_jar(
name = "sshd-sftp",
artifact = "org.apache.sshd:sshd-sftp:" + SSHD_VERS,
- sha1 = "6d01cb8138e60e97e3de08e96cc5a094c8ce2cac",
+ sha1 = "7f9089c87b3b44f19998252fd3b68637e3322920",
)
maven_jar(
@@ -89,14 +89,14 @@
maven_jar(
name = "mina-core",
- artifact = "org.apache.mina:mina-core:2.0.21",
- sha1 = "e1a317689ecd438f54e863747e832f741ef8e092",
+ artifact = "org.apache.mina:mina-core:2.0.23",
+ sha1 = "391228b25d3a24434b205444cd262780a9ea61e7",
)
maven_jar(
name = "sshd-mina",
artifact = "org.apache.sshd:sshd-mina:" + SSHD_VERS,
- sha1 = "5ab797b99630bb0c3e9ebcd8a3a6cad46408a79a",
+ sha1 = "765dced3a2b4069bb0c550e18bda057bad8de26f",
)
maven_jar(
