From 0d9f0eda2f059626caa22640e44d7505aab524a1 Mon Sep 17 00:00:00 2001 From: Vladimir Kotal Date: Wed, 11 Sep 2024 11:46:40 +0200 Subject: [PATCH] add tests for Mercurial tag parsing (#4659) Also, handle tags with multiple spaces. --- .../indexer/history/MercurialRepository.java | 5 ++ .../indexer/history/MercurialTagParser.java | 39 ++++------ .../opengrok/indexer/history/Repository.java | 4 +- .../opengrok/indexer/history/TagEntry.java | 4 ++ .../history/MercurialRepositoryTest.java | 72 ++++++++++++++++++- 5 files changed, 94 insertions(+), 30 deletions(-) diff --git a/opengrok-indexer/src/main/java/org/opengrok/indexer/history/MercurialRepository.java b/opengrok-indexer/src/main/java/org/opengrok/indexer/history/MercurialRepository.java index f1abb03eeae..9c69d789efd 100644 --- a/opengrok-indexer/src/main/java/org/opengrok/indexer/history/MercurialRepository.java +++ b/opengrok-indexer/src/main/java/org/opengrok/indexer/history/MercurialRepository.java @@ -635,6 +635,11 @@ protected void buildTagList(File directory, CommandTimeoutType cmdType) { ensureCommand(CMD_PROPERTY_KEY, CMD_FALLBACK); argv.add(RepoCommand); argv.add("tags"); + argv.add("--template"); + // Use '|' as a revision separator rather than ':' to avoid collision with the commonly used + // separator within the revision string (which is not used in the 'hg tags' output but better + // safe than sorry). + argv.add("{rev}|{tag}\\n"); Executor executor = new Executor(argv, directory, RuntimeEnvironment.getInstance().getCommandTimeout(cmdType)); diff --git a/opengrok-indexer/src/main/java/org/opengrok/indexer/history/MercurialTagParser.java b/opengrok-indexer/src/main/java/org/opengrok/indexer/history/MercurialTagParser.java index 26b8dfe3746..118ffb499c3 100644 --- a/opengrok-indexer/src/main/java/org/opengrok/indexer/history/MercurialTagParser.java +++ b/opengrok-indexer/src/main/java/org/opengrok/indexer/history/MercurialTagParser.java @@ -18,7 +18,7 @@ */ /* - * Copyright (c) 2018, 2019, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2018, 2024, Oracle and/or its affiliates. All rights reserved. */ package org.opengrok.indexer.history; @@ -26,6 +26,7 @@ import java.io.IOException; import java.io.InputStream; import java.io.InputStreamReader; +import java.util.NavigableSet; import java.util.TreeSet; import java.util.logging.Level; import java.util.logging.Logger; @@ -49,18 +50,17 @@ public class MercurialTagParser implements Executor.StreamHandler { * * @return entries a set of tag entries */ - public TreeSet getEntries() { + public NavigableSet getEntries() { return entries; } @Override public void processStream(InputStream input) throws IOException { try { - try (BufferedReader in = new BufferedReader( - new InputStreamReader(input))) { + try (BufferedReader in = new BufferedReader(new InputStreamReader(input))) { String line; while ((line = in.readLine()) != null) { - String[] parts = line.split(" *"); + String[] parts = line.split("\\|"); if (parts.length < 2) { LOGGER.log(Level.WARNING, "Failed to parse tag list: {0}", @@ -68,36 +68,21 @@ public void processStream(InputStream input) throws IOException { entries = null; break; } - // Grrr, how to parse tags with spaces inside? - // This solution will lose multiple spaces ;-/ - String tag = parts[0]; - for (int i = 1; i < parts.length - 1; ++i) { - tag = tag.concat(" "); - tag = tag.concat(parts[i]); - } + String rev = parts[0]; + String tag = parts[1]; + // The implicit 'tip' tag only causes confusion so ignore it. if (tag.contentEquals("tip")) { continue; } - String[] revParts = parts[parts.length - 1].split(":"); - if (revParts.length != 2) { - LOGGER.log(Level.WARNING, - "Failed to parse tag list: {0}", - "Mercurial revision parsing error: " - + parts[parts.length - 1]); - entries = null; - break; - } - TagEntry tagEntry - = new MercurialTagEntry(Integer.parseInt(revParts[0]), - tag); - // Reverse the order of the list + + TagEntry tagEntry = new MercurialTagEntry(Integer.parseInt(rev), tag); + // Reverse the order of the list. entries.add(tagEntry); } } } catch (IOException e) { - LOGGER.log(Level.WARNING, - "Failed to read tag list: {0}", e.getMessage()); + LOGGER.log(Level.WARNING, "Failed to read tag list: {0}", e.getMessage()); entries = null; } } diff --git a/opengrok-indexer/src/main/java/org/opengrok/indexer/history/Repository.java b/opengrok-indexer/src/main/java/org/opengrok/indexer/history/Repository.java index f7584add142..d2ddf2831f2 100644 --- a/opengrok-indexer/src/main/java/org/opengrok/indexer/history/Repository.java +++ b/opengrok-indexer/src/main/java/org/opengrok/indexer/history/Repository.java @@ -18,7 +18,7 @@ */ /* - * Copyright (c) 2008, 2023, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2008, 2024, Oracle and/or its affiliates. All rights reserved. * Portions Copyright (c) 2017, 2020, Chris Fraire . */ package org.opengrok.indexer.history; @@ -283,7 +283,7 @@ boolean hasFileBasedTags() { return false; } - NavigableSet getTagList() { + @Nullable NavigableSet getTagList() { return this.tagList; } diff --git a/opengrok-indexer/src/main/java/org/opengrok/indexer/history/TagEntry.java b/opengrok-indexer/src/main/java/org/opengrok/indexer/history/TagEntry.java index 66f630ac590..a1356e1f576 100644 --- a/opengrok-indexer/src/main/java/org/opengrok/indexer/history/TagEntry.java +++ b/opengrok-indexer/src/main/java/org/opengrok/indexer/history/TagEntry.java @@ -83,6 +83,10 @@ protected TagEntry(Date date, String tags) { this.tags = tags; } + public int getRevision() { + return revision; + } + public String getTags() { return this.tags; } diff --git a/opengrok-indexer/src/test/java/org/opengrok/indexer/history/MercurialRepositoryTest.java b/opengrok-indexer/src/test/java/org/opengrok/indexer/history/MercurialRepositoryTest.java index 4654e7d4b8f..0a80bd78592 100644 --- a/opengrok-indexer/src/test/java/org/opengrok/indexer/history/MercurialRepositoryTest.java +++ b/opengrok-indexer/src/test/java/org/opengrok/indexer/history/MercurialRepositoryTest.java @@ -31,19 +31,26 @@ import org.junit.jupiter.params.provider.MethodSource; import org.junit.jupiter.params.provider.ValueSource; import org.opengrok.indexer.condition.EnabledForRepository; +import org.opengrok.indexer.configuration.CommandTimeoutType; import org.opengrok.indexer.configuration.RuntimeEnvironment; import org.opengrok.indexer.util.Executor; +import org.opengrok.indexer.util.IOUtils; import org.opengrok.indexer.util.TestRepository; import java.io.File; import java.io.IOException; import java.io.InputStream; import java.net.URISyntaxException; +import java.nio.file.Files; +import java.nio.file.Path; import java.nio.file.Paths; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.List; +import java.util.Objects; +import java.util.Set; +import java.util.TreeSet; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -97,7 +104,7 @@ public class MercurialRepositoryTest { */ private void setUpTestRepository() throws IOException, URISyntaxException { repository = new TestRepository(); - repository.create(getClass().getResource("/repositories")); + repository.create(Objects.requireNonNull(getClass().getResource("/repositories"))); repositoryRoot = new File(repository.getSourceRoot(), "mercurial"); } @@ -441,4 +448,67 @@ void testAnnotationPositive(Pair> pair) throws Exception { List revisions = new ArrayList<>(annotation.getRevisions()); assertEquals(pair.getValue(), revisions); } + + /** + * Test special case of repository with no tags, in this case empty repository. + */ + @Test + void testBuildTagListEmpty() throws Exception { + Path repositoryRootPath = Files.createDirectory(Path.of(RuntimeEnvironment.getInstance().getSourceRootPath(), + "emptyTagsTest")); + File repositoryRoot = repositoryRootPath.toFile(); + assertTrue(repositoryRoot.isDirectory()); + runHgCommand(repositoryRoot, "init"); + MercurialRepository hgRepo = (MercurialRepository) RepositoryFactory.getRepository(repositoryRoot); + assertNotNull(hgRepo); + hgRepo.buildTagList(new File(hgRepo.getDirectoryName()), CommandTimeoutType.INDEXER); + assertEquals(0, hgRepo.getTagList().size()); + IOUtils.removeRecursive(repositoryRootPath); + } + + /** + * Extract the tags from the original repository. It already contains one tag. + */ + @Test + void testBuildTagListInitial() throws Exception { + MercurialRepository hgRepo = (MercurialRepository) RepositoryFactory.getRepository(repositoryRoot); + assertNotNull(hgRepo); + hgRepo.buildTagList(new File(hgRepo.getDirectoryName()), CommandTimeoutType.INDEXER); + var tags = hgRepo.getTagList(); + assertEquals(1, tags.size()); + Set expectedTags = new TreeSet<>(); + TagEntry tagEntry = new MercurialTagEntry(7, "start_of_novel"); + expectedTags.add(tagEntry); + assertEquals(expectedTags, tags); + } + + /** + * Clone the original repository, add new tag, check that the extracted tags contain the pre-existing + * and new one. + */ + @Test + void testBuildTagListOneMore() throws Exception { + Path repositoryRootPath = Files.createDirectory(Path.of(RuntimeEnvironment.getInstance().getSourceRootPath(), + "addedTagTest")); + File repositoryRoot = repositoryRootPath.toFile(); + // Clone the internal repository because it will be modified. + // This avoids interference with other tests in this class. + runHgCommand(this.repositoryRoot, "clone", this.repositoryRoot.toString(), repositoryRootPath.toString()); + MercurialRepository hgRepo = (MercurialRepository) RepositoryFactory.getRepository(repositoryRoot); + assertNotNull(hgRepo); + // Using double space on purpose to test the parsing of tags. + final String newTagName = "foo bar"; + runHgCommand(repositoryRoot, "tag", newTagName); + hgRepo.buildTagList(new File(hgRepo.getDirectoryName()), CommandTimeoutType.INDEXER); + var tags = hgRepo.getTagList(); + assertNotNull(tags); + assertEquals(2, tags.size()); + // TagEntry has special semantics for comparing/equality which does not compare the tags at all, + // so using assertEquals() on two sets of TagEntry objects would not help. + // Instead, check the tags separately. + assertEquals(List.of(7, 9), tags.stream().map(TagEntry::getRevision).collect(Collectors.toList())); + List expectedTags = List.of("start_of_novel", newTagName); + assertEquals(expectedTags, tags.stream().map(TagEntry::getTags).collect(Collectors.toList())); + IOUtils.removeRecursive(repositoryRootPath); + } }