Skip to content

Commit

Permalink
skip inactive history entries when generating history context (#4461)
Browse files Browse the repository at this point in the history 
fixes #496
  • Loading branch information
@vladak
vladak authored Nov 1, 2023
1 parent 667c06e commit d24098f
Show file tree
Hide file tree
Showing 3 changed files with 108 additions and 53 deletions.
4 changes: 3 additions & 1 deletion opengrok-indexer/src/main/java/org/opengrok/indexer/history/History.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
package org.opengrok.indexer.history;

import org.jetbrains.annotations.Nullable;
import org.jetbrains.annotations.VisibleForTesting;

import java.io.Serializable;
import java.util.ArrayList;
Expand Down Expand Up @@ -65,7 +66,8 @@ public History() {
this(new ArrayList<>());
}

History(List<HistoryEntry> entries) {
@VisibleForTesting
public History(List<HistoryEntry> entries) {
this(entries, Collections.emptyList());
}

Expand Down
114 changes: 63 additions & 51 deletions opengrok-indexer/src/main/java/org/opengrok/indexer/search/context/HistoryContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
*/

/*
* Copyright (c) 2005, 2021, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2005, 2023, Oracle and/or its affiliates. All rights reserved.
* Portions Copyright (c) 2017, 2020, Chris Fraire <[email protected]>.
*/
package org.opengrok.indexer.search.context;
Expand All @@ -34,6 +34,8 @@
import java.util.logging.Logger;

import org.apache.lucene.search.Query;
import org.jetbrains.annotations.Nullable;
import org.jetbrains.annotations.VisibleForTesting;
import org.opengrok.indexer.history.History;
import org.opengrok.indexer.history.HistoryEntry;
import org.opengrok.indexer.history.HistoryException;
Expand Down Expand Up @@ -119,32 +121,75 @@ public boolean getContext(File src, String path, Writer out, String context) thr
return getHistoryContext(hist, path, out, null, context);
}

private int matchLine(String line, String urlPrefix, String path, @Nullable Writer out, @Nullable List<Hit> hits,
String rev, String nrev) throws IOException {

int matchedLines = 0;
tokens.reInit(line);
String token;
int matchState;
long start = -1;
while ((token = tokens.next()) != null) {
for (LineMatcher lineMatcher : m) {
matchState = lineMatcher.match(token);
if (matchState == LineMatcher.MATCHED) {
if (start < 0) {
start = tokens.getMatchStart();
}
long end = tokens.getMatchEnd();
if (start > Integer.MAX_VALUE || end > Integer.MAX_VALUE) {
LOGGER.log(Level.WARNING, "Unexpected out of bounds for {0}", path);
} else if (hits != null) {
StringBuilder sb = new StringBuilder();
writeMatch(sb, line, (int) start, (int) end,
true, path, urlPrefix, nrev, rev);
hits.add(new Hit(path, sb.toString(), "", false, false));
} else {
writeMatch(out, line, (int) start, (int) end,
false, path, urlPrefix, nrev, rev);
}
matchedLines++;
break;
} else if (matchState == LineMatcher.WAIT) {
if (start < 0) {
start = tokens.getMatchStart();
}
} else {
start = -1;
}
}
}

return matchedLines;
}

/**
* Writes matching History log entries from 'in' to 'out' or to 'hits'.
* @param in the history to fetch entries from
* Writes matching history log entries to either 'out' or to 'hits'.
* @param history the history to fetch entries from
* @param out to write matched context
* @param path path to the file
* @param hits list of hits
* @param wcontext web context - beginning of url
* @param hits list of {@link Hit} instances
* @param urlPrefix URL prefix
* @return whether there was at least one line that matched
*/
private boolean getHistoryContext(
History in, String path, Writer out, List<Hit> hits, String wcontext) {
if (in == null) {
@VisibleForTesting
boolean getHistoryContext(History history, String path, @Nullable Writer out, @Nullable List<Hit> hits,
String urlPrefix) {
if (history == null) {
throw new IllegalArgumentException("`in' is null");
}
if ((out == null) == (hits == null)) {
// There should be exactly one destination for the output. If
// none or both are specified, it's a bug.
throw new IllegalArgumentException(
"Exactly one of out and hits should be non-null");
throw new IllegalArgumentException("Exactly one of out and hits should be non-null");
}

if (m == null) {
return false;
}

int matchedLines = 0;
Iterator<HistoryEntry> it = in.getHistoryEntries().iterator();
Iterator<HistoryEntry> it = history.getHistoryEntries().stream().filter(HistoryEntry::isActive).iterator();
try {
HistoryEntry he;
HistoryEntry nhe = null;
Expand All @@ -153,10 +198,11 @@ private boolean getHistoryContext(
if (nhe == null) {
he = it.next();
} else {
he = nhe; //nhe is the lookahead revision
he = nhe; // nhe is the lookahead revision
}
String line = he.getLine();
String rev = he.getRevision();

if (it.hasNext()) {
nhe = it.next();
} else {
Expand All @@ -169,40 +215,7 @@ private boolean getHistoryContext(
} else {
nrev = nhe.getRevision();
}
tokens.reInit(line);
String token;
int matchState;
long start = -1;
while ((token = tokens.next()) != null) {
for (LineMatcher lineMatcher : m) {
matchState = lineMatcher.match(token);
if (matchState == LineMatcher.MATCHED) {
if (start < 0) {
start = tokens.getMatchStart();
}
long end = tokens.getMatchEnd();
if (start > Integer.MAX_VALUE || end > Integer.MAX_VALUE) {
LOGGER.log(Level.INFO, "Unexpected out of bounds for {0}", path);
} else if (out == null) {
StringBuilder sb = new StringBuilder();
writeMatch(sb, line, (int) start, (int) end,
true, path, wcontext, nrev, rev);
hits.add(new Hit(path, sb.toString(), "", false, false));
} else {
writeMatch(out, line, (int) start, (int) end,
false, path, wcontext, nrev, rev);
}
matchedLines++;
break;
} else if (matchState == LineMatcher.WAIT) {
if (start < 0) {
start = tokens.getMatchStart();
}
} else {
start = -1;
}
}
}
matchedLines += matchLine(line, urlPrefix, path, out, hits, rev, nrev);
}
} catch (Exception e) {
LOGGER.log(Level.WARNING, "Could not get history context for " + path, e);
Expand All @@ -219,24 +232,23 @@ private boolean getHistoryContext(
* @param end position of the first char after the match
* @param flatten should multi-line log entries be flattened to a single
* @param path path to the file
* @param wcontext web context (begin of URL)
* @param urlPrefix URL prefix
* @param nrev old revision
* @param rev current revision
* line? If {@code true}, replace newline with space.
* @throws IOException IO exception
*/
protected static void writeMatch(Appendable out, String line,
int start, int end, boolean flatten, String path,
String wcontext, String nrev, String rev)
String urlPrefix, String nrev, String rev)
throws IOException {

String prefix = line.substring(0, start);
String match = line.substring(start, end);
String suffix = line.substring(end);

if (wcontext != null && nrev != null && !wcontext.isEmpty()) {
if (urlPrefix != null && nrev != null && !urlPrefix.isEmpty()) {
out.append("<a href=\"");
printHTML(out, wcontext + Prefix.DIFF_P +
printHTML(out, urlPrefix + Prefix.DIFF_P +
Util.uriEncodePath(path) +
"?" + QueryParameters.REVISION_2_PARAM_EQ + Util.uriEncodePath(path) + "@" +
rev + "&" + QueryParameters.REVISION_1_PARAM_EQ + Util.uriEncodePath(path) +
Expand Down
43 changes: 42 additions & 1 deletion opengrok-indexer/src/test/java/org/opengrok/indexer/search/context/HistoryContextTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,16 +18,21 @@
*/

/*
* Copyright (c) 2010, 2021, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2010, 2023, Oracle and/or its affiliates. All rights reserved.
* Portions Copyright (c) 2017, Chris Fraire <[email protected]>.
*/
package org.opengrok.indexer.search.context;

import java.io.File;
import java.io.IOException;
import java.io.StringWriter;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.ArrayList;
import java.util.Date;
import java.util.List;
import java.util.Set;

import org.apache.lucene.index.Term;
import org.apache.lucene.search.BooleanClause.Occur;
import org.apache.lucene.search.BooleanQuery;
Expand All @@ -37,11 +42,14 @@
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
import org.opengrok.indexer.condition.EnabledForRepository;
import org.opengrok.indexer.history.History;
import org.opengrok.indexer.history.HistoryEntry;
import org.opengrok.indexer.search.Hit;
import org.opengrok.indexer.util.TestRepository;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.opengrok.indexer.condition.RepositoryInstalled.Type.MERCURIAL;

Expand Down Expand Up @@ -190,4 +198,37 @@ void testWriteMatch() throws IOException {
"r1=/foo%20bar/haf%2Bhaf@1\" title=\"diff to previous version\">diff</a> <b>foo</b>",
sb.toString());
}

/**
* Test that inactive history entries are skipped when generating history context.
*/
@Test
void testGetHistoryContextVsInactiveHistoryEntries() {
Set<String> filePaths = Set.of(File.separator + Paths.get("teamware", "foo.c"));
History history = new History(List.of(
new HistoryEntry("1.2", "1.2", new Date(1485438707000L),
"Totoro",
"Uaaah\n", true, filePaths),
new HistoryEntry("1.2", "1.2", new Date(1485438706000L),
"Catbus",
"Miau\n", false, filePaths),
new HistoryEntry("1.1", "1.1", new Date(1485438705000L),
"Totoro",
"Hmmm\n", true, filePaths)
));

ArrayList<Hit> hits = new ArrayList<>();
BooleanQuery.Builder query = new BooleanQuery.Builder();
HistoryEntry lastHistoryEntry = history.getLastHistoryEntry();
assertNotNull(lastHistoryEntry);
query.add(new TermQuery(new Term("hist", lastHistoryEntry.getMessage().trim())), Occur.MUST);
HistoryContext historyContext = new HistoryContext(query.build());
final String path = "/foo/bar";
final String prefix = "prefix";
assertTrue(historyContext.getHistoryContext(history, path, null, hits, prefix));
assertEquals(1, hits.size());
assertTrue(hits.get(0).getLine().
startsWith(String.format("<a href=\"%s/diff%s?r2=%[email protected]&amp;r1=%[email protected]\"",
prefix, path, path, path)));
}
}

0 comments on commit d24098f

Please sign in to comment.