Use merge base to find original version of transport resources (elastic#135564) (elastic#135728)

Transport version generation relies on finding the original contents of
transport resources before they may have been modified by the current
branch. Currently it looks directly at the upstream main branch. Yet
that means newly generated ids could leave holes if the branch is out of
date with upstream main.

Instead, this commit now uses the merge base of the branch. The merge
base is calculcated against upstream main. This allows generation to be
locally consistent with the branch. The generated id may already be used
on main, but that will be detected by a merge conflict in the upper
bound before merging the PR. Additionally if in the middle of a merge,
like with resolveTransportVersionConflict, the MERGE_HEAD is used as the
base so that the regenerated ids will be based on the state at the end
of the merge.

Author: rjernst

build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/transport/AbstractTransportVersionFuncTest.groovy

@@ -116,7 +116,7 @@ class AbstractTransportVersionFuncTest extends AbstractGradleFuncTest {
             include ':myplugin'
         """
         propertiesFile << """
-            org.elasticsearch.transports.upstreamRef=main
+            org.elasticsearch.transport.upstreamRef=main
         """
         versionPropertiesFile.text = versionPropertiesFile.text.replace("9.1.0", "9.2.0")
 

build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/transport/ResolveTransportVersionConflictFuncTest.groovy

@@ -87,4 +87,44 @@ class ResolveTransportVersionConflictFuncTest extends AbstractTransportVersionFu
         assertUpperBound("9.1", "new_tv,8012002")
         assertUpperBound("8.19", "new_tv,7123002")
     }
+
+    def "no new transport version is idempotent"() {
+        when:
+        def result = runResolveAndValidateTask().build()
+
+        then:
+        assertResolveAndValidateSuccess(result)
+        assertUpperBound("9.2", "existing_92,8123000")
+    }
+
+    def "upstream changes don't affect merge"() {
+        given:
+        // setup main with 2 commits, but we will only merge in the first one
+        execute("git checkout main")
+        referableAndReferencedTransportVersion("upstream_new_tv1", "8124000")
+        transportVersionUpperBound("9.2", "upstream_new_tv1", "8124000")
+        execute("git add .")
+        execute("git commit -m update1")
+        String toMerge = execute("git rev-parse HEAD")
+        referableAndReferencedTransportVersion("upstream_new_tv2", "8125000")
+        transportVersionUpperBound("9.2", "upstream_new_tv2", "8125000")
+        execute("git add .")
+        execute("git commit -m update2")
+        execute("git checkout test")
+        // now commit a conflict on the test branch, a new TV
+        referableAndReferencedTransportVersion("branch_new_tv", "8124000")
+        transportVersionUpperBound("9.2", "branch_new_tv", "8124000")
+        execute("git add .")
+        execute("git commit -m branch")
+        // and finally initiate the merge
+        System.out.println("Merging commit " + toMerge);
+        execute("git merge " + toMerge, testProjectDir.root, true);
+
+        when:
+        def result = runResolveAndValidateTask().build()
+
+        then:
+        assertResolveAndValidateSuccess(result)
+        assertUpperBound("9.2", "branch_new_tv,8125000")
+    }
 }

build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/transport/TransportVersionGenerationFuncTest.groovy

@@ -496,4 +496,21 @@ class TransportVersionGenerationFuncTest extends AbstractTransportVersionFuncTes
         assertUpperBound("9.2", "new_tv,8124000")
         assertReferableDefinition("new_tv", "8124000")
     }
+
+    def "generation is idempotent on upstream changes"() {
+        given:
+        execute("git checkout main")
+        referableAndReferencedTransportVersion("new_tv", "8124000")
+        transportVersionUpperBound("9.2", "new_tv", "8124000")
+        execute("git add .")
+        execute("git commit -m update")
+        execute("git checkout test")
+
+        when:
+        def result = runGenerateAndValidateTask().build()
+
+        then:
+        assertGenerateAndValidateSuccess(result)
+        assertUpperBound("9.2", "existing_92,8123000")
+    }
 }

build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/TransportVersionResourcesService.java

@@ -58,6 +58,7 @@
 public abstract class TransportVersionResourcesService implements BuildService<TransportVersionResourcesService.Parameters> {
 
     private static final Logger logger = Logging.getLogger(TransportVersionResourcesService.class);
+    private static final String UPSTREAM_REMOTE_NAME = "transport-version-resources-upstream";
 
     public interface Parameters extends BuildServiceParameters {
         DirectoryProperty getTransportResourcesDirectory();
@@ -80,17 +81,16 @@ record IdAndDefinition(TransportVersionId id, TransportVersionDefinition definit
 
     private final Path transportResourcesDir;
     private final Path rootDir;
-    private final AtomicReference<String> upstreamRefName = new AtomicReference<>();
+    private final String upstreamRefOverride;
+    private final AtomicReference<String> baseRefName = new AtomicReference<>();
     private final AtomicReference<Set<String>> upstreamResources = new AtomicReference<>(null);
     private final AtomicReference<Set<String>> changedResources = new AtomicReference<>(null);
 
     @Inject
     public TransportVersionResourcesService(Parameters params) {
         this.transportResourcesDir = params.getTransportResourcesDirectory().get().getAsFile().toPath();
         this.rootDir = params.getRootDirectory().get().getAsFile().toPath();
-        if (params.getUpstreamRefOverride().isPresent()) {
-            upstreamRefName.set(params.getUpstreamRefOverride().get());
-        }
+        upstreamRefOverride = params.getUpstreamRefOverride().getOrNull();
     }
 
     /**
@@ -258,52 +258,65 @@ private Path getUpperBoundRelativePath(String name) {
         return UPPER_BOUNDS_DIR.resolve(name + ".csv");
     }
 
-    private String getUpstreamRefName() {
-        if (upstreamRefName.get() == null) {
-            synchronized (upstreamRefName) {
-                String remotesOutput = gitCommand("remote").strip();
-
+    private String getBaseRefName() {
+        if (baseRefName.get() == null) {
+            synchronized (baseRefName) {
                 String refName;
-                if (remotesOutput.isEmpty()) {
-                    refName = "main"; // fallback to local main if no remotes, this happens in tests
+                // the existence of the MERGE_HEAD ref means we are in the middle of a merge, and should use that as our base
+                String gitDir = gitCommand("rev-parse", "--git-dir").strip();
+                if (Files.exists(Path.of(gitDir).resolve("MERGE_HEAD"))) {
+                    refName = gitCommand("rev-parse", "--verify", "MERGE_HEAD").strip();
                 } else {
-                    List<String> remoteNames = List.of(remotesOutput.split("\n"));
-                    String transportVersionRemoteName = "transport-version-resources-upstream";
-                    if (remoteNames.contains(transportVersionRemoteName) == false) {
-                        // our special remote doesn't exist yet, so create it
-                        String upstreamUrl = null;
-                        for (String remoteName : remoteNames) {
-                            String getUrlOutput = gitCommand("remote", "get-url", remoteName).strip();
-                            if (getUrlOutput.startsWith("[email protected]:elastic/")
-                                || getUrlOutput.startsWith("https://github.com/elastic/")) {
-                                upstreamUrl = getUrlOutput;
-                            }
-                        }
-
-                        if (upstreamUrl != null) {
-                            gitCommand("remote", "add", transportVersionRemoteName, upstreamUrl);
-                        } else {
-                            throw new RuntimeException("No elastic github remotes found to copy");
-                        }
-                    }
-
-                    // make sure the remote main ref is up to date
-                    gitCommand("fetch", transportVersionRemoteName, "main");
-
-                    refName = transportVersionRemoteName + "/main";
+                    String upstreamRef = findUpstreamRef();
+                    refName = gitCommand("merge-base", upstreamRef, "HEAD").strip();
+                }
+
+                baseRefName.set(refName);
+            }
+        }
+        return baseRefName.get();
+    }
+
+    private String findUpstreamRef() {
+        if (upstreamRefOverride != null) {
+            return upstreamRefOverride;
+        }
+
+        String remotesOutput = gitCommand("remote").strip();
+        if (remotesOutput.isEmpty()) {
+            throw new RuntimeException(
+                "No remotes found. If this is a test set gradle property " + "org.elasticsearch.transport.upstreamRef"
+            );
+        }
+        List<String> remoteNames = List.of(remotesOutput.split("\n"));
+        if (remoteNames.contains(UPSTREAM_REMOTE_NAME) == false) {
+            // our special remote doesn't exist yet, so create it
+            String upstreamUrl = null;
+            for (String remoteName : remoteNames) {
+                String getUrlOutput = gitCommand("remote", "get-url", remoteName).strip();
+                if (getUrlOutput.startsWith("[email protected]:elastic/") || getUrlOutput.startsWith("https://github.com/elastic/")) {
+                    upstreamUrl = getUrlOutput;
                 }
-                upstreamRefName.set(refName);
+            }
 
+            if (upstreamUrl != null) {
+                gitCommand("remote", "add", UPSTREAM_REMOTE_NAME, upstreamUrl);
+            } else {
+                throw new RuntimeException("No elastic github remotes found to copy");
             }
         }
-        return upstreamRefName.get();
+
+        // make sure the remote main ref is up to date
+        gitCommand("fetch", UPSTREAM_REMOTE_NAME, "main");
+
+        return UPSTREAM_REMOTE_NAME + "/main";
     }
 
     // Return the transport version resources paths that exist in upstream
     private Set<String> getUpstreamResources() {
         if (upstreamResources.get() == null) {
             synchronized (upstreamResources) {
-                String output = gitCommand("ls-tree", "--name-only", "-r", getUpstreamRefName(), ".");
+                String output = gitCommand("ls-tree", "--name-only", "-r", getBaseRefName(), ".");
 
                 HashSet<String> resources = new HashSet<>();
                 Collections.addAll(resources, output.split("\n")); // git always outputs LF
@@ -319,7 +332,7 @@ private Set<String> getChangedResources() {
             synchronized (changedResources) {
                 HashSet<String> resources = new HashSet<>();
 
-                String diffOutput = gitCommand("diff", "--name-only", "--relative", getUpstreamRefName(), ".");
+                String diffOutput = gitCommand("diff", "--name-only", "--relative", getBaseRefName(), ".");
                 if (diffOutput.strip().isEmpty() == false) {
                     Collections.addAll(resources, diffOutput.split("\n")); // git always outputs LF
                 }
@@ -357,7 +370,7 @@ private <T> T getUpstreamFile(Path resourcePath, BiFunction<Path, String, T> par
             return null;
         }
 
-        String content = gitCommand("show", getUpstreamRefName() + ":./" + pathString).strip();
+        String content = gitCommand("show", getBaseRefName() + ":./" + pathString).strip();
         return parser.apply(resourcePath, content);
     }
 

build-tools/src/testFixtures/groovy/org/elasticsearch/gradle/fixtures/AbstractGradleFuncTest.groovy

@@ -211,10 +211,10 @@ abstract class AbstractGradleFuncTest extends Specification {
         """
     }
 
-    String execute(String command, File workingDir = testProjectDir.root) {
+    String execute(String command, File workingDir = testProjectDir.root, boolean ignoreFailure = false) {
         def proc = command.execute(Collections.emptyList(), workingDir)
         proc.waitFor()
-        if (proc.exitValue()) {
+        if (proc.exitValue() && ignoreFailure == false) {
             String msg = """Error running command ${command}:
                 Sysout: ${proc.inputStream.text}
                 Syserr: ${proc.errorStream.text}