From a12f21c06c17f55f8674cc0768db3898ebadd35f Mon Sep 17 00:00:00 2001 From: Gal Paikin Date: Mon, 31 May 2021 17:25:32 +0200 Subject: Improve error message for a failed submission for FAST_FORWARD_ONLY There is a specific case for FAST_FORWARD_ONLY submit strategy that currently fails with NullPointerException: two changes that are independent but with the same topic and the same destination branch. It's impossible to perform this submission in a fast-forward way, hence this should in fact fail. However, this should fail without NullPointerException. This change creates a better error message. Release-Notes: Improve error message when failing a submission for FAST_FORWARD_ONLY projects, avoiding also an NPE on the error_log Change-Id: I58cb3391176606b257348cb81f1ff00b49b30bfa (cherry picked from commit ac1224ec2010040e134b154c3f3fc4d0b887ed56) --- .../gerrit/server/submit/CommitMergeStatus.java | 9 +++++- .../gerrit/server/submit/FastForwardOnly.java | 19 ++++++++++++ .../server/submit/SubmitStrategyListener.java | 1 + .../rest/change/SubmitByFastForwardIT.java | 34 ++++++++++++++++++++++ 4 files changed, 62 insertions(+), 1 deletion(-) diff --git a/java/com/google/gerrit/server/submit/CommitMergeStatus.java b/java/com/google/gerrit/server/submit/CommitMergeStatus.java index bf8b8408d0..4638bfa996 100644 --- a/java/com/google/gerrit/server/submit/CommitMergeStatus.java +++ b/java/com/google/gerrit/server/submit/CommitMergeStatus.java @@ -77,7 +77,14 @@ public enum CommitMergeStatus { EMPTY_COMMIT( "Change could not be merged because the commit is empty.\n" + "\n" - + "Project policy requires all commits to contain modifications to at least one file."); + + "Project policy requires all commits to contain modifications to at least one file."), + + FAST_FORWARD_INDEPENDENT_CHANGES( + "Change could not be merged because the submission has two independent changes " + + "with the same destination branch.\n" + + "\n" + + "Independent changes can't be submitted to the same destination branch with " + + "FAST_FORWARD_ONLY submit strategy"); private final String description; diff --git a/java/com/google/gerrit/server/submit/FastForwardOnly.java b/java/com/google/gerrit/server/submit/FastForwardOnly.java index 176b063690..8a30898290 100644 --- a/java/com/google/gerrit/server/submit/FastForwardOnly.java +++ b/java/com/google/gerrit/server/submit/FastForwardOnly.java @@ -14,11 +14,15 @@ package com.google.gerrit.server.submit; +import com.google.common.collect.ImmutableList; +import com.google.gerrit.entities.BranchNameKey; import com.google.gerrit.server.git.CodeReviewCommit; import com.google.gerrit.server.update.RepoContext; import java.util.ArrayList; import java.util.Collection; +import java.util.HashMap; import java.util.List; +import java.util.Map; public class FastForwardOnly extends SubmitStrategy { FastForwardOnly(SubmitStrategy.Arguments args) { @@ -28,6 +32,21 @@ public class FastForwardOnly extends SubmitStrategy { @Override public List buildOps(Collection toMerge) { List sorted = args.mergeUtil.reduceToMinimalMerge(args.mergeSorter, toMerge); + + Map branchToCommit = new HashMap<>(); + for (CodeReviewCommit codeReviewCommit : sorted) { + BranchNameKey branchNameKey = codeReviewCommit.change().getDest(); + CodeReviewCommit otherCommitInBranch = branchToCommit.get(branchNameKey); + if (otherCommitInBranch == null) { + branchToCommit.put(branchNameKey, codeReviewCommit); + } else { + // we found another change with the same destination branch. + codeReviewCommit.setStatusCode(CommitMergeStatus.FAST_FORWARD_INDEPENDENT_CHANGES); + otherCommitInBranch.setStatusCode(CommitMergeStatus.FAST_FORWARD_INDEPENDENT_CHANGES); + return ImmutableList.of(); + } + } + List ops = new ArrayList<>(sorted.size()); CodeReviewCommit newTipCommit = args.mergeUtil.getFirstFastForward(args.mergeTip.getInitialTip(), args.rw, sorted); diff --git a/java/com/google/gerrit/server/submit/SubmitStrategyListener.java b/java/com/google/gerrit/server/submit/SubmitStrategyListener.java index b533bebc7e..59c6b81162 100644 --- a/java/com/google/gerrit/server/submit/SubmitStrategyListener.java +++ b/java/com/google/gerrit/server/submit/SubmitStrategyListener.java @@ -134,6 +134,7 @@ public class SubmitStrategyListener implements BatchUpdateListener { case NOT_FAST_FORWARD: case EMPTY_COMMIT: case MISSING_DEPENDENCY: + case FAST_FORWARD_INDEPENDENT_CHANGES: // TODO(dborowitz): Reformat these messages to be more appropriate for // short problem descriptions. String message = s.getDescription(); diff --git a/javatests/com/google/gerrit/acceptance/rest/change/SubmitByFastForwardIT.java b/javatests/com/google/gerrit/acceptance/rest/change/SubmitByFastForwardIT.java index 66eb48c9ee..58e48e92cc 100644 --- a/javatests/com/google/gerrit/acceptance/rest/change/SubmitByFastForwardIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/change/SubmitByFastForwardIT.java @@ -20,6 +20,7 @@ import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.a import com.google.gerrit.acceptance.GitUtil; import com.google.gerrit.acceptance.PushOneCommit; +import com.google.gerrit.acceptance.config.GerritConfig; import com.google.gerrit.acceptance.testsuite.project.ProjectOperations; import com.google.gerrit.entities.Change; import com.google.gerrit.entities.PatchSet; @@ -106,6 +107,39 @@ public class SubmitByFastForwardIT extends AbstractSubmit { assertChangeMergedEvents(); } + @Test + @GerritConfig(name = "change.submitWholeTopic", value = "true") + public void submitTwoIndependentChangesWithFastForwardFail() throws Throwable { + RevCommit initialHead = projectOperations.project(project).getHead("master"); + PushOneCommit.Result change1 = createChange("subject1", "file1.txt", "content", "topic"); + + testRepo.reset(initialHead); + PushOneCommit.Result change2 = createChange("subject2", "file2.txt", "content", "topic"); + + approve(change1.getChangeId()); + approve(change2.getChangeId()); + + String fastForwardIndependentChangesError = + "Change could not be merged because the submission" + + " has two independent changes with the same destination branch. Independent changes can't " + + "be submitted to the same destination branch with FAST_FORWARD_ONLY submit strategy"; + + submitWithConflict( + change2.getChangeId(), + String.format( + "Failed to submit 2 changes due to the following problems:\n" + + "Change %d: %s\nChange %d: %s", + change1.getChange().getId().get(), + fastForwardIndependentChangesError, + change2.getChange().getId().get(), + fastForwardIndependentChangesError)); + + RevCommit updatedHead = projectOperations.project(project).getHead("master"); + assertThat(updatedHead.getId()).isEqualTo(initialHead.getId()); + assertRefUpdatedEvents(); + assertChangeMergedEvents(); + } + @Test public void submitFastForwardNotPossible_Conflict() throws Throwable { RevCommit initialHead = projectOperations.project(project).getHead("master"); -- cgit v1.2.3 From 38ea83f8744a4b0b3601db3a5ebca4e72bbfc83b Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Wed, 9 Jul 2025 23:58:21 +0100 Subject: Align delete refs to the rest of Gerrit Gerrit represents the removal of refs as the transition from a non-zero SHA to a zero-SHA, see one example in DeleteRef.deleteSingleRef() method. The ChangeEditUtil.deleteRef() and StarredChangesUtilNoteDbImpl.deleteRef() failed to set the new object id in the RefUpdate, causing a situation where other parts of the code are expecting a non-null SHA1 and therefore causing potential NPEs. Align the removal of refs to the rest of Gerrit and allow more resilience in other parts of the code and in plugins, such as the global-refdb. Bug: Issue 430336832 Forward-Compatible: checked Release-Notes: Fix removal of edits that failed when used with global-refdb Change-Id: I0602aa007ed5d5d3341eca6160ce631e7bae1c04 (cherry picked from commit 9469ddfdfaf00c707a9e4854a91b3c39a7df5d65) --- java/com/google/gerrit/server/StarredChangesUtil.java | 1 + java/com/google/gerrit/server/edit/ChangeEditUtil.java | 1 + 2 files changed, 2 insertions(+) diff --git a/java/com/google/gerrit/server/StarredChangesUtil.java b/java/com/google/gerrit/server/StarredChangesUtil.java index 904490cb10..fd53586a69 100644 --- a/java/com/google/gerrit/server/StarredChangesUtil.java +++ b/java/com/google/gerrit/server/StarredChangesUtil.java @@ -514,6 +514,7 @@ public class StarredChangesUtil { RefUpdate u = repo.updateRef(refName); u.setForceUpdate(true); u.setExpectedOldObjectId(oldObjectId); + u.setNewObjectId(ObjectId.zeroId()); u.setRefLogIdent(serverIdent.get()); u.setRefLogMessage("Unstar change", true); RefUpdate.Result result = u.delete(); diff --git a/java/com/google/gerrit/server/edit/ChangeEditUtil.java b/java/com/google/gerrit/server/edit/ChangeEditUtil.java index 710916e62f..56f9643e49 100644 --- a/java/com/google/gerrit/server/edit/ChangeEditUtil.java +++ b/java/com/google/gerrit/server/edit/ChangeEditUtil.java @@ -245,6 +245,7 @@ public class ChangeEditUtil { String refName = edit.getRefName(); RefUpdate ru = repo.updateRef(refName, true); ru.setExpectedOldObjectId(edit.getEditCommit()); + ru.setNewObjectId(ObjectId.zeroId()); ru.setForceUpdate(true); RefUpdate.Result result = ru.delete(); switch (result) { -- cgit v1.2.3