From f6de626cd5843f3153a367d816160f4dd191b64d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Pfeuffer?= Date: Tue, 28 Sep 2021 09:54:14 +0200 Subject: [PATCH] Fix edge cases in mirror command (#1812) - The clone of the repository failed, if no master branch had been mirrored, because this was what the HEAD was set to, and jgit fails with an exception if the HEAD branch does not exist. - The HEAD branch had to be corrected to an existing branch, because otherwise a rejected 'master' branch could not be deleted and was synchronized in a second sync if the working directory had been cached. --- .../changelog/mirror_with_workdir_cache.yaml | 2 + .../scm/repository/spi/GitMirrorCommand.java | 60 ++++++++++++++++++- .../repository/spi/GitMirrorCommandTest.java | 40 ++++++++++++- 3 files changed, 96 insertions(+), 6 deletions(-) create mode 100644 gradle/changelog/mirror_with_workdir_cache.yaml diff --git a/gradle/changelog/mirror_with_workdir_cache.yaml b/gradle/changelog/mirror_with_workdir_cache.yaml new file mode 100644 index 0000000000..f260803569 --- /dev/null +++ b/gradle/changelog/mirror_with_workdir_cache.yaml @@ -0,0 +1,2 @@ +- type: Fixed + description: Edge cases in mirror command with cached workdirs ([#1812](https://github.com/scm-manager/scm-manager/pull/1812)) diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitMirrorCommand.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitMirrorCommand.java index 278811ca8b..fe115f39a9 100644 --- a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitMirrorCommand.java +++ b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitMirrorCommand.java @@ -34,6 +34,7 @@ import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.lib.RefDatabase; import org.eclipse.jgit.lib.RefUpdate; +import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevObject; import org.eclipse.jgit.revwalk.RevTag; @@ -48,6 +49,7 @@ import org.slf4j.LoggerFactory; import sonia.scm.repository.Changeset; import sonia.scm.repository.GitChangesetConverter; import sonia.scm.repository.GitChangesetConverterFactory; +import sonia.scm.repository.GitHeadModifier; import sonia.scm.repository.GitWorkingCopyFactory; import sonia.scm.repository.InternalRepositoryException; import sonia.scm.repository.Tag; @@ -99,14 +101,16 @@ public class GitMirrorCommand extends AbstractGitCommand implements MirrorComman private final GitChangesetConverterFactory converterFactory; private final GitTagConverter gitTagConverter; private final GitWorkingCopyFactory workingCopyFactory; + private final GitHeadModifier gitHeadModifier; @Inject - GitMirrorCommand(GitContext context, MirrorHttpConnectionProvider mirrorHttpConnectionProvider, GitChangesetConverterFactory converterFactory, GitTagConverter gitTagConverter, GitWorkingCopyFactory workingCopyFactory) { + GitMirrorCommand(GitContext context, MirrorHttpConnectionProvider mirrorHttpConnectionProvider, GitChangesetConverterFactory converterFactory, GitTagConverter gitTagConverter, GitWorkingCopyFactory workingCopyFactory, GitHeadModifier gitHeadModifier) { super(context); this.mirrorHttpConnectionProvider = mirrorHttpConnectionProvider; this.converterFactory = converterFactory; this.gitTagConverter = gitTagConverter; this.workingCopyFactory = workingCopyFactory; + this.gitHeadModifier = gitHeadModifier; } @Override @@ -116,7 +120,23 @@ public class GitMirrorCommand extends AbstractGitCommand implements MirrorComman @Override public MirrorCommandResult update(MirrorCommandRequest mirrorCommandRequest) { - return inClone(git -> new Worker(context, mirrorCommandRequest, repository, git), workingCopyFactory, null); + // we have to select an existing branch here (and we cannot depend on a correct "default branch" in the + // configuration), because otherwise the clone will fail with an unresolvable branch. + String headBranch = resolveHeadBranch(); + return inClone(git -> new Worker(context, mirrorCommandRequest, this.repository, git), workingCopyFactory, headBranch); + } + + private String resolveHeadBranch() { + try { + Repository repository = context.open(); + Ref headRef = repository.findRef(Constants.HEAD); + if (headRef == null) { + throw new InternalRepositoryException(context.getRepository(), "Cannot handle missing HEAD in repository"); + } + return headRef.getTarget().getName().substring("refs/heads/".length()); + } catch (IOException e) { + throw new InternalRepositoryException(context.getRepository(), "could not resolve HEAD", e); + } } private class Worker extends GitCloneWorker { @@ -146,6 +166,11 @@ public class GitMirrorCommand extends AbstractGitCommand implements MirrorComman * pushing an empty ref to the central repository. */ private boolean deleteMasterAfterInitialSync = false; + /** + * We store a branch that has not been rejected here, so we can easily correct the HEAD reference + * afterwards (see #setHeadIfMirroredBranchExists) + */ + private String acceptedBranch; private Worker(GitContext context, MirrorCommandRequest mirrorCommandRequest, sonia.scm.repository.Repository repository, Git git) { super(git, context, repository); @@ -179,12 +204,37 @@ public class GitMirrorCommand extends AbstractGitCommand implements MirrorComman } push(generatePushRefSpecs().toArray(new String[0])); + setHeadIfMirroredBranchExists(); cleanUpMasterIfNecessary(); return new MirrorCommandResult(result, mirrorLog, stopwatch.stop().elapsed()); } + private void setHeadIfMirroredBranchExists() { + if (acceptedBranch != null) { + // Ensures that HEAD is set to an existing mirrored branch in the working copy (if this is set to a branch that + // should not have been mirrored, this branch cannot be deleted otherwise; see #cleanupMasterIfNecessary) and + // in the "real" mirror repository (here a HEAD with a not existing branch will lead to errors in the next clone + // call). + try { + RefUpdate refUpdate = git.getRepository().getRefDatabase().newUpdate(Constants.HEAD, true); + refUpdate.setForceUpdate(true); + refUpdate.link(acceptedBranch); + } catch (IOException e) { + throw new InternalRepositoryException(getRepository(), "Error while setting HEAD", e); + } + gitHeadModifier.ensure(repository, acceptedBranch.substring("refs/heads/".length())); + } + } + private void cleanUpMasterIfNecessary() { if (deleteMasterAfterInitialSync) { + try { + // we have to delete the master branch in the working copy, because otherwise it may be pushed + // to the mirror in the next synchronization call, when the working directory is cached. + git.branchDelete().setBranchNames("master").setForce(true).call(); + } catch (GitAPIException e) { + LOG.error("Could not delete master branch in mirror repository {}", getRepository().getNamespaceAndName(), e); + } push(":refs/heads/master"); } } @@ -269,7 +319,11 @@ public class GitMirrorCommand extends AbstractGitCommand implements MirrorComman private Result testFilterForBranch() { try { - return filter.acceptBranch(filterContext.getBranchUpdate(ref.getLocalName())); + Result filterResult = filter.acceptBranch(filterContext.getBranchUpdate(ref.getLocalName())); + if (filterResult.isAccepted()) { + acceptedBranch = ref.getLocalName(); + } + return filterResult; } catch (Exception e) { return handleExceptionFromFilter(e); } diff --git a/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitMirrorCommandTest.java b/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitMirrorCommandTest.java index 3dcad14627..5aa640dac7 100644 --- a/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitMirrorCommandTest.java +++ b/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitMirrorCommandTest.java @@ -30,10 +30,13 @@ import org.eclipse.jgit.api.Git; import org.eclipse.jgit.api.errors.GitAPIException; import org.eclipse.jgit.junit.http.AppServer; import org.eclipse.jgit.junit.http.SimpleHttpServer; +import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.Ref; +import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.revwalk.RevObject; import org.eclipse.jgit.revwalk.RevWalk; +import org.junit.After; import org.junit.Before; import org.junit.Rule; import org.junit.Test; @@ -43,13 +46,17 @@ import sonia.scm.net.GlobalProxyConfiguration; import sonia.scm.net.HttpURLConnectionFactory; import sonia.scm.repository.GitChangesetConverterFactory; import sonia.scm.repository.GitConfig; +import sonia.scm.repository.GitHeadModifier; +import sonia.scm.repository.GitUtil; import sonia.scm.repository.api.MirrorCommandResult; import sonia.scm.repository.api.MirrorFilter; import sonia.scm.repository.api.SimpleUsernamePasswordCredential; import sonia.scm.repository.work.NoneCachingWorkingCopyPool; +import sonia.scm.repository.work.SimpleWorkingCopyFactory; import sonia.scm.repository.work.WorkdirProvider; import sonia.scm.security.GPG; import sonia.scm.store.InMemoryConfigurationStoreFactory; +import sonia.scm.util.IOUtil; import javax.net.ssl.TrustManager; import java.io.File; @@ -80,9 +87,10 @@ public class GitMirrorCommandTest extends AbstractGitCommandTestBase { private final GitTagConverter gitTagConverter = new GitTagConverter(gpg); private File clone; + private File workdirAfterClose; private GitMirrorCommand command; - + private final GitHeadModifier gitHeadModifier = mock(GitHeadModifier.class); @Before public void bendContextToNewRepository() throws IOException, GitAPIException { @@ -92,7 +100,9 @@ public class GitMirrorCommandTest extends AbstractGitCommandTestBase { GitContext emptyContext = createMirrorContext(clone); SimpleGitWorkingCopyFactory workingCopyFactory = new SimpleGitWorkingCopyFactory( - new NoneCachingWorkingCopyPool(new WorkdirProvider(repositoryLocationResolver)), new SimpleMeterRegistry()); + new KeepingWorkingCopyPool(new WorkdirProvider(repositoryLocationResolver)), + new SimpleMeterRegistry() + ); MirrorHttpConnectionProvider mirrorHttpConnectionProvider = new MirrorHttpConnectionProvider( new HttpURLConnectionFactory( @@ -106,7 +116,15 @@ public class GitMirrorCommandTest extends AbstractGitCommandTestBase { mirrorHttpConnectionProvider, gitChangesetConverterFactory, gitTagConverter, - workingCopyFactory); + workingCopyFactory, + gitHeadModifier); + } + + @After + public void cleanupWorkdir() { + if (workdirAfterClose != null) { + IOUtil.deleteSilently(workdirAfterClose); + } } @Test @@ -154,6 +172,10 @@ public class GitMirrorCommandTest extends AbstractGitCommandTestBase { assertThat(createdMirror.branchList().call().stream().filter(r -> r.getName().contains("master")).findAny()) .isEmpty(); } + try (Repository workdirRepository = GitUtil.open(workdirAfterClose)) { + assertThat(workdirRepository.findRef(Constants.HEAD).getTarget().getName()).isEqualTo("refs/heads/test-branch"); + } + verify(gitHeadModifier).ensure(repository, "test-branch"); } @Test @@ -840,4 +862,16 @@ public class GitMirrorCommandTest extends AbstractGitCommandTestBase { private interface InvocationCheck { void invoked(); } + + private class KeepingWorkingCopyPool extends NoneCachingWorkingCopyPool { + + public KeepingWorkingCopyPool(WorkdirProvider workdirProvider) { + super(workdirProvider); + } + + @Override + public void contextClosed(SimpleWorkingCopyFactory.WorkingCopyContext workingCopyContext, File workdir) { + workdirAfterClose = workdir; + } + } }