Unnecessary refresh when staying on the same label and are not supposed to refresh

spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepository.java

@@ -24,6 +24,7 @@
 import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
+import java.util.concurrent.locks.ReentrantLock;
 
 import io.micrometer.observation.ObservationRegistry;
 import org.eclipse.jgit.api.CheckoutCommand;
@@ -78,6 +79,7 @@
  * @author Ryan Lynch
  * @author Gareth Clay
  * @author ChaoDong Xi
+ * @author Henri Tremblay
  */
 public class JGitEnvironmentRepository extends AbstractScmEnvironmentRepository
 		implements EnvironmentRepository, SearchPathLocator, InitializingBean {
@@ -150,12 +152,21 @@ public class JGitEnvironmentRepository extends AbstractScmEnvironmentRepository
 
 	private final ObservationRegistry observationRegistry;
 
+	/**
+	 * Contains the previousLabel that was checked out to prevent doing it again.
+	 *
+	 * @implNote
+	 * The field is not volatile since we tend to lock around the {@link #refresh(String)} method which uses it.
+	 * However, this method is public so maybe it's a bad bet.
+	 */
+	private String previousLabel = "$%^&"; // none existing branch
+
 	/**
 	 * This lock is used to ensure thread safety between accessing the local git repo from
 	 * both the ResourceController and the EnvironmentController. See <a href=
 	 * "https://github.com/spring-cloud/spring-cloud-config/issues/2681">#2681</a>.
 	 */
-	private final Object LOCK = new Object();
+	private final ReentrantLock LOCK = new ReentrantLock();
 
 	public JGitEnvironmentRepository(ConfigurableEnvironment environment, JGitEnvironmentProperties properties,
 			ObservationRegistry observationRegistry) {
@@ -262,9 +273,13 @@ public void setSkipSslValidation(boolean skipSslValidation) {
 
 	@Override
 	public synchronized Environment findOne(String application, String profile, String label, boolean includeOrigin) {
-		synchronized (LOCK) {
+		LOCK.lock();
+		try {
 			return super.findOne(application, profile, label, includeOrigin);
 		}
+		finally {
+			LOCK.unlock();
+		}
 	}
 
 	@Override
@@ -274,16 +289,26 @@ public synchronized Locations getLocations(String application, String profile, S
 		}
 		String version;
 		try {
-			synchronized (LOCK) {
+			try {
+				LOCK.lock();
 				version = refresh(label);
 			}
+			finally {
+				LOCK.unlock();
+			}
 		}
 		catch (Exception e) {
 			if (this.defaultLabel.equals(label) && JGitEnvironmentProperties.MAIN_LABEL.equals(this.defaultLabel)
 					&& tryMasterBranch) {
 				logger.info("Could not refresh default label " + label, e);
 				logger.info("Will try to refresh master label instead.");
-				version = refresh(JGitEnvironmentProperties.MASTER_LABEL);
+				LOCK.lock();
+				try {
+					version = refresh(JGitEnvironmentProperties.MASTER_LABEL);
+				}
+				finally {
+					LOCK.unlock();
+				}
 			}
 			else {
 				throw e;
@@ -310,18 +335,27 @@ public String refresh(String label) {
 		Git git = null;
 		try {
 			git = createGitClient();
+
+			// If we need to pull (which means we need to refresh from the remote),
+			//   we need to fetch, checkout and merge, even on the same label, because if that label is a tag and someone nastily moved it, we need a checkout
+			// If we do not need to pull
+			//   case a -> we are on the same label, we should do nothing
+			//   case b -> we should checkout
 			if (shouldPull(git)) {
 				FetchResult fetchStatus = fetch(git, label);
 				if (this.deleteUntrackedBranches && fetchStatus != null) {
 					deleteUntrackedLocalBranches(fetchStatus.getTrackingRefUpdates(), git);
 				}
-			}
 
-			// checkout after fetch so we can get any new branches, tags, ect.
-			// if nothing to update so just checkout and merge.
-			// Merge because remote branch could have been updated before
-			checkout(git, label);
-			tryMerge(git, label);
+				// checkout after fetch, so we can get any new branches, tags, etc.
+				checkout(git, label);
+				// merge because remote branch could have been updated
+				tryMerge(git, label);
+			}
+			else if (!label.equals(this.previousLabel)) {
+				// checkout the new label
+				checkout(git, label);
+			}
 
 			// always return what is currently HEAD as the version
 			return git.getRepository().findRef("HEAD").getObjectId().getName();
@@ -481,14 +515,16 @@ private Ref checkout(Git git, String label) throws GitAPIException {
 			// works for tags and local branches
 			checkout.setName(label);
 		}
-		return checkout.call();
+		Ref call = checkout.call();
+		previousLabel = label; // do it after the call to prevent changing it when it fails
+		return call;
 	}
 
 	protected boolean shouldPull(Git git) throws GitAPIException {
 		boolean shouldPull;
 
 		if (this.refreshRate < 0 || (this.refreshRate > 0
-				&& System.currentTimeMillis() - this.lastRefresh < (this.refreshRate * 1000))) {
+				&& System.currentTimeMillis() - this.lastRefresh < (this.refreshRate * 1000L))) {
 			return false;
 		}
 
@@ -566,7 +602,7 @@ protected FetchResult fetch(Git git, String label) {
 		configureCommand(fetch);
 		try {
 			FetchResult result = fetch.call();
-			if (result.getTrackingRefUpdates() != null && result.getTrackingRefUpdates().size() > 0) {
+			if (result.getTrackingRefUpdates() != null && !result.getTrackingRefUpdates().isEmpty()) {
 				this.logger.info("Fetched for remote " + label + " and found " + result.getTrackingRefUpdates().size()
 						+ " updates");
 			}

spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepositoryIntegrationTests.java

@@ -71,6 +71,7 @@
  * @author Roy Clarkson
  * @author Daniel Lavoie
  * @author Ryan Lynch
+ * @author Henri Tremblay
  */
 public class JGitEnvironmentRepositoryIntegrationTests {
 
@@ -533,12 +534,13 @@ public void testNewCommitIDWithRefreshRate() throws Exception {
 		StreamUtils.copy("foo: barNewCommit", Charset.defaultCharset(), out);
 		testData.getServerGit().getGit().add().addFilepattern("bar.properties").call();
 		testData.getServerGit().getGit().commit().setMessage("Updated for pull").call();
+		testData.getServerGit().getGit().tag().setName("testTag").call();
 		String updatedRemoteVersion = getCommitID(testData.getServerGit().getGit(), "master");
 
 		// Set refresh rate to 60 seconds (now it will fetch the remote repo only once)
 		testData.getRepository().setRefreshRate(60);
 
-		// Ask test label configuration first
+		// Ask test label configuration first, it will fetch but fail on the checkout
 		try {
 			testData.getRepository().findOne("bar", "staging", "test");
 			fail("Should have thrown NoSuchLabelException.");
@@ -547,10 +549,16 @@ public void testNewCommitIDWithRefreshRate() throws Exception {
 			// OK
 		}
 
-		// do a normal request and verify we get the new version
+		// do a normal request, the previous branch was the "master" branch, so it won't move
 		environment = testData.getRepository().findOne("bar", "staging", "master");
-		assertThat(updatedRemoteVersion).isEqualTo(environment.getVersion());
+		assertThat(startingRemoteVersion).isEqualTo(environment.getVersion());
 		Object fooProperty = ConfigServerTestUtils.getProperty(environment, "bar.properties", "foo");
+		assertThat("bar").isEqualTo(fooProperty);
+
+		// get the test tag which is on the latest commit. It was fetched so it will be found
+		environment = testData.getRepository().findOne("bar", "staging", "testTag");
+		assertThat(updatedRemoteVersion).isEqualTo(environment.getVersion());
+		fooProperty = ConfigServerTestUtils.getProperty(environment, "bar.properties", "foo");
 		assertThat("barNewCommit").isEqualTo(fooProperty);
 
 		// request the prior commit ID and make sure we get it

spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepositoryTests.java

@@ -85,8 +85,10 @@
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyString;
 import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.clearInvocations;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.mockingDetails;
+import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
@@ -95,6 +97,7 @@
 /**
  * @author Dave Syer
  * @author Gareth Clay
+ * @author Henri Tremblay
  */
 public class JGitEnvironmentRepositoryTests {
 
@@ -285,8 +288,8 @@ public void afterPropertiesSet_CloneOnStartFalse_CloneAndFetchNotCalled() throws
 		envRepository.setGitFactory(new MockGitFactory(mockGit, mockCloneCommand));
 		envRepository.setUri("http://somegitserver/somegitrepo");
 		envRepository.afterPropertiesSet();
-		verify(mockCloneCommand, times(0)).call();
-		verify(mockGit, times(0)).fetch();
+		verify(mockCloneCommand, never()).call();
+		verify(mockGit, never()).fetch();
 	}
 
 	@Test
@@ -303,8 +306,8 @@ public void afterPropertiesSet_CloneOnStartTrueWithFileURL_CloneAndFetchNotCalle
 		envRepository.setUri("file://somefilesystem/somegitrepo");
 		envRepository.setCloneOnStart(true);
 		envRepository.afterPropertiesSet();
-		verify(mockCloneCommand, times(0)).call();
-		verify(mockGit, times(0)).fetch();
+		verify(mockCloneCommand, never()).call();
+		verify(mockGit, never()).fetch();
 	}
 
 	@Test
@@ -638,7 +641,7 @@ public void testFetchException() throws Exception {
 		SearchPathLocator.Locations locations = this.repository.getLocations("bar", "staging", null);
 		assertThat(newObjectId.getName()).isEqualTo(locations.getVersion());
 
-		verify(git, times(0)).branchDelete();
+		verify(git, never()).branchDelete();
 	}
 
 	private Repository stubbedRepo() {
@@ -753,7 +756,7 @@ public void testMergeException() throws Exception {
 		SearchPathLocator.Locations locations = this.repository.getLocations("bar", "staging", "master");
 		assertThat(newObjectId.getName()).isEqualTo(locations.getVersion());
 
-		verify(git, times(0)).branchDelete();
+		verify(git, never()).branchDelete();
 	}
 
 	@Test
@@ -799,16 +802,35 @@ public void testRefreshWithoutFetch() throws Exception {
 		repo.setUri("http://somegitserver/somegitrepo");
 		repo.setBasedir(this.basedir);
 
-		// Set the refresh rate to 2 seconds and last update before 100ms. There should be
-		// no remote repo fetch.
+		// Set the refresh rate to 10 seconds and last update before 100ms. There should be
+		// no remote repo fetch and not merge.
 		repo.setLastRefresh(System.currentTimeMillis() - 100);
-		repo.setRefreshRate(2);
+		repo.setRefreshRate(10);
 
 		repo.refresh("master");
 
-		// Verify no fetch but merge only.
-		verify(git, times(0)).fetch();
-		verify(git).merge();
+		// Verify no fetch nor merge.
+		verify(git, never()).fetch();
+		verify(git).checkout();
+		verify(git, never()).merge();
+
+		// the checkout should not be called anymore since we are on the same branch
+		clearInvocations(git);
+		repo.refresh("master");
+
+		// Verify no fetch, checkout or merge
+		verify(git, never()).fetch();
+		verify(git, never()).checkout();
+		verify(git, never()).merge();
+
+		// checkout another branch, but still before the cache expires
+		clearInvocations(git);
+		repo.refresh("staging");
+
+		// Verify no fetch, a checkout and still no merge
+		verify(git, never()).fetch();
+		verify(git).checkout();
+		verify(git, never()).merge();
 	}
 
 	@Test
@@ -865,7 +887,7 @@ public void testResetHardException() throws Exception {
 		SearchPathLocator.Locations locations = this.repository.getLocations("bar", "staging", "master");
 		assertThat(newObjectId.getName()).isEqualTo(locations.getVersion());
 
-		verify(git, times(0)).branchDelete();
+		verify(git, never()).branchDelete();
 	}
 
 	@Test
@@ -1425,9 +1447,9 @@ public void afterPropertiesSet_CloneOnStartTrue_DefaultLabelSameAsDefaultBranch_
 		envRepository.afterPropertiesSet();
 		verify(mockCloneCommand, times(1)).call();
 		// Checkout/List Branch/Checkout setName should not be called
-		verify(mockCheckoutCommand, times(0)).call();
-		verify(mockListBranchCommand, times(0)).call();
-		verify(mockCheckoutCommand, times(0)).setName(anyString());
+		verify(mockCheckoutCommand, never()).call();
+		verify(mockListBranchCommand, never()).call();
+		verify(mockCheckoutCommand, never()).setName(anyString());
 	}
 
 	class MockCloneCommand extends CloneCommand {