diff --git a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/git/CILocalGitInfoBuilder.java b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/git/CILocalGitInfoBuilder.java index 8c2a73ff31d..acb8ba42fd6 100644 --- a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/git/CILocalGitInfoBuilder.java +++ b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/git/CILocalGitInfoBuilder.java @@ -54,7 +54,7 @@ private Path getGitPath(String repositoryPath) { @Override public int order() { - return 2; + return 3; } @Override diff --git a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/git/CIProviderGitInfoBuilder.java b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/git/CIProviderGitInfoBuilder.java index cf4d60ce8cf..97e05897780 100644 --- a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/git/CIProviderGitInfoBuilder.java +++ b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/git/CIProviderGitInfoBuilder.java @@ -32,7 +32,7 @@ public GitInfo build(@Nullable String repositoryPath) { @Override public int order() { - return 1; + return 2; } @Override diff --git a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/git/GitClientGitInfoBuilder.java b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/git/GitClientGitInfoBuilder.java index a12646a8f67..2c84b24d0f8 100644 --- a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/git/GitClientGitInfoBuilder.java +++ b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/git/GitClientGitInfoBuilder.java @@ -1,5 +1,7 @@ package datadog.trace.civisibility.git; +import static java.util.Collections.emptyList; + import datadog.trace.api.Config; import datadog.trace.api.civisibility.telemetry.tag.GitProviderDiscrepant; import datadog.trace.api.civisibility.telemetry.tag.GitProviderExpected; @@ -7,6 +9,7 @@ import datadog.trace.api.git.GitInfo; import datadog.trace.api.git.GitInfoBuilder; import datadog.trace.civisibility.git.tree.GitClient; +import datadog.trace.civisibility.utils.ShellCommandExecutor; import java.util.List; import javax.annotation.Nullable; import org.slf4j.Logger; @@ -33,14 +36,23 @@ public GitInfo build(@Nullable String repositoryPath) { GitClient gitClient = gitClientFactory.create(repositoryPath); try { String remoteName = config.getCiVisibilityGitRemoteName(); - String remoteUrl = gitClient.getRemoteUrl(remoteName); + String remoteUrl = null; + try { + remoteUrl = gitClient.getRemoteUrl(remoteName); + } catch (ShellCommandExecutor.ShellCommandFailedException e) { + LOGGER.debug("Could not read Git remote URL from {}", repositoryPath, e); + } String branch = gitClient.getCurrentBranch(); - List tags = gitClient.getTags(GitClient.HEAD); - String tag = !tags.isEmpty() ? tags.iterator().next() : null; + List tags = + branch == null || branch.isEmpty() ? gitClient.getTags(GitClient.HEAD) : emptyList(); + String tag = tags.size() == 1 ? tags.get(0) : null; CommitInfo commitInfo = gitClient.getCommitInfo(GitClient.HEAD, false); return new GitInfo(remoteUrl, branch, tag, commitInfo); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + return GitInfo.NOOP; } catch (Exception e) { LOGGER.debug("Error while getting Git data from {}", repositoryPath, e); LOGGER.warn("Error while getting Git data by executing shell commands"); @@ -50,7 +62,7 @@ public GitInfo build(@Nullable String repositoryPath) { @Override public int order() { - return 3; + return 1; } @Override diff --git a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/git/tree/GitClient.java b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/git/tree/GitClient.java index 140aa3999ab..231c1e019b7 100644 --- a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/git/tree/GitClient.java +++ b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/git/tree/GitClient.java @@ -48,6 +48,10 @@ CommitInfo getCommitInfo(String commit, boolean fetchIfNotPresent) @Nonnull List getLatestCommits() throws IOException, TimeoutException, InterruptedException; + @Nonnull + List getLatestCommits(String commit) + throws IOException, TimeoutException, InterruptedException; + @Nonnull List getObjects(Collection commitsToSkip, Collection commitsToInclude) throws IOException, TimeoutException, InterruptedException; diff --git a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/git/tree/GitDataUploaderImpl.java b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/git/tree/GitDataUploaderImpl.java index 5cbc46a51c9..deca10baeb1 100644 --- a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/git/tree/GitDataUploaderImpl.java +++ b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/git/tree/GitDataUploaderImpl.java @@ -96,7 +96,15 @@ private void uploadGitData() { GitInfo gitInfo = gitInfoProvider.getGitInfo(repoRoot); String remoteUrl = gitInfo.getRepositoryURL(); - List latestCommits = gitClient.getLatestCommits(); + String selectedCommit = gitInfo.getCommit().getSha(); + List latestCommits = gitClient.getLatestCommits(selectedCommit); + boolean unshallowed = false; + if (latestCommits.isEmpty() && config.isCiVisibilityGitUnshallowDefer()) { + unshallowed = gitRepoUnshallow.unshallow(); + if (unshallowed) { + latestCommits = gitClient.getLatestCommits(selectedCommit); + } + } if (latestCommits.isEmpty()) { LOGGER.debug("No commits in the last month, skipping git data upload"); callback.complete(null); @@ -112,8 +120,10 @@ private void uploadGitData() { return; } - if (config.isCiVisibilityGitUnshallowDefer() && gitRepoUnshallow.unshallow()) { - latestCommits = gitClient.getLatestCommits(); + if (config.isCiVisibilityGitUnshallowDefer() + && !unshallowed + && gitRepoUnshallow.unshallow()) { + latestCommits = gitClient.getLatestCommits(selectedCommit); commitsToSkip = gitDataApi.searchCommits(remoteUrl, latestCommits); } @@ -132,7 +142,7 @@ private void uploadGitData() { return; } - String currentCommit = latestCommits.get(0); + String currentCommit = gitInfo.getCommit().getSha(); Path packFilesDirectory = gitClient.createPackFiles(objectHashes); try { diff --git a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/git/tree/NoOpGitClient.java b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/git/tree/NoOpGitClient.java index 0a77fb72163..4f548931d7b 100644 --- a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/git/tree/NoOpGitClient.java +++ b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/git/tree/NoOpGitClient.java @@ -74,6 +74,11 @@ public CommitInfo getCommitInfo(String commit, boolean fetchIfNotPresent) { } @Nonnull + @Override + public List getLatestCommits(String commit) { + return Collections.emptyList(); + } + @Override public List getLatestCommits() { return Collections.emptyList(); diff --git a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/git/tree/ShellGitClient.java b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/git/tree/ShellGitClient.java index 8f63a088b28..a615148893a 100644 --- a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/git/tree/ShellGitClient.java +++ b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/git/tree/ShellGitClient.java @@ -24,6 +24,7 @@ import java.util.Collection; import java.util.Collections; import java.util.Comparator; +import java.util.LinkedHashSet; import java.util.List; import java.util.Objects; import java.util.concurrent.TimeoutException; @@ -45,7 +46,7 @@ public class ShellGitClient implements GitClient { Arrays.asList("release/", "hotfix/"); private static final Pattern WHITESPACE_PATTERN = Pattern.compile("\\s+"); private static final String ORIGIN = "origin"; - private static final Pattern COMMIT_INFO_SPLIT = Pattern.compile("\",\""); + private static final Pattern COMMIT_INFO_SPLIT = Pattern.compile("\\x00"); private final CiVisibilityMetricCollector metricCollector; private final String repoRoot; @@ -323,10 +324,9 @@ public List getTags(String commit) () -> { try { return commandExecutor.executeCommand( - IOUtils::readLines, buildGitCommand("describe", "--tags", "--exact-match", commit)); + IOUtils::readLines, buildGitCommand("tag", "--no-column", "--points-at", commit)); } catch (ShellCommandExecutor.ShellCommandFailedException e) { - // if provided commit is not tagged, - // command will fail because "--exact-match" is specified + // An unavailable revision cannot have local tags. return Collections.emptyList(); } }); @@ -438,14 +438,15 @@ public CommitInfo getCommitInfo(String commit, boolean fetchIfNotPresent) "show", commit, "-s", - "--format=%H\",\"%an\",\"%ae\",\"%aI\",\"%cn\",\"%ce\",\"%cI\",\"%B")) + "--no-show-signature", + "--format=%H%x00%an%x00%ae%x00%aI%x00%cn%x00%ce%x00%cI%x00%B")) .trim(); } catch (ShellCommandExecutor.ShellCommandFailedException e) { LOGGER.error("Failed to fetch commit info", e); return CommitInfo.NOOP; } - String[] fields = COMMIT_INFO_SPLIT.split(info); + String[] fields = COMMIT_INFO_SPLIT.split(info, 8); if (fields.length < 8) { LOGGER.error("Could not parse commit info: {}", info); return CommitInfo.NOOP; @@ -472,17 +473,42 @@ public CommitInfo getCommitInfo(String commit, boolean fetchIfNotPresent) @Override public List getLatestCommits() throws IOException, TimeoutException, InterruptedException { - return executeCommand( - Command.GET_LOCAL_COMMITS, - () -> - commandExecutor.executeCommand( - IOUtils::readLines, - buildGitCommand( - "log", - "--format=%H", - "-n", - String.valueOf(latestCommitsLimit), - String.format("--since='%s'", latestCommitsSince)))); + return getLatestCommits(HEAD); + } + + @Nonnull + @Override + public List getLatestCommits(String commit) + throws IOException, TimeoutException, InterruptedException { + if (GitUtils.isNotValidCommit(commit)) { + return Collections.emptyList(); + } + List commits; + try { + commits = + executeCommand( + Command.GET_LOCAL_COMMITS, + () -> + commandExecutor.executeCommand( + IOUtils::readLines, + buildGitCommand( + "log", + "--format=%H", + "-n", + String.valueOf(latestCommitsLimit), + String.format("--since='%s'", latestCommitsSince), + commit, + "--"))); + } catch (ShellCommandExecutor.ShellCommandFailedException e) { + if (!isCommitPresent(commit)) { + return Collections.emptyList(); + } + throw e; + } + if (!HEAD.equals(commit) && !commits.contains(commit) && isCommitPresent(commit)) { + commits.add(0, commit); + } + return commits; } /** @@ -505,14 +531,15 @@ public List getObjects( return executeCommand( Command.GET_OBJECTS, () -> { - String[] gitArgs = new String[5 + commitsToSkip.size() + commitsToInclude.size()]; + String[] gitArgs = new String[6 + commitsToSkip.size() + commitsToInclude.size()]; gitArgs[0] = "rev-list"; gitArgs[1] = "--objects"; gitArgs[2] = "--no-object-names"; gitArgs[3] = "--filter=blob:none"; gitArgs[4] = String.format("--since='%s'", latestCommitsSince); + gitArgs[5] = "--max-count=" + latestCommitsLimit; - int count = 5; + int count = 6; for (String commitToSkip : commitsToSkip) { gitArgs[count++] = "^" + commitToSkip; } @@ -520,7 +547,29 @@ public List getObjects( gitArgs[count++] = commitToInclude; } - return commandExecutor.executeCommand(IOUtils::readLines, buildGitCommand(gitArgs)); + List objects = + commandExecutor.executeCommand(IOUtils::readLines, buildGitCommand(gitArgs)); + LinkedHashSet outsideWindow = new LinkedHashSet<>(commitsToInclude); + for (String object : objects) { + outsideWindow.remove(object); + } + if (!outsideWindow.isEmpty()) { + List selectedArgs = new ArrayList<>(); + Collections.addAll( + selectedArgs, + "rev-list", + "--objects", + "--no-object-names", + "--filter=blob:none", + "--no-walk=unsorted"); + selectedArgs.addAll(outsideWindow); + LinkedHashSet combined = new LinkedHashSet<>(objects); + combined.addAll( + commandExecutor.executeCommand( + IOUtils::readLines, buildGitCommand(selectedArgs.toArray(new String[0])))); + return new ArrayList<>(combined); + } + return objects; }); } diff --git a/dd-java-agent/agent-ci-visibility/src/test/groovy/datadog/trace/civisibility/git/GitClientGitInfoBuilderTest.groovy b/dd-java-agent/agent-ci-visibility/src/test/groovy/datadog/trace/civisibility/git/GitClientGitInfoBuilderTest.groovy index 9d2dc02a2ee..dff99b1adda 100644 --- a/dd-java-agent/agent-ci-visibility/src/test/groovy/datadog/trace/civisibility/git/GitClientGitInfoBuilderTest.groovy +++ b/dd-java-agent/agent-ci-visibility/src/test/groovy/datadog/trace/civisibility/git/GitClientGitInfoBuilderTest.groovy @@ -52,6 +52,27 @@ class GitClientGitInfoBuilderTest extends Specification { "* Changes based on the review." } + def "keeps checkout metadata when no remote is configured"() { + given: + givenGitRepo() + Files.delete(tempDir.resolve(".git/config")) + def metricCollector = Stub(CiVisibilityMetricCollectorImpl) + def config = Stub(Config) { + getCiVisibilityGitRemoteName() >> "origin" + getCiVisibilityGitCommandTimeoutMillis() >> GIT_COMMAND_TIMEOUT_MILLIS + } + def builder = new GitClientGitInfoBuilder(config, new ShellGitClient.Factory(config, metricCollector)) + + when: + def gitInfo = builder.build(tempDir.toString()) + + then: + gitInfo.repositoryURL == null + gitInfo.branch == "master" + gitInfo.commit.sha == "5b6f3a6dab5972d73a56dff737bd08d995255c08" + gitInfo.commit.author.name == "Tony Redondo" + } + private void givenGitRepo() { givenGitRepo("ci/git/with_pack/git") } diff --git a/dd-java-agent/agent-ci-visibility/src/test/groovy/datadog/trace/civisibility/git/tree/GitDataUploaderImplTest.groovy b/dd-java-agent/agent-ci-visibility/src/test/groovy/datadog/trace/civisibility/git/tree/GitDataUploaderImplTest.groovy index 04cbb090c37..f7f80a6b4b9 100644 --- a/dd-java-agent/agent-ci-visibility/src/test/groovy/datadog/trace/civisibility/git/tree/GitDataUploaderImplTest.groovy +++ b/dd-java-agent/agent-ci-visibility/src/test/groovy/datadog/trace/civisibility/git/tree/GitDataUploaderImplTest.groovy @@ -2,6 +2,7 @@ package datadog.trace.civisibility.git.tree import datadog.trace.api.Config import datadog.trace.civisibility.telemetry.CiVisibilityMetricCollectorImpl +import datadog.trace.api.git.CommitInfo import datadog.trace.api.git.GitInfo import datadog.trace.api.git.GitInfoProvider import datadog.communication.util.IOUtils @@ -37,7 +38,7 @@ class GitDataUploaderImplTest extends Specification { } def api = Mock(GitDataApi) def gitInfoProvider = Stub(GitInfoProvider) - gitInfoProvider.getGitInfo(repoRoot) >> new GitInfo(repoUrl, null, null, null) + gitInfoProvider.getGitInfo(repoRoot) >> new GitInfo(repoUrl, null, null, new CommitInfo("5b6f3a6dab5972d73a56dff737bd08d995255c08")) def gitClient = new ShellGitClient(metricCollector, repoRoot, "25 years ago", 3, TIMEOUT_MILLIS) def unshallow = new GitRepoUnshallow(config, gitClient) diff --git a/dd-java-agent/agent-ci-visibility/src/test/java/datadog/trace/civisibility/git/GitClientGitInfoBuilderUnitTest.java b/dd-java-agent/agent-ci-visibility/src/test/java/datadog/trace/civisibility/git/GitClientGitInfoBuilderUnitTest.java new file mode 100644 index 00000000000..a99f3c0a767 --- /dev/null +++ b/dd-java-agent/agent-ci-visibility/src/test/java/datadog/trace/civisibility/git/GitClientGitInfoBuilderUnitTest.java @@ -0,0 +1,63 @@ +package datadog.trace.civisibility.git; + +import static java.util.Arrays.asList; +import static java.util.Collections.singletonList; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import datadog.trace.api.Config; +import datadog.trace.api.git.CommitInfo; +import datadog.trace.api.git.GitInfo; +import datadog.trace.civisibility.git.tree.GitClient; +import java.util.concurrent.TimeoutException; +import org.junit.jupiter.api.Test; + +class GitClientGitInfoBuilderUnitTest { + @Test + void interruptedRemoteLookupAbortsAndPreservesInterruption() throws Exception { + GitClient client = mock(GitClient.class); + when(client.getRemoteUrl("origin")).thenThrow(new InterruptedException()); + try { + assertEquals(GitInfo.NOOP, builder(client).build("/repo")); + assertTrue(Thread.currentThread().isInterrupted()); + verify(client, never()).getCurrentBranch(); + } finally { + Thread.interrupted(); + } + assertFalse(Thread.currentThread().isInterrupted()); + } + + @Test + void timedOutRemoteLookupAborts() throws Exception { + GitClient client = mock(GitClient.class); + when(client.getRemoteUrl("origin")).thenThrow(new TimeoutException()); + assertEquals(GitInfo.NOOP, builder(client).build("/repo")); + verify(client, never()).getCurrentBranch(); + } + + @Test + void tagsAreOnlyInferredForAnUnambiguousDetachedCheckout() throws Exception { + GitClient client = mock(GitClient.class); + when(client.getCommitInfo(GitClient.HEAD, false)).thenReturn(new CommitInfo("local-sha")); + when(client.getCurrentBranch()).thenReturn("main", "", ""); + when(client.getTags(GitClient.HEAD)).thenReturn(singletonList("v1"), asList("v1", "stable")); + GitClientGitInfoBuilder builder = builder(client); + assertNull(builder.build("/repo").getTag()); + assertEquals("v1", builder.build("/repo").getTag()); + assertNull(builder.build("/repo").getTag()); + } + + private GitClientGitInfoBuilder builder(GitClient client) { + Config config = mock(Config.class); + when(config.getCiVisibilityGitRemoteName()).thenReturn("origin"); + GitClient.Factory factory = mock(GitClient.Factory.class); + when(factory.create("/repo")).thenReturn(client); + return new GitClientGitInfoBuilder(config, factory); + } +} diff --git a/dd-java-agent/agent-ci-visibility/src/test/java/datadog/trace/civisibility/git/tree/GitDataUploaderSelectedCommitTest.java b/dd-java-agent/agent-ci-visibility/src/test/java/datadog/trace/civisibility/git/tree/GitDataUploaderSelectedCommitTest.java new file mode 100644 index 00000000000..256f4c4498b --- /dev/null +++ b/dd-java-agent/agent-ci-visibility/src/test/java/datadog/trace/civisibility/git/tree/GitDataUploaderSelectedCommitTest.java @@ -0,0 +1,58 @@ +package datadog.trace.civisibility.git.tree; + +import static java.util.Collections.emptyList; +import static java.util.Collections.singletonList; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.inOrder; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import datadog.trace.api.Config; +import datadog.trace.api.civisibility.telemetry.CiVisibilityMetricCollector; +import datadog.trace.api.git.CommitInfo; +import datadog.trace.api.git.GitInfo; +import datadog.trace.api.git.GitInfoProvider; +import java.util.concurrent.TimeUnit; +import org.junit.jupiter.api.Test; +import org.mockito.InOrder; + +class GitDataUploaderSelectedCommitTest { + @Test + void deferredUnshallowCanRecoverAMissingSelectedRevision() throws Exception { + String selected = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; + Config config = mock(Config.class); + when(config.isCiVisibilityGitUnshallowDefer()).thenReturn(true); + GitClient client = mock(GitClient.class); + when(client.getLatestCommits(selected)).thenReturn(emptyList(), singletonList(selected)); + GitRepoUnshallow unshallow = mock(GitRepoUnshallow.class); + when(unshallow.unshallow()).thenReturn(true); + GitInfoProvider info = mock(GitInfoProvider.class); + when(info.getGitInfo("/checkout")) + .thenReturn( + new GitInfo("https://example.com/repo.git", null, null, new CommitInfo(selected))); + GitDataApi api = mock(GitDataApi.class); + when(api.searchCommits("https://example.com/repo.git", singletonList(selected))) + .thenReturn(singletonList(selected)); + GitDataUploaderImpl uploader = + new GitDataUploaderImpl( + config, + mock(CiVisibilityMetricCollector.class), + api, + client, + unshallow, + info, + "/checkout", + "origin"); + + uploader.startOrObserveGitDataUpload().get(5, TimeUnit.SECONDS); + + InOrder order = inOrder(client, unshallow, api); + order.verify(client).getLatestCommits(selected); + order.verify(unshallow).unshallow(); + order.verify(client).getLatestCommits(selected); + order.verify(api).searchCommits("https://example.com/repo.git", singletonList(selected)); + verify(client, never()).getObjects(any(), any()); + } +} diff --git a/dd-java-agent/agent-ci-visibility/src/test/java/datadog/trace/civisibility/git/tree/ShellGitClientParsingTest.java b/dd-java-agent/agent-ci-visibility/src/test/java/datadog/trace/civisibility/git/tree/ShellGitClientParsingTest.java new file mode 100644 index 00000000000..ede296334ee --- /dev/null +++ b/dd-java-agent/agent-ci-visibility/src/test/java/datadog/trace/civisibility/git/tree/ShellGitClientParsingTest.java @@ -0,0 +1,125 @@ +package datadog.trace.civisibility.git.tree; + +import static java.nio.charset.StandardCharsets.UTF_8; +import static java.util.Collections.emptyList; +import static java.util.Collections.singletonList; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.mock; + +import datadog.communication.util.IOUtils; +import datadog.trace.api.civisibility.telemetry.CiVisibilityMetricCollector; +import datadog.trace.api.git.CommitInfo; +import datadog.trace.civisibility.utils.ShellCommandExecutor; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.List; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +class ShellGitClientParsingTest { + @TempDir Path checkout; + + @Test + void enumeratesEveryTagOnTheSelectedCommit() throws Exception { + Path fixture = + Paths.get(getClass().getClassLoader().getResource("ci/git/with_pack/git").toURI()); + IOUtils.copyFolder(fixture, Files.createDirectories(checkout.resolve(".git"))); + ShellCommandExecutor command = new ShellCommandExecutor(checkout.toFile(), 10_000); + command.executeCommand(IOUtils::readFully, "git", "config", "column.ui", "always"); + command.executeCommand(IOUtils::readFully, "git", "config", "column.tag", "always"); + ShellGitClient client = + new ShellGitClient( + mock(CiVisibilityMetricCollector.class), + checkout.toString(), + "1 month ago", + 1000, + 10_000); + assertTrue(client.getTags(GitClient.HEAD).isEmpty()); + command.executeCommand(IOUtils::readFully, "git", "update-ref", "refs/tags/v1", GitClient.HEAD); + assertEquals(singletonList("v1"), client.getTags(GitClient.HEAD)); + command.executeCommand( + IOUtils::readFully, "git", "update-ref", "refs/tags/stable", GitClient.HEAD); + assertEquals(2, client.getTags(GitClient.HEAD).size()); + assertTrue(client.getTags(GitClient.HEAD).contains("stable")); + assertTrue(client.getTags(GitClient.HEAD).contains("v1")); + } + + @Test + void selectedCommitHistoryAndPackObjectsDoNotSubstituteHeadOrDiscardOldCommits() + throws Exception { + Path fixture = + Paths.get(getClass().getClassLoader().getResource("ci/git/with_pack/git").toURI()); + IOUtils.copyFolder(fixture, Files.createDirectories(checkout.resolve(".git"))); + ShellGitClient client = + new ShellGitClient( + mock(CiVisibilityMetricCollector.class), + checkout.toString(), + "1 month ago", + 1000, + 10_000); + String selected = "98cd7c8e9cf71e02dc28bd9b13928bee0f85b74c"; + String head = "5b6f3a6dab5972d73a56dff737bd08d995255c08"; + String parent = + new ShellCommandExecutor(checkout.toFile(), 10_000) + .executeCommand(IOUtils::readFully, "git", "rev-parse", selected + "^") + .trim(); + + assertEquals(selected, client.getLatestCommits(selected).get(0)); + assertFalse(client.getLatestCommits(selected).contains(head)); + List objects = client.getObjects(emptyList(), singletonList(selected)); + assertTrue(objects.contains(selected)); + assertFalse(objects.contains(head)); + assertFalse(objects.contains(parent)); + assertTrue(client.getLatestCommits("--all").isEmpty()); + } + + @Test + void preservesSeparatorsInNamesAndMessagesAndSuppressesSignatureOutput() throws Exception { + Path fixture = + Paths.get(getClass().getClassLoader().getResource("ci/git/with_pack/git").toURI()); + Path gitDirectory = Files.createDirectories(checkout.resolve(".git")); + IOUtils.copyFolder(fixture, gitDirectory); + ShellCommandExecutor command = new ShellCommandExecutor(checkout.toFile(), 10_000); + String tree = + command.executeCommand(IOUtils::readFully, "git", "rev-parse", "HEAD^{tree}").trim(); + String author = "Do\",\"Jo"; + String message = "Update [\"a\",\"b\"]\n\nPreserve the complete message."; + String object = + "tree " + + tree + + "\nauthor " + + author + + " 1 +0000\n" + + "committer Committer 2 +0000\n\n" + + message + + "\n"; + String sha = + command + .executeCommand( + IOUtils::readFully, + object.getBytes(UTF_8), + "git", + "hash-object", + "-t", + "commit", + "-w", + "--stdin") + .trim(); + command.executeCommand(IOUtils::readFully, "git", "config", "log.showSignature", "true"); + ShellGitClient client = + new ShellGitClient( + mock(CiVisibilityMetricCollector.class), + checkout.toString(), + "1 month ago", + 1000, + 10_000); + + CommitInfo info = client.getCommitInfo(sha, false); + assertEquals(author, info.getAuthor().getName()); + assertEquals(message, info.getFullMessage()); + assertEquals("Tony Redondo", client.getCommitInfo(GitClient.HEAD, false).getAuthor().getName()); + } +} diff --git a/internal-api/src/main/java/datadog/trace/api/git/GitInfoProvider.java b/internal-api/src/main/java/datadog/trace/api/git/GitInfoProvider.java index 5dc94e713bc..e4aef310b40 100644 --- a/internal-api/src/main/java/datadog/trace/api/git/GitInfoProvider.java +++ b/internal-api/src/main/java/datadog/trace/api/git/GitInfoProvider.java @@ -71,7 +71,7 @@ private GitInfo buildGitInfo(String repositoryPath) { gi -> GitUtils.filterSensitiveInfo(gi.getRepositoryURL()), GitInfoProvider::validateGitRemoteUrl), evaluator.get(GitInfo::getBranch, Strings::isNotBlank), - evaluator.get(GitInfo::getTag, Strings::isNotBlank), + evaluator.getIfCommitShaMatches(GitInfo::getTag, Strings::isNotBlank), new CommitInfo( evaluator.get(gi1 -> gi1.getCommit().getSha(), Strings::isNotBlank), new PersonInfo( @@ -162,6 +162,7 @@ private static final class Evaluator { private final String repositoryPath; private final Map infos; private final Set shaDiscrepancies; + private Boolean gitClientAvailable; private Evaluator(String repositoryPath, Collection builders) { this.repositoryPath = repositoryPath; @@ -172,6 +173,49 @@ private Evaluator(String repositoryPath, Collection builders) { } } + private boolean isGitClientAvailable() { + if (gitClientAvailable != null) { + return gitClientAvailable; + } + GitInfo localInfo = null; + String explicitSha = null; + for (Map.Entry entry : infos.entrySet()) { + if (entry.getKey().providerAsExpected() == GitProviderExpected.GIT_CLIENT) { + localInfo = entry.getKey().build(repositoryPath); + entry.setValue(localInfo); + } else if (entry.getKey().providerAsExpected() == GitProviderExpected.USER_SUPPLIED + && entry.getValue() != null + && Strings.isNotBlank(entry.getValue().getCommit().getSha())) { + explicitSha = entry.getValue().getCommit().getSha(); + } + } + gitClientAvailable = localInfo != null && Strings.isNotBlank(localInfo.getCommit().getSha()); + if (gitClientAvailable && explicitSha == null) { + for (Map.Entry entry : infos.entrySet()) { + if (entry.getKey().providerAsExpected() != GitProviderExpected.CI_PROVIDER) { + continue; + } + GitInfo ciInfo = entry.getKey().build(repositoryPath); + entry.setValue(ciInfo); + String ciSha = ciInfo.getCommit().getSha(); + if (Strings.isNotBlank(ciSha) && !ciSha.equals(localInfo.getCommit().getSha())) { + boolean repositoryMismatch = + localInfo.getRepositoryURL() != null + && ciInfo.getRepositoryURL() != null + && !localInfo.getRepositoryURL().equals(ciInfo.getRepositoryURL()); + shaDiscrepancies.add( + new ShaDiscrepancy( + GitProviderExpected.GIT_CLIENT, + GitProviderDiscrepant.CI_PROVIDER, + repositoryMismatch + ? GitShaDiscrepancyType.REPOSITORY_DISCREPANCY + : GitShaDiscrepancyType.COMMIT_DISCREPANCY)); + } + } + } + return gitClientAvailable; + } + private String get(Function function, Predicate validator) { return get(function, validator, false); } @@ -194,6 +238,15 @@ private String get( GitProviderExpected expectedGitProvider = null; for (Map.Entry e : infos.entrySet()) { + GitProviderExpected provider = e.getKey().providerAsExpected(); + if (provider != GitProviderExpected.USER_SUPPLIED) { + boolean readableCheckout = isGitClientAvailable(); + if (readableCheckout + ? provider != GitProviderExpected.GIT_CLIENT + : provider == GitProviderExpected.GIT_CLIENT) { + continue; + } + } GitInfo info = e.getValue(); if (info == null) { GitInfoBuilder builder = e.getKey(); @@ -209,7 +262,7 @@ private String get( expectedCommitSha = currentCommitSha; expectedRepoUrl = info.getRepositoryURL(); expectedGitProvider = e.getKey().providerAsExpected(); - } else if (!expectedCommitSha.equals(currentCommitSha)) { + } else if (!expectedCommitSha.equalsIgnoreCase(currentCommitSha)) { // We already have a commit SHA from source that has higher priority. // Commit SHA from current source is different, so we have to skip it GitShaDiscrepancyType discrepancyType = GitShaDiscrepancyType.COMMIT_DISCREPANCY; diff --git a/internal-api/src/main/java/datadog/trace/api/git/GitUtils.java b/internal-api/src/main/java/datadog/trace/api/git/GitUtils.java index 892d4986bde..39bd4c6208e 100644 --- a/internal-api/src/main/java/datadog/trace/api/git/GitUtils.java +++ b/internal-api/src/main/java/datadog/trace/api/git/GitUtils.java @@ -299,6 +299,8 @@ public static boolean isValidPath(@Nonnull String path) { /** Checks if the provided string is neither a valid commit SHA nor a valid Git reference */ public static boolean isNotValidCommit(@Nullable String commit) { - return !isValidCommitSha(commit) && !isValidRef(commit); + return commit == null + || commit.startsWith("-") + || (!isValidCommitSha(commit) && !isValidRef(commit)); } } diff --git a/internal-api/src/test/java/datadog/trace/api/git/GitInfoProviderTest.java b/internal-api/src/test/java/datadog/trace/api/git/GitInfoProviderTest.java index 1291684d26b..cb05e08d50b 100644 --- a/internal-api/src/test/java/datadog/trace/api/git/GitInfoProviderTest.java +++ b/internal-api/src/test/java/datadog/trace/api/git/GitInfoProviderTest.java @@ -232,18 +232,17 @@ void testAddsCorrectTelemetryMetricsWhenShaDiscrepanciesAreFound() { new CommitInfo("shaA", PersonInfo.NOOP, PersonInfo.NOOP, "message")); GitInfo gitInfoB = new GitInfo("repoUrlA", null, null, new CommitInfo("shaB", AUTHOR, COMMITTER, "message")); - GitInfo gitInfoC = - new GitInfo("repoUrlB", null, null, new CommitInfo("shaC", AUTHOR, COMMITTER, "message")); + GitInfo gitInfoC = new GitInfo("repoUrlB", null, null, new CommitInfo("shaC")); GitInfoBuilder gitInfoBuilderA = givenABuilderReturning( - gitInfoA, 1, GitProviderExpected.CI_PROVIDER, GitProviderDiscrepant.CI_PROVIDER); + gitInfoA, 3, GitProviderExpected.CI_PROVIDER, GitProviderDiscrepant.CI_PROVIDER); GitInfoBuilder gitInfoBuilderB = givenABuilderReturning( gitInfoB, 2, GitProviderExpected.LOCAL_GIT, GitProviderDiscrepant.LOCAL_GIT); GitInfoBuilder gitInfoBuilderC = givenABuilderReturning( - gitInfoC, 3, GitProviderExpected.GIT_CLIENT, GitProviderDiscrepant.GIT_CLIENT); + gitInfoC, 1, GitProviderExpected.GIT_CLIENT, GitProviderDiscrepant.GIT_CLIENT); GitInfoProvider gitInfoProvider = new GitInfoProvider(); gitInfoProvider.registerGitInfoBuilder(gitInfoBuilderA); @@ -257,15 +256,8 @@ void testAddsCorrectTelemetryMetricsWhenShaDiscrepanciesAreFound() { .add( GIT_COMMIT_SHA_DISCREPANCY, 1L, - GitProviderExpected.CI_PROVIDER, - GitProviderDiscrepant.LOCAL_GIT, - GitShaDiscrepancyType.COMMIT_DISCREPANCY); - verify(metricCollector) - .add( - GIT_COMMIT_SHA_DISCREPANCY, - 1L, - GitProviderExpected.CI_PROVIDER, - GitProviderDiscrepant.GIT_CLIENT, + GitProviderExpected.GIT_CLIENT, + GitProviderDiscrepant.CI_PROVIDER, GitShaDiscrepancyType.REPOSITORY_DISCREPANCY); } @@ -320,6 +312,147 @@ void testIgnoresRemoteUrlsStartingWithFileProtocol() { assertEquals("http://usefulUrl", actualGitInfo.getRepositoryURL()); } + @Test + void testGitClientPrecedesCiAndDoesNotFillMissingFieldsFromCi() { + GitInfoProvider provider = new GitInfoProvider(); + provider.registerGitInfoBuilder( + givenABuilderReturning( + new GitInfo( + "https://ci/repo.git", + "ci-branch", + "ci-tag", + new CommitInfo("ci-sha", AUTHOR, COMMITTER, "ci-message")), + 3, + GitProviderExpected.CI_PROVIDER, + GitProviderDiscrepant.CI_PROVIDER)); + provider.registerGitInfoBuilder( + givenABuilderReturning( + new GitInfo(null, null, null, new CommitInfo("local-sha")), + 1, + GitProviderExpected.GIT_CLIENT, + GitProviderDiscrepant.GIT_CLIENT)); + + provider.registerGitInfoBuilder( + givenABuilderReturning( + new GitInfo( + "https://filesystem/repo.git", + "filesystem-branch", + "filesystem-tag", + new CommitInfo("local-sha", AUTHOR, COMMITTER, "filesystem-message")), + 4, + GitProviderExpected.LOCAL_GIT, + GitProviderDiscrepant.LOCAL_GIT)); + provider.registerGitInfoBuilder( + givenABuilderReturning( + new GitInfo( + "https://embedded/repo.git", + "embedded-branch", + "embedded-tag", + new CommitInfo("local-sha", AUTHOR, COMMITTER, "embedded-message")), + 5, + GitProviderExpected.EMBEDDED, + GitProviderDiscrepant.EMBEDDED)); + + GitInfo info = provider.getGitInfo(REPO_PATH); + assertEquals("local-sha", info.getCommit().getSha()); + assertNull(info.getRepositoryURL()); + assertNull(info.getBranch()); + assertNull(info.getTag()); + assertNull(info.getCommit().getFullMessage()); + assertNull(info.getCommit().getAuthor().getName()); + } + + @Test + void testCiGitMetadataIsUsedWhenGitClientCannotReadCheckout() { + GitInfoProvider provider = new GitInfoProvider(); + provider.registerGitInfoBuilder( + givenABuilderReturning( + new GitInfo( + "https://ci/repo.git", + "ci-branch", + "ci-tag", + new CommitInfo("ci-sha", AUTHOR, COMMITTER, "ci-message")), + 3, + GitProviderExpected.CI_PROVIDER, + GitProviderDiscrepant.CI_PROVIDER)); + provider.registerGitInfoBuilder( + givenABuilderReturning( + new GitInfo( + "https://unreadable/repo.git", "partial-branch", "partial-tag", CommitInfo.NOOP), + 1, + GitProviderExpected.GIT_CLIENT, + GitProviderDiscrepant.GIT_CLIENT)); + + GitInfo info = provider.getGitInfo(REPO_PATH); + assertEquals("ci-sha", info.getCommit().getSha()); + assertEquals("https://ci/repo.git", info.getRepositoryURL()); + assertEquals("ci-branch", info.getBranch()); + assertEquals("ci-tag", info.getTag()); + assertEquals("ci-message", info.getCommit().getFullMessage()); + } + + @Test + void testExplicitOverridesPrecedeReadableCheckout() { + GitInfoProvider provider = new GitInfoProvider(); + provider.registerGitInfoBuilder( + givenABuilderReturning( + new GitInfo( + "https://user/repo.git", + "user-branch", + "user-tag", + new CommitInfo("user-sha", AUTHOR, COMMITTER, "user-message")), + 0, + GitProviderExpected.USER_SUPPLIED, + GitProviderDiscrepant.USER_SUPPLIED)); + GitInfoBuilder gitClient = + givenABuilderReturning( + new GitInfo( + "https://local/repo.git", "local-branch", "local-tag", new CommitInfo("local-sha")), + 1, + GitProviderExpected.GIT_CLIENT, + GitProviderDiscrepant.GIT_CLIENT); + provider.registerGitInfoBuilder(gitClient); + + GitInfo info = provider.getGitInfo(REPO_PATH); + assertEquals("user-sha", info.getCommit().getSha()); + assertEquals("https://user/repo.git", info.getRepositoryURL()); + assertEquals("user-branch", info.getBranch()); + assertEquals("user-tag", info.getTag()); + assertEquals("user-message", info.getCommit().getFullMessage()); + verify(gitClient, never()).build(any()); + } + + @Test + void testExplicitShaDoesNotReportCiAgainstAnUnselectedLocalSha() { + CiVisibilityMetricCollector collector = mock(CiVisibilityMetricCollector.class); + InstrumentationBridge.registerMetricCollector(collector); + GitInfoProvider provider = new GitInfoProvider(); + provider.registerGitInfoBuilder( + givenABuilderReturning( + new GitInfo(null, null, null, new CommitInfo("selected-sha")), + 0, + GitProviderExpected.USER_SUPPLIED, + GitProviderDiscrepant.USER_SUPPLIED)); + provider.registerGitInfoBuilder( + givenABuilderReturning( + new GitInfo( + "https://local/repo.git", "local-branch", "local-tag", new CommitInfo("local-sha")), + 1, + GitProviderExpected.GIT_CLIENT, + GitProviderDiscrepant.GIT_CLIENT)); + GitInfoBuilder ci = + givenABuilderReturning( + new GitInfo("https://ci/repo.git", "ci-branch", null, new CommitInfo("selected-sha")), + 2, + GitProviderExpected.CI_PROVIDER, + GitProviderDiscrepant.CI_PROVIDER); + provider.registerGitInfoBuilder(ci); + + assertEquals("selected-sha", provider.getGitInfo(REPO_PATH).getCommit().getSha()); + assertNull(provider.getGitInfo(REPO_PATH).getTag()); + verify(ci, never()).build(any()); + } + private static void assertFallbackCommitInfo(GitInfo actualGitInfo) { assertEquals("repoUrl", actualGitInfo.getRepositoryURL()); diff --git a/internal-api/src/test/java/datadog/trace/api/git/GitUtilsTest.java b/internal-api/src/test/java/datadog/trace/api/git/GitUtilsTest.java index 73a0501ef70..03a8620baea 100644 --- a/internal-api/src/test/java/datadog/trace/api/git/GitUtilsTest.java +++ b/internal-api/src/test/java/datadog/trace/api/git/GitUtilsTest.java @@ -12,6 +12,20 @@ class GitUtilsTest { + @TableTest({ + "scenario | revision | invalid", + "all refs option | --all | true ", + "output option | --output=result | true ", + "head | HEAD | false ", + "branch | refs/heads/main | false ", + "short SHA | 123abcd | false ", + "missing | | true ", + "empty | '' | true " + }) + void testCommitReferenceRejectsOptions(String revision, boolean invalid) { + assertEquals(invalid, GitUtils.isNotValidCommit(revision)); + } + private static final PersonInfo JANE_DOE_PERSON_INFO = new PersonInfo("Jane Doe", "jane.doe@email.com");