From efcd3168a2f2053597ac467d53f254500de6c1ed Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Wed, 20 Aug 2014 14:40:59 -0700 Subject: [PATCH] Fix stale dates in committer field The server's PersonIdent cannot be held by a singleton such as SubmitStrategyFactory. Holding it would retain the timestamp of the first merge in this process, with the time forever stuck in the past. Instead use Provider so Guice can defer creation of the server's identity until it is required to write a new commit. This allows the current date to be injected into a newly created PersonIdent, keeping the committer field current. Bug: issue 2773 Change-Id: I9e5edd41e42a702ed8541cfef57660204a8c5b57 --- .../google/gerrit/server/git/strategy/CherryPick.java | 6 +++--- .../google/gerrit/server/git/strategy/MergeAlways.java | 2 +- .../gerrit/server/git/strategy/MergeIfNecessary.java | 2 +- .../gerrit/server/git/strategy/RebaseIfNecessary.java | 10 +++------- .../gerrit/server/git/strategy/SubmitStrategy.java | 7 ++++--- .../server/git/strategy/SubmitStrategyFactory.java | 8 ++++---- 6 files changed, 16 insertions(+), 19 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java index 8568b1e527..fbbed67859 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java @@ -108,7 +108,7 @@ public class CherryPick extends SubmitStrategy { mergeTip = n; } else { mergeTip = - args.mergeUtil.mergeOneCommit(args.myIdent, args.repo, + args.mergeUtil.mergeOneCommit(args.serverIdent.get(), args.repo, args.rw, args.inserter, args.canMergeFlag, args.destBranch, mergeTip, n); } @@ -147,10 +147,10 @@ public class CherryPick extends SubmitStrategy { cherryPickUser = args.identifiedUserFactory.create(submitAudit.getAccountId()); cherryPickCommitterIdent = cherryPickUser.newCommitterIdent( - submitAudit.getGranted(), args.myIdent.getTimeZone()); + submitAudit.getGranted(), args.serverIdent.get().getTimeZone()); } else { cherryPickUser = args.identifiedUserFactory.create(n.change().getOwner()); - cherryPickCommitterIdent = args.myIdent; + cherryPickCommitterIdent = args.serverIdent.get(); } final String cherryPickCmtMsg = args.mergeUtil.createCherryPickCommitMessage(n); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/MergeAlways.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/MergeAlways.java index dc0d72192e..9023623ef0 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/MergeAlways.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/MergeAlways.java @@ -38,7 +38,7 @@ public class MergeAlways extends SubmitStrategy { } while (!toMerge.isEmpty()) { mergeTip = - args.mergeUtil.mergeOneCommit(args.myIdent, args.repo, args.rw, + args.mergeUtil.mergeOneCommit(args.serverIdent.get(), args.repo, args.rw, args.inserter, args.canMergeFlag, args.destBranch, mergeTip, toMerge.remove(0)); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/MergeIfNecessary.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/MergeIfNecessary.java index 601c6f8ccc..b84586f682 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/MergeIfNecessary.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/MergeIfNecessary.java @@ -43,7 +43,7 @@ public class MergeIfNecessary extends SubmitStrategy { // For every other commit do a pair-wise merge. while (!toMerge.isEmpty()) { mergeTip = - args.mergeUtil.mergeOneCommit(args.myIdent, args.repo, args.rw, + args.mergeUtil.mergeOneCommit(args.serverIdent.get(), args.repo, args.rw, args.inserter, args.canMergeFlag, args.destBranch, mergeTip, toMerge.remove(0)); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java index c38f5f21f1..130d170dc5 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java @@ -32,7 +32,6 @@ import com.google.gerrit.server.project.NoSuchChangeException; import com.google.gwtorm.server.OrmException; import org.eclipse.jgit.lib.ObjectId; -import org.eclipse.jgit.lib.PersonIdent; import java.io.IOException; import java.util.HashMap; @@ -43,17 +42,14 @@ public class RebaseIfNecessary extends SubmitStrategy { private final PatchSetInfoFactory patchSetInfoFactory; private final RebaseChange rebaseChange; private final Map newCommits; - private final PersonIdent committerIdent; RebaseIfNecessary(SubmitStrategy.Arguments args, PatchSetInfoFactory patchSetInfoFactory, - RebaseChange rebaseChange, - PersonIdent committerIdent) { + RebaseChange rebaseChange) { super(args); this.patchSetInfoFactory = patchSetInfoFactory; this.rebaseChange = rebaseChange; this.newCommits = new HashMap<>(); - this.committerIdent = committerIdent; } @Override @@ -91,7 +87,7 @@ public class RebaseIfNecessary extends SubmitStrategy { final PatchSet newPatchSet = rebaseChange.rebase(args.repo, args.rw, args.inserter, n.getPatchsetId(), n.change(), uploader, - newMergeTip, args.mergeUtil, committerIdent, + newMergeTip, args.mergeUtil, args.serverIdent.get(), false, false, ValidatePolicy.NONE); List approvals = Lists.newArrayList(); @@ -133,7 +129,7 @@ public class RebaseIfNecessary extends SubmitStrategy { newMergeTip = n; } else { newMergeTip = args.mergeUtil.mergeOneCommit( - args.myIdent, args.repo, args.rw, args.inserter, + args.serverIdent.get(), args.repo, args.rw, args.inserter, args.canMergeFlag, args.destBranch, newMergeTip, n); } final PatchSetApproval submitApproval = args.mergeUtil.markCleanMerges( diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategy.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategy.java index aa080857dc..a864b6c380 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategy.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategy.java @@ -27,6 +27,7 @@ import com.google.gerrit.server.git.MergeSorter; import com.google.gerrit.server.git.MergeUtil; import com.google.gerrit.server.index.ChangeIndexer; import com.google.gerrit.server.project.ChangeControl; +import com.google.inject.Provider; import org.eclipse.jgit.lib.ObjectInserter; import org.eclipse.jgit.lib.PersonIdent; @@ -52,7 +53,7 @@ public abstract class SubmitStrategy { static class Arguments { protected final IdentifiedUser.GenericFactory identifiedUserFactory; - protected final PersonIdent myIdent; + protected final Provider serverIdent; protected final ReviewDb db; protected final ChangeControl.GenericFactory changeControlFactory; @@ -68,14 +69,14 @@ public abstract class SubmitStrategy { protected final MergeSorter mergeSorter; Arguments(final IdentifiedUser.GenericFactory identifiedUserFactory, - final PersonIdent myIdent, final ReviewDb db, + final Provider serverIdent, final ReviewDb db, final ChangeControl.GenericFactory changeControlFactory, final Repository repo, final RevWalk rw, final ObjectInserter inserter, final RevFlag canMergeFlag, final Set alreadyAccepted, final Branch.NameKey destBranch, final ApprovalsUtil approvalsUtil, final MergeUtil mergeUtil, final ChangeIndexer indexer) { this.identifiedUserFactory = identifiedUserFactory; - this.myIdent = myIdent; + this.serverIdent = serverIdent; this.db = db; this.changeControlFactory = changeControlFactory; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyFactory.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyFactory.java index d427b8d955..091523bdc5 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyFactory.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyFactory.java @@ -31,6 +31,7 @@ import com.google.gerrit.server.project.NoSuchProjectException; import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.project.ProjectState; import com.google.inject.Inject; +import com.google.inject.Provider; import com.google.inject.Singleton; import org.eclipse.jgit.lib.ObjectInserter; @@ -51,7 +52,7 @@ public class SubmitStrategyFactory { .getLogger(SubmitStrategyFactory.class); private final IdentifiedUser.GenericFactory identifiedUserFactory; - private final PersonIdent myIdent; + private final Provider myIdent; private final ChangeControl.GenericFactory changeControlFactory; private final PatchSetInfoFactory patchSetInfoFactory; private final GitReferenceUpdated gitRefUpdated; @@ -64,7 +65,7 @@ public class SubmitStrategyFactory { @Inject SubmitStrategyFactory( final IdentifiedUser.GenericFactory identifiedUserFactory, - @GerritPersonIdent final PersonIdent myIdent, + @GerritPersonIdent Provider myIdent, final ChangeControl.GenericFactory changeControlFactory, final PatchSetInfoFactory patchSetInfoFactory, final GitReferenceUpdated gitRefUpdated, final RebaseChange rebaseChange, @@ -105,8 +106,7 @@ public class SubmitStrategyFactory { case MERGE_IF_NECESSARY: return new MergeIfNecessary(args); case REBASE_IF_NECESSARY: - return new RebaseIfNecessary( - args, patchSetInfoFactory, rebaseChange, myIdent); + return new RebaseIfNecessary(args, patchSetInfoFactory, rebaseChange); default: final String errorMsg = "No submit strategy for: " + submitType; log.error(errorMsg);