diff --git a/java/com/google/gerrit/server/account/AccountsUpdate.java b/java/com/google/gerrit/server/account/AccountsUpdate.java index 730bb9515e..2e10af4437 100644 --- a/java/com/google/gerrit/server/account/AccountsUpdate.java +++ b/java/com/google/gerrit/server/account/AccountsUpdate.java @@ -31,6 +31,7 @@ import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.MetaDataUpdate; import com.google.gerrit.server.index.change.ReindexAfterRefUpdate; import com.google.gerrit.server.mail.send.OutgoingEmailValidator; +import com.google.gerrit.server.update.RefUpdateUtil; import com.google.gerrit.server.update.RetryHelper; import com.google.gwtorm.server.OrmDuplicateKeyException; import com.google.gwtorm.server.OrmException; @@ -42,6 +43,7 @@ import java.util.List; import java.util.Optional; import java.util.function.Consumer; import org.eclipse.jgit.errors.ConfigInvalidException; +import org.eclipse.jgit.lib.BatchRefUpdate; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.PersonIdent; import org.eclipse.jgit.lib.Ref; @@ -111,8 +113,8 @@ public class AccountsUpdate { private final GitReferenceUpdated gitRefUpdated; private final AllUsersName allUsersName; private final OutgoingEmailValidator emailValidator; - private final Provider serverIdent; - private final Provider metaDataUpdateServerFactory; + private final Provider serverIdentProvider; + private final Provider metaDataUpdateInternalFactory; private final RetryHelper retryHelper; @Inject @@ -121,29 +123,30 @@ public class AccountsUpdate { GitReferenceUpdated gitRefUpdated, AllUsersName allUsersName, OutgoingEmailValidator emailValidator, - @GerritPersonIdent Provider serverIdent, - Provider metaDataUpdateServerFactory, + @GerritPersonIdent Provider serverIdentProvider, + Provider metaDataUpdateInternalFactory, RetryHelper retryHelper) { this.repoManager = repoManager; this.gitRefUpdated = gitRefUpdated; this.allUsersName = allUsersName; this.emailValidator = emailValidator; - this.serverIdent = serverIdent; - this.metaDataUpdateServerFactory = metaDataUpdateServerFactory; + this.serverIdentProvider = serverIdentProvider; + this.metaDataUpdateInternalFactory = metaDataUpdateInternalFactory; this.retryHelper = retryHelper; } public AccountsUpdate create() { - PersonIdent i = serverIdent.get(); + PersonIdent serverIdent = serverIdentProvider.get(); return new AccountsUpdate( repoManager, gitRefUpdated, null, allUsersName, emailValidator, - i, - () -> metaDataUpdateServerFactory.get().create(allUsersName), - retryHelper); + metaDataUpdateInternalFactory, + retryHelper, + serverIdent, + serverIdent); } } @@ -159,9 +162,9 @@ public class AccountsUpdate { private final GitReferenceUpdated gitRefUpdated; private final AllUsersName allUsersName; private final OutgoingEmailValidator emailValidator; - private final Provider serverIdent; + private final Provider serverIdentProvider; private final Provider identifiedUser; - private final Provider metaDataUpdateUserFactory; + private final Provider metaDataUpdateInternalFactory; private final RetryHelper retryHelper; @Inject @@ -170,32 +173,34 @@ public class AccountsUpdate { GitReferenceUpdated gitRefUpdated, AllUsersName allUsersName, OutgoingEmailValidator emailValidator, - @GerritPersonIdent Provider serverIdent, + @GerritPersonIdent Provider serverIdentProvider, Provider identifiedUser, - Provider metaDataUpdateUserFactory, + Provider metaDataUpdateInternalFactory, RetryHelper retryHelper) { this.repoManager = repoManager; this.gitRefUpdated = gitRefUpdated; this.allUsersName = allUsersName; - this.serverIdent = serverIdent; + this.serverIdentProvider = serverIdentProvider; this.emailValidator = emailValidator; this.identifiedUser = identifiedUser; - this.metaDataUpdateUserFactory = metaDataUpdateUserFactory; + this.metaDataUpdateInternalFactory = metaDataUpdateInternalFactory; this.retryHelper = retryHelper; } public AccountsUpdate create() { IdentifiedUser user = identifiedUser.get(); - PersonIdent i = serverIdent.get(); + PersonIdent serverIdent = serverIdentProvider.get(); + PersonIdent userIdent = createPersonIdent(serverIdent, user); return new AccountsUpdate( repoManager, gitRefUpdated, user, allUsersName, emailValidator, - createPersonIdent(i, user), - () -> metaDataUpdateUserFactory.get().create(allUsersName), - retryHelper); + metaDataUpdateInternalFactory, + retryHelper, + serverIdent, + userIdent); } private PersonIdent createPersonIdent(PersonIdent ident, IdentifiedUser user) { @@ -208,9 +213,10 @@ public class AccountsUpdate { @Nullable private final IdentifiedUser currentUser; private final AllUsersName allUsersName; private final OutgoingEmailValidator emailValidator; - private final PersonIdent committerIdent; - private final MetaDataUpdateFactory metaDataUpdateFactory; + private final Provider metaDataUpdateInternalFactory; private final RetryHelper retryHelper; + private final PersonIdent committerIdent; + private final PersonIdent authorIdent; private final Runnable afterReadRevision; private AccountsUpdate( @@ -219,18 +225,20 @@ public class AccountsUpdate { @Nullable IdentifiedUser currentUser, AllUsersName allUsersName, OutgoingEmailValidator emailValidator, + Provider metaDataUpdateInternalFactory, + RetryHelper retryHelper, PersonIdent committerIdent, - MetaDataUpdateFactory metaDataUpdateFactory, - RetryHelper retryHelper) { + PersonIdent authorIdent) { this( repoManager, gitRefUpdated, currentUser, allUsersName, emailValidator, - committerIdent, - metaDataUpdateFactory, + metaDataUpdateInternalFactory, retryHelper, + committerIdent, + authorIdent, Runnables.doNothing()); } @@ -241,18 +249,21 @@ public class AccountsUpdate { @Nullable IdentifiedUser currentUser, AllUsersName allUsersName, OutgoingEmailValidator emailValidator, - PersonIdent committerIdent, - MetaDataUpdateFactory metaDataUpdateFactory, + Provider metaDataUpdateInternalFactory, RetryHelper retryHelper, + PersonIdent committerIdent, + PersonIdent authorIdent, Runnable afterReadRevision) { this.repoManager = checkNotNull(repoManager, "repoManager"); this.gitRefUpdated = checkNotNull(gitRefUpdated, "gitRefUpdated"); this.currentUser = currentUser; this.allUsersName = checkNotNull(allUsersName, "allUsersName"); this.emailValidator = checkNotNull(emailValidator, "emailValidator"); - this.committerIdent = checkNotNull(committerIdent, "committerIdent"); - this.metaDataUpdateFactory = checkNotNull(metaDataUpdateFactory, "metaDataUpdateFactory"); + this.metaDataUpdateInternalFactory = + checkNotNull(metaDataUpdateInternalFactory, "metaDataUpdateInternalFactory"); this.retryHelper = checkNotNull(retryHelper, "retryHelper"); + this.committerIdent = checkNotNull(committerIdent, "committerIdent"); + this.authorIdent = checkNotNull(authorIdent, "authorIdent"); this.afterReadRevision = afterReadRevision; } @@ -286,8 +297,8 @@ public class AccountsUpdate { public Account insert(Account.Id accountId, AccountUpdater updater) throws OrmException, IOException, ConfigInvalidException { return updateAccount( - () -> { - AccountConfig accountConfig = read(accountId); + r -> { + AccountConfig accountConfig = read(r, accountId); Account account = accountConfig.getNewAccount(); InternalAccountUpdate.Builder updateBuilder = InternalAccountUpdate.builder(); updater.update(account, updateBuilder); @@ -332,8 +343,8 @@ public class AccountsUpdate { public Account update(Account.Id accountId, AccountUpdater updater) throws OrmException, IOException, ConfigInvalidException { return updateAccount( - () -> { - AccountConfig accountConfig = read(accountId); + r -> { + AccountConfig accountConfig = read(r, accountId); Optional account = accountConfig.getLoadedAccount(); if (!account.isPresent()) { return null; @@ -383,7 +394,7 @@ public class AccountsUpdate { private void deleteUserBranch(Account.Id accountId) throws IOException { try (Repository repo = repoManager.openRepository(allUsersName)) { - deleteUserBranch(repo, allUsersName, gitRefUpdated, currentUser, committerIdent, accountId); + deleteUserBranch(repo, allUsersName, gitRefUpdated, currentUser, authorIdent, accountId); } } @@ -414,66 +425,84 @@ public class AccountsUpdate { gitRefUpdated.fire(project, ru, user != null ? user.getAccount() : null); } - private AccountConfig read(Account.Id accountId) throws IOException, ConfigInvalidException { - try (Repository repo = repoManager.openRepository(allUsersName)) { - AccountConfig accountConfig = new AccountConfig(emailValidator, accountId); - accountConfig.load(repo); + private AccountConfig read(Repository allUsersRepo, Account.Id accountId) + throws IOException, ConfigInvalidException { + AccountConfig accountConfig = new AccountConfig(emailValidator, accountId); + accountConfig.load(allUsersRepo); - afterReadRevision.run(); + afterReadRevision.run(); - return accountConfig; - } + return accountConfig; } private Account updateAccount(AccountUpdate accountUpdate) throws IOException, ConfigInvalidException, OrmException { return retryHelper.execute( () -> { - UpdatedAccount updatedAccount = accountUpdate.update(); - if (updatedAccount == null) { - return null; - } + try (Repository allUsersRepo = repoManager.openRepository(allUsersName)) { + UpdatedAccount updatedAccount = accountUpdate.update(allUsersRepo); + if (updatedAccount == null) { + return null; + } - commit(updatedAccount); - return updatedAccount.getAccount(); + commit(allUsersRepo, updatedAccount); + return updatedAccount.getAccount(); + } }); } - private void commit(UpdatedAccount updatedAccount) throws IOException { + private void commit(Repository allUsersRepo, UpdatedAccount updatedAccount) throws IOException { + BatchRefUpdate batchRefUpdate = allUsersRepo.getRefDatabase().newBatchUpdate(); if (updatedAccount.isCreated()) { - commitNew(updatedAccount.getAccountConfig()); + commitNewAccountConfig(allUsersRepo, batchRefUpdate, updatedAccount.getAccountConfig()); } else { - commit(updatedAccount.getAccountConfig()); + commitAccountConfig(allUsersRepo, batchRefUpdate, updatedAccount.getAccountConfig()); } + RefUpdateUtil.executeChecked(batchRefUpdate, allUsersRepo); + gitRefUpdated.fire( + allUsersName, batchRefUpdate, currentUser != null ? currentUser.getAccount() : null); } - private void commitNew(AccountConfig accountConfig) throws IOException { + private void commitNewAccountConfig( + Repository allUsersRepo, BatchRefUpdate batchRefUpdate, AccountConfig accountConfig) + throws IOException { // When creating a new account we must allow empty commits so that the user branch gets created // with an empty commit when no account properties are set and hence no 'account.config' file // will be created. - commit(accountConfig, true); + commitAccountConfig(allUsersRepo, batchRefUpdate, accountConfig, true); } - private void commit(AccountConfig accountConfig) throws IOException { - commit(accountConfig, false); + private void commitAccountConfig( + Repository allUsersRepo, BatchRefUpdate batchRefUpdate, AccountConfig accountConfig) + throws IOException { + commitAccountConfig(allUsersRepo, batchRefUpdate, accountConfig, false); } - private void commit(AccountConfig accountConfig, boolean allowEmptyCommit) throws IOException { - try (MetaDataUpdate md = metaDataUpdateFactory.create()) { + private void commitAccountConfig( + Repository allUsersRepo, + BatchRefUpdate batchRefUpdate, + AccountConfig accountConfig, + boolean allowEmptyCommit) + throws IOException { + try (MetaDataUpdate md = createMetaDataUpdate(allUsersRepo, batchRefUpdate)) { md.setAllowEmpty(allowEmptyCommit); accountConfig.commit(md); } } - @VisibleForTesting - @FunctionalInterface - public static interface MetaDataUpdateFactory { - MetaDataUpdate create() throws IOException; + private MetaDataUpdate createMetaDataUpdate( + Repository allUsersRepo, BatchRefUpdate batchRefUpdate) { + MetaDataUpdate metaDataUpdate = + metaDataUpdateInternalFactory.get().create(allUsersName, allUsersRepo, batchRefUpdate); + metaDataUpdate.getCommitBuilder().setCommitter(committerIdent); + metaDataUpdate.getCommitBuilder().setAuthor(authorIdent); + return metaDataUpdate; } @FunctionalInterface private static interface AccountUpdate { - UpdatedAccount update() throws IOException, ConfigInvalidException, OrmException; + UpdatedAccount update(Repository allUsersRepo) + throws IOException, ConfigInvalidException, OrmException; } private static class UpdatedAccount { diff --git a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java index e296747e04..0d3a8310a2 100644 --- a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java +++ b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java @@ -95,6 +95,7 @@ import com.google.gerrit.server.account.externalids.ExternalIds; import com.google.gerrit.server.account.externalids.ExternalIdsUpdate; import com.google.gerrit.server.extensions.events.GitReferenceUpdated; import com.google.gerrit.server.git.LockFailureException; +import com.google.gerrit.server.git.MetaDataUpdate; import com.google.gerrit.server.git.ProjectConfig; import com.google.gerrit.server.index.account.AccountIndexer; import com.google.gerrit.server.index.account.StalenessChecker; @@ -195,6 +196,8 @@ public class AccountIT extends AbstractDaemonTest { @Inject private RetryHelper.Metrics retryMetrics; + @Inject private Provider metaDataUpdateInternalFactory; + @Inject @Named("accounts") private LoadingCache> accountsCache; @@ -1899,6 +1902,7 @@ public class AccountIT extends AbstractDaemonTest { String status = "happy"; String fullName = "Foo"; AtomicBoolean doneBgUpdate = new AtomicBoolean(false); + PersonIdent ident = serverIdent.get(); AccountsUpdate update = new AccountsUpdate( repoManager, @@ -1906,8 +1910,7 @@ public class AccountIT extends AbstractDaemonTest { null, allUsers, emailValidator, - serverIdent.get(), - () -> metaDataUpdateFactory.create(allUsers), + metaDataUpdateInternalFactory, new RetryHelper( cfg, retryMetrics, @@ -1915,6 +1918,8 @@ public class AccountIT extends AbstractDaemonTest { null, null, r -> r.withBlockStrategy(noSleepBlockStrategy)), + ident, + ident, () -> { if (!doneBgUpdate.getAndSet(true)) { try { @@ -1945,6 +1950,7 @@ public class AccountIT extends AbstractDaemonTest { List status = ImmutableList.of("foo", "bar", "baz"); String fullName = "Foo"; AtomicInteger bgCounter = new AtomicInteger(0); + PersonIdent ident = serverIdent.get(); AccountsUpdate update = new AccountsUpdate( repoManager, @@ -1952,8 +1958,7 @@ public class AccountIT extends AbstractDaemonTest { null, allUsers, emailValidator, - serverIdent.get(), - () -> metaDataUpdateFactory.create(allUsers), + metaDataUpdateInternalFactory, new RetryHelper( cfg, retryMetrics, @@ -1963,6 +1968,8 @@ public class AccountIT extends AbstractDaemonTest { r -> r.withStopStrategy(StopStrategies.stopAfterAttempt(status.size())) .withBlockStrategy(noSleepBlockStrategy)), + ident, + ident, () -> { try { accountsUpdate