Performance improvement by doing a checkout and merge only when the label is changing or if it's refresh time

Author: henri-tremblay

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

@@ -151,6 +151,15 @@ 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 si 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=
@@ -325,18 +334,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();
@@ -496,7 +514,9 @@ 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 {

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

@@ -533,12 +533,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 +548,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,6 +85,7 @@
 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;
@@ -800,17 +801,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 nor merge.
 		verify(git, never()).fetch();
 		verify(git).checkout();
-		verify(git).merge();
+		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