opendev/gerrit
Code Issues Proposed changes

Fix stale dates in committer field

Browse Source 
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<PersonIdent> 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
This commit is contained in:
Shawn Pearce 2014-08-20 14:40:59 -07:00
parent 85a8f993ed
commit efcd3168a2
6 changed files with 16 additions and 19 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java
View File

@ -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);

2
gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/MergeAlways.java
View File

@ -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));
}

2
gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/MergeIfNecessary.java
View File

@ -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));
}

10
gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java
View File

@ -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<Change.Id, CodeReviewCommit> 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<PatchSetApproval> 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(

7
gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategy.java
View File

@ -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<PersonIdent> 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<PersonIdent> serverIdent, final ReviewDb db,
final ChangeControl.GenericFactory changeControlFactory,
final Repository repo, final RevWalk rw, final ObjectInserter inserter,
final RevFlag canMergeFlag, final Set<RevCommit> 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;

8
gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyFactory.java
View File

@ -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<PersonIdent> 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<PersonIdent> 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);