Merge "Add an interface to allow executing pending draft updates together"
diff --git a/Documentation/images/generated-suggested-edit-added.png b/Documentation/images/generated-suggested-edit-added.png
new file mode 100644
index 0000000..37300c3
--- /dev/null
+++ b/Documentation/images/generated-suggested-edit-added.png
Binary files differ
diff --git a/Documentation/images/generated-suggested-edit-preview.png b/Documentation/images/generated-suggested-edit-preview.png
new file mode 100644
index 0000000..9ca82f1
--- /dev/null
+++ b/Documentation/images/generated-suggested-edit-preview.png
Binary files differ
diff --git a/Documentation/metrics.txt b/Documentation/metrics.txt
index a560b84..7dab1ad 100644
--- a/Documentation/metrics.txt
+++ b/Documentation/metrics.txt
@@ -415,6 +415,13 @@
* `permissions/ref_filter/skip_filter_count`: Rate of ref filter operations
where we skip full evaluation because the user can read all refs
+=== Validation
+
+* `validation/file_count`: Track number of files per change during commit
+ validation, if it exceeds the FILE_COUNT_WARNING_THRESHOLD threshold.
+** `file_count`: number of files in the patchset
+** `host_repo`: host and repository of the change in the format 'host/repo'
+
=== Reviewer Suggestion
* `reviewer_suggestion/query_accounts`: Latency for querying accounts for
diff --git a/Documentation/user-suggest-edits.txt b/Documentation/user-suggest-edits.txt
index 9c67358..3b2c83a 100644
--- a/Documentation/user-suggest-edits.txt
+++ b/Documentation/user-suggest-edits.txt
@@ -36,6 +36,21 @@
You can use copy to clipboard button to copy suggestion to clipboard and then you can paste it
in your editor.
+== Generate Suggestion
+
+Following UI needs to be activated by a plugin that implements SuggestionsProvider. Gerrit is providing just UI.
+
+** When a user types a comment, Gerrit queries a plugin for a code snippet. When there is a snippet, the user can see a preview of snippet under comment.
+
+image::images/generated-suggested-edit-preview.png["Generate Suggested Edit", align="center", width=400]
+
+** A user needs to click on "ADD SUGGESTION TO COMMENT" button if they want to use this suggestion. Otherwise the suggestion is never used.
+
+image::images/generated-suggested-edit-added.png["Added Generated Suggested Edit", align="center", width=400]
+
+** By clicking on "ADD SUGGESTION TO COMMENT" button, the suggestion is added to end of comment. The user can then edit the suggestion, if needed.
+
+
GERRIT
------
Part of link:index.html[Gerrit Code Review]
diff --git a/java/com/google/gerrit/server/git/validators/CommitValidators.java b/java/com/google/gerrit/server/git/validators/CommitValidators.java
index 6adaae2..45618e5 100644
--- a/java/com/google/gerrit/server/git/validators/CommitValidators.java
+++ b/java/com/google/gerrit/server/git/validators/CommitValidators.java
@@ -36,6 +36,10 @@
import com.google.gerrit.extensions.api.config.ConsistencyCheckInfo.ConsistencyProblemInfo;
import com.google.gerrit.extensions.registration.DynamicItem;
import com.google.gerrit.extensions.restapi.AuthException;
+import com.google.gerrit.metrics.Counter2;
+import com.google.gerrit.metrics.Description;
+import com.google.gerrit.metrics.Field;
+import com.google.gerrit.metrics.MetricMaker;
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.IdentifiedUser;
@@ -115,6 +119,7 @@
private final DiffOperations diffOperations;
private final Config config;
private final ChangeUtil changeUtil;
+ private final MetricMaker metricMaker;
@Inject
Factory(
@@ -131,7 +136,8 @@
ProjectCache projectCache,
ProjectConfig.Factory projectConfigFactory,
DiffOperations diffOperations,
- ChangeUtil changeUtil) {
+ ChangeUtil changeUtil,
+ MetricMaker metricMaker) {
this.gerritIdent = gerritIdent;
this.urlFormatter = urlFormatter;
this.config = config;
@@ -146,6 +152,7 @@
this.projectConfigFactory = projectConfigFactory;
this.diffOperations = diffOperations;
this.changeUtil = changeUtil;
+ this.metricMaker = metricMaker;
}
public CommitValidators forReceiveCommits(
@@ -166,7 +173,7 @@
.add(new ProjectStateValidationListener(projectState))
.add(new AmendedGerritMergeCommitValidationListener(perm, gerritIdent))
.add(new AuthorUploaderValidator(user, perm, urlFormatter.get()))
- .add(new FileCountValidator(config, urlFormatter.get(), diffOperations))
+ .add(new FileCountValidator(config, urlFormatter.get(), diffOperations, metricMaker))
.add(new CommitterUploaderValidator(user, perm, urlFormatter.get()))
.add(new SignedOffByValidator(user, perm, projectState))
.add(
@@ -198,7 +205,7 @@
.add(new ProjectStateValidationListener(projectState))
.add(new AmendedGerritMergeCommitValidationListener(perm, gerritIdent))
.add(new AuthorUploaderValidator(user, perm, urlFormatter.get()))
- .add(new FileCountValidator(config, urlFormatter.get(), diffOperations))
+ .add(new FileCountValidator(config, urlFormatter.get(), diffOperations, metricMaker))
.add(new SignedOffByValidator(user, perm, projectState))
.add(
new ChangeIdValidator(
@@ -445,10 +452,25 @@
private final int maxFileCount;
private final UrlFormatter urlFormatter;
private final DiffOperations diffOperations;
+ private final Counter2<Integer, String> metricCountManyFilesPerChange;
- FileCountValidator(Config config, UrlFormatter urlFormatter, DiffOperations diffOperations) {
+ FileCountValidator(
+ Config config,
+ UrlFormatter urlFormatter,
+ DiffOperations diffOperations,
+ MetricMaker metricMaker) {
this.urlFormatter = urlFormatter;
this.diffOperations = diffOperations;
+ this.metricCountManyFilesPerChange =
+ metricMaker.newCounter(
+ "validation/file_count",
+ new Description("Count commits with many files per change."),
+ Field.ofInteger("file_count", (meta, value) -> {})
+ .description("number of files in the patchset")
+ .build(),
+ Field.ofString("host_repo", (meta, value) -> {})
+ .description("host and repository of the change in the format 'host/repo'")
+ .build());
maxFileCount = config.getInt("change", null, "maxFiles", 100_000);
}
@@ -482,6 +504,9 @@
logger.atWarning().log(
"Warning: Change with %d files on host %s, project %s, ref %s",
changedFiles, host, project, refName);
+
+ this.metricCountManyFilesPerChange.increment(
+ Math.toIntExact(changedFiles), String.format("%s/%s", host, project));
}
} catch (DiffNotAvailableException e) {
// This happens e.g. for cherrypicks.
diff --git a/java/com/google/gerrit/server/patch/DiffFileSizeValidator.java b/java/com/google/gerrit/server/patch/DiffFileSizeValidator.java
index 4001ab4..6964d63 100644
--- a/java/com/google/gerrit/server/patch/DiffFileSizeValidator.java
+++ b/java/com/google/gerrit/server/patch/DiffFileSizeValidator.java
@@ -15,6 +15,7 @@
package com.google.gerrit.server.patch;
import com.google.common.annotations.VisibleForTesting;
+import com.google.gerrit.entities.Patch.PatchType;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.git.LargeObjectException;
import com.google.gerrit.server.patch.filediff.FileDiffOutput;
@@ -43,8 +44,14 @@
@Override
public void validate(FileDiffOutput fileDiff) throws LargeObjectException {
- if (maxFileSize <= 0) {
+ if (maxFileSize <= 0
+ || (fileDiff.patchType().isPresent()
+ && fileDiff.patchType().get().equals(PatchType.BINARY))) {
// Do not apply limits if the config is not set.
+ // Also do not check file size for binary files. For modified binary files, JGit skips the
+ // diff and returns no edits. On the API layer, we only set the DiffInfo.ContentEntry.skip
+ // parameter to the number of lines in the file and the front-end displays a "Difference in
+ // binary files" in the diff view.
return;
}
if (fileDiff.size() > maxFileSize) {
diff --git a/javatests/com/google/gerrit/server/patch/DiffValidatorsTest.java b/javatests/com/google/gerrit/server/patch/DiffValidatorsTest.java
index 9cc19f5..9f697fd 100644
--- a/javatests/com/google/gerrit/server/patch/DiffValidatorsTest.java
+++ b/javatests/com/google/gerrit/server/patch/DiffValidatorsTest.java
@@ -74,13 +74,25 @@
diffValidators.validate(fileDiff);
}
+ @Test
+ public void binaryFileSizeExceeded_notCheckedForFileSize() throws Exception {
+ diffFileSizeValidator.setMaxFileSize(1000);
+ int largeSize = 100000000;
+ FileDiffOutput fileDiff = createFakeFileDiffOutput(largeSize, PatchType.BINARY);
+ diffValidators.validate(fileDiff);
+ }
+
private FileDiffOutput createFakeFileDiffOutput(int largeSize) {
+ return createFakeFileDiffOutput(largeSize, PatchType.UNIFIED);
+ }
+
+ private FileDiffOutput createFakeFileDiffOutput(int largeSize, PatchType patchType) {
return FileDiffOutput.builder()
.oldCommitId(ObjectId.zeroId())
.newCommitId(ObjectId.zeroId())
.comparisonType(ComparisonType.againstRoot())
.changeType(ChangeType.ADDED)
- .patchType(Optional.of(PatchType.UNIFIED))
+ .patchType(Optional.of(patchType))
.oldPath(Optional.empty())
.newPath(Optional.of("f.txt"))
.oldMode(Optional.empty())
diff --git a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
index 1e91345..5dea952 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
@@ -76,6 +76,9 @@
import {formStyles} from '../../../styles/form-styles';
import {Interaction} from '../../../constants/reporting';
import {Suggestion} from '../../../api/suggestions';
+import {when} from 'lit/directives/when.js';
+import {getDocUrl} from '../../../utils/url-util';
+import {configModelToken} from '../../../models/config/config-model';
// visible for testing
export const AUTO_SAVE_DEBOUNCE_DELAY_MS = 2000;
@@ -211,7 +214,10 @@
generatedSuggestion?: Suggestion;
@state()
- generatedReplacementId?: string;
+ generatedSuggestionId?: string;
+
+ @state()
+ suggestionLoading = false;
@property({type: Boolean, attribute: 'show-patchset'})
showPatchset = false;
@@ -231,6 +237,8 @@
@state()
commentedText?: string;
+ @state() private docsBaseUrl = '';
+
private readonly restApiService = getAppContext().restApiService;
private readonly reporting = getAppContext().reportingService;
@@ -243,6 +251,8 @@
private readonly getPluginLoader = resolve(this, pluginLoaderToken);
+ private readonly getConfigModel = resolve(this, configModelToken);
+
private readonly flagsService = getAppContext().flagsService;
private readonly shortcuts = new ShortcutController(this);
@@ -338,6 +348,11 @@
this.autoSave();
}
);
+ subscribe(
+ this,
+ () => this.getConfigModel().docsBaseUrl$,
+ docsBaseUrl => (this.docsBaseUrl = docsBaseUrl)
+ );
if (this.flagsService.isEnabled(KnownExperimentId.ML_SUGGESTED_EDIT)) {
subscribe(
this,
@@ -550,6 +565,16 @@
color: var(--selected-foreground);
margin-right: var(--spacing-xl);
}
+ /* The basics of .loadingSpin are defined in shared styles. */
+ .loadingSpin {
+ width: calc(var(--line-height-normal) - 2px);
+ height: calc(var(--line-height-normal) - 2px);
+ display: inline-block;
+ vertical-align: top;
+ position: relative;
+ /* Making up for the 2px reduced height above. */
+ top: 1px;
+ }
`,
];
}
@@ -949,7 +974,7 @@
return html`<gr-suggestion-diff-preview
.showAddSuggestionButton=${true}
.suggestion=${this.generatedSuggestion?.replacement}
- .uuid=${this.generatedReplacementId}
+ .uuid=${this.generatedSuggestionId}
></gr-suggestion-diff-preview>`;
}
@@ -979,8 +1004,26 @@
);
}}
/>
- Generate Suggestion${numberOfSuggestions}
+ Generate Suggestion
+ ${when(
+ this.suggestionLoading,
+ () => html`<span class="loadingSpin"></span>`,
+ () => html`${numberOfSuggestions}`
+ )}
</label>
+ <a
+ href=${getDocUrl(
+ this.docsBaseUrl,
+ 'user-suggest-edits.html#_generate_suggestion'
+ )}
+ target="_blank"
+ rel="noopener noreferrer"
+ >
+ <gr-icon
+ icon="help"
+ title="About Generated Suggested Edits"
+ ></gr-icon>
+ </a>
</div>
`;
}
@@ -1005,10 +1048,11 @@
this.messageText.length === 0
)
return;
- this.generatedReplacementId = uuid();
+ this.generatedSuggestionId = uuid();
this.reporting.reportInteraction(Interaction.GENERATE_SUGGESTION_REQUEST, {
- uuid: this.generatedReplacementId,
+ uuid: this.generatedSuggestionId,
});
+ this.suggestionLoading = true;
const suggestionResponse = await suggestionsPlugins[0].provider.suggestCode(
{
prompt: this.messageText,
@@ -1019,11 +1063,12 @@
lineNumber: this.comment.line,
}
);
+ this.suggestionLoading = false;
// TODO(milutin): The suggestionResponse can contain multiple suggestion
// options. We pick the first one for now. In future we shouldn't ignore
// other suggestions.
this.reporting.reportInteraction(Interaction.GENERATE_SUGGESTION_RESPONSE, {
- uuid: this.generatedReplacementId,
+ uuid: this.generatedSuggestionId,
response: suggestionResponse.responseCode,
numSuggestions: suggestionResponse.suggestions.length,
hasNewRange: suggestionResponse.suggestions?.[0]?.newRange !== undefined,
@@ -1130,7 +1175,7 @@
if (
changed.has('changeNum') ||
changed.has('comment') ||
- changed.has('generatedReplacement')
+ changed.has('generatedSuggestion')
) {
if (
!this.changeNum ||
diff --git a/resources/com/google/gerrit/server/mail/ChangeHeader.soy b/resources/com/google/gerrit/server/mail/ChangeHeader.soy
index 12b68b6..708d046 100644
--- a/resources/com/google/gerrit/server/mail/ChangeHeader.soy
+++ b/resources/com/google/gerrit/server/mail/ChangeHeader.soy
@@ -17,7 +17,7 @@
{namespace com.google.gerrit.server.mail.template.ChangeHeader}
{template ChangeHeader kind="text"}
- {@param attentionSet: list<string>|null}
+ {@param? attentionSet: list<string>|null}
{if $attentionSet and length($attentionSet) > 0}
Attention is currently required from:{sp}
{for $attentionSetUser, $index in $attentionSet}
diff --git a/resources/com/google/gerrit/server/mail/ChangeHeaderHtml.soy b/resources/com/google/gerrit/server/mail/ChangeHeaderHtml.soy
index e17e021..d687ce9 100644
--- a/resources/com/google/gerrit/server/mail/ChangeHeaderHtml.soy
+++ b/resources/com/google/gerrit/server/mail/ChangeHeaderHtml.soy
@@ -18,7 +18,7 @@
{namespace com.google.gerrit.server.mail.template.ChangeHeaderHtml}
{template ChangeHeaderHtml}
- {@param attentionSet: list<string>|null}
+ {@param? attentionSet: list<string>|null}
{if $attentionSet and length($attentionSet) > 0}
<p> Attention is currently required from:{sp}
{for $attentionSetUser, $index in $attentionSet}
