Merge pull request #416 from TheSnoozer/skip-git-invocation-correctly

The new caching should work for reactor builds with different prefixes and injectAllReactorProjects=true

Author: TheSnoozer

src/main/java/pl/project13/maven/git/GitCommitIdMojo.java

@@ -436,35 +436,36 @@ public void execute() throws MojoExecutionException {
         Properties contextProperties = getContextProperties(project);
         boolean alreadyInjected = injectAllReactorProjects && contextProperties != null;
         if (alreadyInjected) {
-          log.info("injectAllReactorProjects is enabled and this project already contains properties - using already computed values");
+          log.info("injectAllReactorProjects is enabled - attempting to use the already computed values");
           properties = contextProperties;
-        } else {
-          loadGitData(properties);
-          loadBuildData(properties);
-          propertiesReplacer.performReplacement(properties, replacementProperties);
-          propertiesFilterer.filter(properties, includeOnlyProperties, this.prefixDot);
-          propertiesFilterer.filterNot(properties, excludeProperties, this.prefixDot);
         }
+
+        loadGitData(properties);
+        loadBuildData(properties);
+        propertiesReplacer.performReplacement(properties, replacementProperties);
+        propertiesFilterer.filter(properties, includeOnlyProperties, this.prefixDot);
+        propertiesFilterer.filterNot(properties, excludeProperties, this.prefixDot);
+
         logProperties();
 
         if (generateGitPropertiesFile) {
           new PropertiesFileGenerator(log, buildContext, format, prefixDot, project.getName()).maybeGeneratePropertiesFile(
                   properties, project.getBasedir(), generateGitPropertiesFilename, sourceCharset);
         }
-        if (!alreadyInjected) {
-          publishPropertiesInto(project.getProperties());
-          // some plugins rely on the user properties (e.g. flatten-maven-plugin)
-          publishPropertiesInto(session.getUserProperties());
-
-          if (injectAllReactorProjects) {
-            appendPropertiesToReactorProjects();
-          }
-
-          if (injectIntoSysProperties) {
-            publishPropertiesInto(System.getProperties());
-            publishPropertiesInto(session.getSystemProperties());
-          }
+
+        publishPropertiesInto(project.getProperties());
+        // some plugins rely on the user properties (e.g. flatten-maven-plugin)
+        publishPropertiesInto(session.getUserProperties());
+
+        if (injectAllReactorProjects) {
+          appendPropertiesToReactorProjects();
         }
+
+        if (injectIntoSysProperties) {
+          publishPropertiesInto(System.getProperties());
+          publishPropertiesInto(session.getSystemProperties());
+        }
+
       } catch (Exception e) {
         handlePluginFailure(e);
       }
@@ -536,7 +537,7 @@ private File lookupGitDirectory() throws GitCommitIdExecutionException {
   private void logProperties() {
     for (Object key : properties.keySet()) {
       String keyString = key.toString();
-      log.info("found property {}", keyString);
+      log.info("including property {} in results", keyString);
     }
   }
 

src/main/java/pl/project13/maven/git/GitDataProvider.java

@@ -242,9 +242,12 @@ protected SimpleDateFormat getSimpleDateFormatWithTimeZone() {
   protected void maybePut(@Nonnull Properties properties, String key, SupplierEx<String> value)
           throws GitCommitIdExecutionException {
     String keyWithPrefix = prefixDot + key;
-    if (PropertiesFilterer.isIncluded(keyWithPrefix, includeOnlyProperties, excludeProperties)) {
+    if (properties.contains(keyWithPrefix)) {
+      String propertyValue = properties.getProperty(keyWithPrefix);
+      log.info("Using cached {} with value {}", keyWithPrefix, propertyValue);
+    } else if (PropertiesFilterer.isIncluded(keyWithPrefix, includeOnlyProperties, excludeProperties)) {
       String propertyValue = value.get();
-      log.info("{} {}", keyWithPrefix, propertyValue);
+      log.info("Collected {} with value {}", keyWithPrefix, propertyValue);
       PropertyManager.putWithoutPrefix(properties, keyWithPrefix, propertyValue);
     }
   }

src/main/java/pl/project13/maven/git/build/BuildServerDataProvider.java

@@ -30,6 +30,7 @@
 import java.util.Map;
 import java.util.Properties;
 import java.util.TimeZone;
+import java.util.function.Supplier;
 
 public abstract class BuildServerDataProvider {
 
@@ -125,21 +126,37 @@ private void loadBuildVersionAndTimeData(@Nonnull Properties properties) {
   }
 
   private void loadBuildHostData(@Nonnull Properties properties) {
-    String buildHost = null;
-    try {
-      buildHost = InetAddress.getLocalHost().getHostName();
-    } catch (UnknownHostException e) {
-      log.info("Unable to get build host, skipping property {}. Error message: {}",
-          GitCommitPropertyConstant.BUILD_HOST,
-          e.getMessage());
-    }
-    put(properties, GitCommitPropertyConstant.BUILD_HOST, buildHost);
+    Supplier<String> buildHostSupplier = () -> {
+      String buildHost = null;
+      try {
+        // this call might be costly - try to avoid it too (similar concept as in GitDataProvider)
+        buildHost = InetAddress.getLocalHost().getHostName();
+      } catch (UnknownHostException e) {
+        log.info("Unable to get build host, skipping property {}. Error message: {}",
+            GitCommitPropertyConstant.BUILD_HOST,
+            e.getMessage());
+      }
+      return buildHost;
+    };
+    maybePut(properties, GitCommitPropertyConstant.BUILD_HOST, buildHostSupplier);
   }
 
   protected void put(@Nonnull Properties properties, @Nonnull String key, String value) {
     String keyWithPrefix = prefixDot + key;
-    log.info("{} {}", keyWithPrefix, value);
+    log.info("Collected {} with value {}", keyWithPrefix, value);
     PropertyManager.putWithoutPrefix(properties, keyWithPrefix, value);
   }
 
+  protected void maybePut(@Nonnull Properties properties, @Nonnull String key, Supplier<String> supplier) {
+    String keyWithPrefix = prefixDot + key;
+    if (properties.contains(keyWithPrefix)) {
+      String propertyValue = properties.getProperty(keyWithPrefix);
+      log.info("Using cached {} with value {}", keyWithPrefix, propertyValue);
+    } else {
+      String propertyValue = supplier.get();
+      log.info("Collected {} with value {}", keyWithPrefix, propertyValue);
+      PropertyManager.putWithoutPrefix(properties, keyWithPrefix, propertyValue);
+    }
+  }
+
 }

src/test/java/pl/project13/maven/git/GitCommitIdMojoIntegrationTest.java

@@ -1568,7 +1568,45 @@ public void verifyDetachedHeadIsNotReportedAsBranch(boolean useNativeGit) throws
     assertPropertyPresentAndEqual(targetProject.getProperties(), "git.branch", "master");
   }
 
-  // TODO: Test that fails when trying to pass invalid data to evaluateOnCommit
+  @Test
+  @Parameters(method = "useNativeGit")
+  public void shouldGeneratePropertiesWithMultiplePrefixesAndReactorProject(boolean useNativeGit) throws Exception {
+    // given
+    mavenSandbox.withParentProject("my-pom-project", "pom")
+            .withGitRepoInParent(AvailableGitTestRepo.ON_A_TAG)
+            .withChildProject("my-child-module", "jar")
+            .create();
+    MavenProject targetProject = mavenSandbox.getChildProject(); // "my-child-two-module"
+
+    setProjectToExecuteMojoIn(targetProject);
+    GitDescribeConfig gitDescribeConfig = createGitDescribeConfig(false, 7);
+    gitDescribeConfig.setDirty("-dirty"); // checking if dirty works as expected
+
+    mojo.gitDescribe = gitDescribeConfig;
+    mojo.useNativeGit = useNativeGit;
+    mojo.injectAllReactorProjects = true;
+
+    List<String> prefixes = Arrays.asList("prefix-one", "prefix-two");
+    // when
+    // simulate plugin execution with multiple prefixes
+    // see https://github.com/git-commit-id/maven-git-commit-id-plugin/issues/137#issuecomment-418144756
+    for (String prefix: prefixes) {
+      mojo.prefix = prefix;
+
+      mojo.execute();
+    }
+
+    // then
+    // since we inject into all reactors both projects should have both properties
+    Properties properties = targetProject.getProperties();
+    for (String prefix: prefixes) {
+      assertPropertyPresentAndEqual(properties, prefix + ".commit.id.abbrev", "de4db35");
+
+      assertPropertyPresentAndEqual(properties, prefix + ".closest.tag.name", "v1.0.0");
+
+      assertPropertyPresentAndEqual(properties, prefix + ".closest.tag.commit.count", "0");
+    }
+  }
 
   private GitDescribeConfig createGitDescribeConfig(boolean forceLongFormat, int abbrev) {
     GitDescribeConfig gitDescribeConfig = new GitDescribeConfig();

src/test/java/pl/project13/maven/git/GitIntegrationTest.java

@@ -18,6 +18,7 @@
 package pl.project13.maven.git;
 
 import org.apache.commons.io.FileUtils;
+import org.apache.maven.execution.MavenSession;
 import org.apache.maven.project.MavenProject;
 import org.eclipse.jgit.api.Git;
 import org.junit.After;
@@ -26,9 +27,15 @@
 import javax.annotation.Nonnull;
 import java.io.File;
 import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
 import java.util.Optional;
+import java.util.Properties;
 import java.util.concurrent.ThreadLocalRandom;
 
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
 public abstract class GitIntegrationTest {
 
   private static final String SANDBOX_DIR = "target" + File.separator + "sandbox" + File.separator;
@@ -108,11 +115,30 @@ public static void initializeMojoWithDefaults(GitCommitIdMojo mojo) {
     mojo.commitIdGenerationMode = "full";
     mojo.evaluateOnCommit = evaluateOnCommit;
     mojo.nativeGitTimeoutInMs = (30 * 1000);
+    mojo.session = mockSession();
   }
 
   public void setProjectToExecuteMojoIn(@Nonnull MavenProject project) {
     mojo.project = project;
     mojo.dotGitDirectory = new File(project.getBasedir(), ".git");
+    mojo.reactorProjects = getReactorProjects(project);
+  }
+
+  private static MavenSession mockSession() {
+    MavenSession session = mock(MavenSession.class);
+    when(session.getUserProperties()).thenReturn(new Properties());
+    when(session.getSystemProperties()).thenReturn(new Properties());
+    return session;
+  }
+
+  private static List<MavenProject> getReactorProjects(@Nonnull MavenProject project) {
+    List<MavenProject> reactorProjects = new ArrayList<>();
+    MavenProject mavenProject = project;
+    while (mavenProject != null) {
+      reactorProjects.add(mavenProject);
+      mavenProject = mavenProject.getParent();
+    }
+    return reactorProjects;
   }
 
 }