Make the "ahead of remote" check more explicit (both in docs and error message) (#931)

Author: radoslaw-panuszewski

docs/configuration/checks.md

@@ -42,8 +42,15 @@ You can disable this check using either
 
 ## Ahead of remote check
 
-This check tries to verify if all local commits have been pushed to
-remote. In order to do so, it needs full information about remote and
+This check verifies that:
+
+- all local commits have been pushed to remote
+- no new remote commits appeared that haven't been pulled to local
+
+For long-running CI builds, it may happen that build for older merge commit will finish after the newer merge commit.
+In this case, the versions would be mixed (older commit would be tagged with a newer version).
+
+It needs full information about remote and
 HEAD commit, which might be lacking in CI environment (for more on CI
 build read [CI Servers](ci_servers.md)).
 

src/main/groovy/pl/allegro/tech/build/axion/release/VerifyReleaseTask.groovy

@@ -48,10 +48,13 @@ abstract class VerifyReleaseTask extends BaseAxionTask {
         boolean remoteAttached = repository.remoteAttached(versionConfig.repository.remote.get())
         def localOnly = localOnlyResolver.localOnly(remoteAttached)
         if (versionConfig.checks.checkAheadOfRemote().get() && !localOnly) {
-            boolean aheadOfRemote = repository.checkAheadOfRemote()
-            logger.quiet("Checking if branch is ahead of remote.. ${aheadOfRemote ? 'FAILED' : ''}")
-            if (aheadOfRemote && !dryRun) {
-                throw new IllegalStateException("Current branch is ahead of remote counterpart - can't release.")
+            int aheadOrBehindCommits = repository.numberOfCommitsAheadOrBehindRemote()
+
+            if (aheadOrBehindCommits > 0 && !dryRun) {
+                throw new IllegalStateException("Current branch is $aheadOrBehindCommits commits ahead of remote - can't release.")
+            }
+            if (aheadOrBehindCommits < 0 && !dryRun) {
+                throw new IllegalStateException("Current branch is ${-aheadOrBehindCommits} commits behind remote - can't release.")
             }
         } else {
             logger.quiet("Skipping ahead of remote check")

src/main/groovy/pl/allegro/tech/build/axion/release/infrastructure/DummyRepository.groovy

@@ -100,9 +100,9 @@ class DummyRepository implements ScmRepository {
     }
 
     @Override
-    boolean checkAheadOfRemote() {
+    int numberOfCommitsAheadOrBehindRemote() {
         log('check ahead of remote')
-        return false
+        return 0
     }
 
     @Override

src/main/groovy/pl/allegro/tech/build/axion/release/infrastructure/NoOpRepository.groovy

@@ -2,7 +2,13 @@ package pl.allegro.tech.build.axion.release.infrastructure
 
 import org.gradle.api.logging.Logger
 import org.gradle.api.logging.Logging
-import pl.allegro.tech.build.axion.release.domain.scm.*
+import pl.allegro.tech.build.axion.release.domain.scm.ScmIdentity
+import pl.allegro.tech.build.axion.release.domain.scm.ScmPosition
+import pl.allegro.tech.build.axion.release.domain.scm.ScmPushOptions
+import pl.allegro.tech.build.axion.release.domain.scm.ScmPushResult
+import pl.allegro.tech.build.axion.release.domain.scm.ScmPushResultOutcome
+import pl.allegro.tech.build.axion.release.domain.scm.ScmRepository
+import pl.allegro.tech.build.axion.release.domain.scm.TagsOnCommit
 
 import java.util.regex.Pattern
 
@@ -95,8 +101,8 @@ class NoOpRepository implements ScmRepository {
     }
 
     @Override
-    boolean checkAheadOfRemote() {
-        boolean aheadOfRemote = delegateRepository.checkAheadOfRemote()
+    int numberOfCommitsAheadOrBehindRemote() {
+        int aheadOfRemote = delegateRepository.numberOfCommitsAheadOrBehindRemote()
         log("ahead of remote: $aheadOfRemote")
         return aheadOfRemote
     }

src/main/java/pl/allegro/tech/build/axion/release/domain/scm/ScmRepository.java

@@ -34,7 +34,7 @@ public interface ScmRepository {
 
     boolean checkUncommittedChanges();
 
-    boolean checkAheadOfRemote();
+    int numberOfCommitsAheadOrBehindRemote();
 
     boolean isLegacyDefTagnameRepo();
 

src/main/java/pl/allegro/tech/build/axion/release/infrastructure/git/GitRepository.java

@@ -1,26 +1,56 @@
 package pl.allegro.tech.build.axion.release.infrastructure.git;
 
-import org.eclipse.jgit.api.*;
+import org.eclipse.jgit.api.AddCommand;
+import org.eclipse.jgit.api.FetchCommand;
+import org.eclipse.jgit.api.Git;
+import org.eclipse.jgit.api.LogCommand;
+import org.eclipse.jgit.api.PushCommand;
+import org.eclipse.jgit.api.Status;
 import org.eclipse.jgit.api.errors.GitAPIException;
 import org.eclipse.jgit.api.errors.NoHeadException;
 import org.eclipse.jgit.diff.DiffFormatter;
 import org.eclipse.jgit.errors.RepositoryNotFoundException;
-import org.eclipse.jgit.lib.*;
+import org.eclipse.jgit.lib.AnyObjectId;
+import org.eclipse.jgit.lib.BranchTrackingStatus;
+import org.eclipse.jgit.lib.Config;
+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.lib.StoredConfig;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevSort;
 import org.eclipse.jgit.revwalk.RevWalk;
-import org.eclipse.jgit.transport.*;
+import org.eclipse.jgit.transport.PushResult;
+import org.eclipse.jgit.transport.RemoteConfig;
+import org.eclipse.jgit.transport.RemoteRefUpdate;
+import org.eclipse.jgit.transport.TagOpt;
+import org.eclipse.jgit.transport.URIish;
 import org.eclipse.jgit.treewalk.filter.PathFilter;
 import org.eclipse.jgit.util.SystemReader;
 import org.eclipse.jgit.util.io.DisabledOutputStream;
 import org.gradle.api.logging.Logger;
 import org.gradle.api.logging.Logging;
-import pl.allegro.tech.build.axion.release.domain.scm.*;
+import pl.allegro.tech.build.axion.release.domain.scm.ScmException;
+import pl.allegro.tech.build.axion.release.domain.scm.ScmIdentity;
+import pl.allegro.tech.build.axion.release.domain.scm.ScmPosition;
+import pl.allegro.tech.build.axion.release.domain.scm.ScmProperties;
+import pl.allegro.tech.build.axion.release.domain.scm.ScmPushOptions;
+import pl.allegro.tech.build.axion.release.domain.scm.ScmPushResult;
+import pl.allegro.tech.build.axion.release.domain.scm.ScmPushResultOutcome;
+import pl.allegro.tech.build.axion.release.domain.scm.ScmRepository;
+import pl.allegro.tech.build.axion.release.domain.scm.ScmRepositoryUnavailableException;
+import pl.allegro.tech.build.axion.release.domain.scm.TagsOnCommit;
 
 import java.io.File;
 import java.io.IOException;
 import java.net.URISyntaxException;
-import java.util.*;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
 import java.util.regex.Pattern;
 import java.util.stream.StreamSupport;
 
@@ -554,7 +584,7 @@ public boolean checkUncommittedChanges() {
     }
 
     @Override
-    public boolean checkAheadOfRemote() {
+    public int numberOfCommitsAheadOrBehindRemote() {
         try {
             String branchName = jgitRepository.getRepository().getFullBranch();
             BranchTrackingStatus status = BranchTrackingStatus.of(jgitRepository.getRepository(), branchName);
@@ -563,7 +593,14 @@ public boolean checkAheadOfRemote() {
                 throw new ScmException("Branch " + branchName + " is not set to track another branch");
             }
 
-            return status.getAheadCount() != 0 || status.getBehindCount() != 0;
+            if (status.getAheadCount() > 0) {
+                return status.getAheadCount();
+            }
+            if (status.getBehindCount() > 0) {
+                return -status.getBehindCount();
+            }
+            return 0;
+
         } catch (IOException e) {
             throw new ScmException(e);
         }

src/test/groovy/pl/allegro/tech/build/axion/release/infrastructure/git/GitRepositoryTest.groovy

@@ -463,7 +463,7 @@ class GitRepositoryTest extends Specification {
 
     def "should pass ahead of remote check when in sync with remote"() {
         expect:
-        !repository.checkAheadOfRemote()
+        !repository.numberOfCommitsAheadOrBehindRemote()
     }
 
     def "should fail ahead of remote check when repository behind remote"() {
@@ -472,15 +472,15 @@ class GitRepositoryTest extends Specification {
         repository.fetchTags(ScmIdentity.defaultIdentityWithoutAgents(), "origin")
 
         expect:
-        repository.checkAheadOfRemote()
+        repository.numberOfCommitsAheadOrBehindRemote()
     }
 
     def "should fail ahead of remote check when repository has local commits"() {
         given:
         repository.commit(["*"], "local commit")
 
         expect:
-        repository.checkAheadOfRemote()
+        repository.numberOfCommitsAheadOrBehindRemote()
     }
 
     def "should fail ahead of remote check when on branch with no remote tracking"() {
@@ -496,7 +496,7 @@ class GitRepositoryTest extends Specification {
         rawRepository.branch.change(name: 'new-branch', startPoint: 'new-branch')
 
         when:
-        noRemoteRepository.checkAheadOfRemote()
+        noRemoteRepository.numberOfCommitsAheadOrBehindRemote()
 
         then:
         thrown(ScmException)

src/test/groovy/pl/allegro/tech/build/axion/release/infrastructure/git/NoOpRepositoryTest.groovy

@@ -59,10 +59,18 @@ class NoOpRepositoryTest extends Specification {
 
     def "should check ahead of remote on real scm"() {
         given:
-        scm.checkAheadOfRemote() >> true
+        scm.numberOfCommitsAheadOrBehindRemote() >> 1
 
         expect:
-        dryRepository.checkAheadOfRemote()
+        dryRepository.numberOfCommitsAheadOrBehindRemote() == 1
+    }
+
+    def "should check behind remote on real scm"() {
+        given:
+        scm.numberOfCommitsAheadOrBehindRemote() >> -1
+
+        expect:
+        dryRepository.numberOfCommitsAheadOrBehindRemote() == -1
     }
 
 }