Skip to content

Commit 4d0ddab

Browse files
vladakvladak
committed
rework the logic of displaying times for directory entries
fixes #4293
1 parent 68dacfa commit 4d0ddab

File tree

8 files changed

+69
-55
lines changed

8 files changed

+69
-55
lines changed

‎opengrok-indexer/src/main/java/org/opengrok/indexer/history/FileHistoryCache.java‎

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,6 @@
3939
import java.nio.file.Files;
4040
import java.nio.file.Path;
4141
import java.util.ArrayList;
42-
import java.util.Collections;
4342
import java.util.HashMap;
4443
import java.util.Iterator;
4544
import java.util.List;
@@ -772,22 +771,24 @@ private String getCachedRevision(Repository repository, String revPath) {
772771
}
773772

774773
@Override
775-
public Map<String, HistoryEntry> getLastHistoryEntries(List<DirectoryEntry> entries) {
774+
public void fillLastHistoryEntries(List<DirectoryEntry> entries) {
776775
if (entries == null) {
777-
return Collections.emptyMap();
776+
return;
778777
}
779778

780-
Map<String, HistoryEntry> map = new HashMap<>();
781779
for (DirectoryEntry directoryEntry : entries) {
782780
try {
783781
File file = directoryEntry.getFile();
784782
if (file.isDirectory()) {
783+
directoryEntry.setDescription("-");
784+
directoryEntry.setDate(null);
785785
continue;
786786
}
787787

788788
HistoryEntry historyEntry = getLastHistoryEntry(file);
789789
if (historyEntry != null && historyEntry.getDate() != null) {
790-
map.put(directoryEntry.getFile().getName(), historyEntry);
790+
directoryEntry.setDescription(historyEntry.getDescription());
791+
directoryEntry.setDate(historyEntry.getDate());
791792
} else {
792793
LOGGER.log(Level.FINE, "cannot get last history entry for ''{0}''",
793794
directoryEntry.getFile());
@@ -796,8 +797,6 @@ public Map<String, HistoryEntry> getLastHistoryEntries(List<DirectoryEntry> entr
796797
LOGGER.log(Level.FINER, "cannot get last history entry for ''{0}''", directoryEntry.getFile());
797798
}
798799
}
799-
800-
return map;
801800
}
802801

803802
@Override

‎opengrok-indexer/src/main/java/org/opengrok/indexer/history/HistoryCache.java‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@
2424

2525
import java.io.File;
2626
import java.util.List;
27-
import java.util.Map;
2827

2928
import org.jetbrains.annotations.Nullable;
3029
import org.opengrok.indexer.search.DirectoryEntry;
@@ -96,10 +95,9 @@ interface HistoryCache extends Cache {
9695
* specified directory.
9796
*
9897
* @param entries list of {@link DirectoryEntry} instances
99-
* @return a map from file names to {@link HistoryEntry} instance
10098
* @throws CacheException on error
10199
*/
102-
Map<String, HistoryEntry> getLastHistoryEntries(List<DirectoryEntry> entries) throws CacheException;
100+
void fillLastHistoryEntries(List<DirectoryEntry> entries) throws CacheException;
103101

104102
/**
105103
* Clear entry for single file from history cache.

‎opengrok-indexer/src/main/java/org/opengrok/indexer/history/HistoryGuru.java‎

Lines changed: 14 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -711,28 +711,31 @@ public boolean hasAnnotationCacheForFile(File file) {
711711
}
712712

713713
/**
714-
* Get the last modified times for all files and subdirectories in the specified directory.
715-
* If the related {@link Repository} instance does not exist or if it is capable or merge commits,
716-
* however merge commits are disabled in its properties, empty map will be returned.
714+
* Get the last modified times and descriptions for all files and subdirectories in the specified directory.
717715
* @param directory the directory whose files to check
718716
* @param entries list of {@link DirectoryEntry} instances
719-
* @return a map from file names to {@link HistoryEntry} instance for the files that
720-
* the history cache has information about
717+
* @return whether to fall back to file system based time stamps if the date is {@code null}
721718
* @throws org.opengrok.indexer.history.CacheException if history cannot be retrieved
722719
*/
723-
public Map<String, HistoryEntry> getLastHistoryEntries(File directory, List<DirectoryEntry> entries) throws CacheException {
720+
public boolean getLastHistoryEntries(File directory, List<DirectoryEntry> entries) throws CacheException {
724721

725722
if (!env.isUseHistoryCacheForDirectoryListing()) {
726723
LOGGER.log(Level.FINEST, "using history cache to retrieve last modified times for ''{0}}'' is disabled",
727724
directory);
728-
return Collections.emptyMap();
725+
return true;
726+
}
727+
728+
if (!useHistoryCache()) {
729+
LOGGER.log(Level.FINEST, "history cache is disabled for ''{0}'' to retrieve last modified times",
730+
directory);
731+
return true;
729732
}
730733

731734
Repository repository = getRepository(directory);
732735
if (repository == null) {
733736
LOGGER.log(Level.FINEST, "cannot find repository for ''{0}}'' to retrieve last modified times",
734737
directory);
735-
return Collections.emptyMap();
738+
return true;
736739
}
737740

738741
// Do not use history cache for repositories with merge commits disabled as some files in the repository
@@ -742,16 +745,12 @@ public Map<String, HistoryEntry> getLastHistoryEntries(File directory, List<Dire
742745
LOGGER.log(Level.FINEST,
743746
"will not retrieve last modified times due to merge changesets disabled for ''{0}}''",
744747
directory);
745-
return Collections.emptyMap();
748+
return true;
746749
}
747750

748-
if (!useHistoryCache()) {
749-
LOGGER.log(Level.FINEST, "history cache is disabled for ''{0}'' to retrieve last modified times",
750-
directory);
751-
return Collections.emptyMap();
752-
}
751+
historyCache.fillLastHistoryEntries(entries);
753752

754-
return historyCache.getLastHistoryEntries(entries);
753+
return false;
755754
}
756755

757756
/**

‎opengrok-indexer/src/main/java/org/opengrok/indexer/search/DirectoryEntry.java‎

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,14 +18,15 @@
1818
*/
1919

2020
/*
21-
* Copyright (c) 2017, 2018, Oracle and/or its affiliates. All rights reserved.
21+
* Copyright (c) 2017, 2023, Oracle and/or its affiliates. All rights reserved.
2222
* Portions Copyright (c) 2017, 2020, Chris Fraire <cfraire@me.com>.
2323
*/
2424
package org.opengrok.indexer.search;
2525

2626
import org.opengrok.indexer.analysis.NullableNumLinesLOC;
2727

2828
import java.io.File;
29+
import java.util.Date;
2930

3031
/**
3132
* Represents a pairing of {@link File} along with supplemental
@@ -36,6 +37,10 @@ public class DirectoryEntry {
3637
private final File file;
3738
private final NullableNumLinesLOC extra;
3839

40+
private String description;
41+
42+
private Date date;
43+
3944
/**
4045
* Initializes an instance with a specified, required {@link File}.
4146
* @param file a defined instance
@@ -71,4 +76,20 @@ public File getFile() {
7176
public NullableNumLinesLOC getExtra() {
7277
return extra;
7378
}
79+
80+
public String getDescription() {
81+
return description;
82+
}
83+
84+
public void setDescription(String description) {
85+
this.description = description;
86+
}
87+
88+
public Date getDate() {
89+
return date;
90+
}
91+
92+
public void setDate(Date date) {
93+
this.date = date;
94+
}
7495
}

‎opengrok-indexer/src/test/java/org/opengrok/indexer/history/FileHistoryCacheTest.java‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1009,7 +1009,7 @@ void testGetLastHistoryEntry() throws Exception {
10091009
}
10101010

10111011
/**
1012-
* Test {@link FileHistoryCache#getLastHistoryEntries(List)}, in particular that it avoids
1012+
* Test {@link FileHistoryCache#fillLastHistoryEntries(List)}, in particular that it avoids
10131013
* getting history cache entries for directories.
10141014
*/
10151015
@Test
@@ -1036,8 +1036,7 @@ void testGetLastHistoryEntries() throws Exception {
10361036
directoryEntries.add(new DirectoryEntry(file));
10371037
}
10381038

1039-
Map<String, HistoryEntry> historyEntries = spyCache.getLastHistoryEntries(directoryEntries);
1040-
assertNotNull(historyEntries);
1039+
spyCache.fillLastHistoryEntries(directoryEntries);
10411040
Mockito.verify(spyCache, never()).getLastHistoryEntry(ArgumentMatchers.eq(subDir));
10421041

10431042
// Cleanup.

‎opengrok-indexer/src/test/java/org/opengrok/indexer/history/HistoryGuruTest.java‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,6 @@
4444
import java.util.Collection;
4545
import java.util.Collections;
4646
import java.util.List;
47-
import java.util.Map;
4847
import java.util.stream.Collectors;
4948

5049
import org.junit.jupiter.api.AfterAll;
@@ -453,11 +452,11 @@ void testGetLastHistoryEntriesWrtMergeCommits(boolean isMergeCommitsEnabled) thr
453452
}
454453
boolean useHistoryCacheForDirectoryListingOrig = env.isUseHistoryCacheForDirectoryListing();
455454
env.setUseHistoryCacheForDirectoryListing(true);
456-
Map<String, HistoryEntry> historyEntryMap = instance.getLastHistoryEntries(repositoryRoot, directoryEntries);
455+
boolean fallback = instance.getLastHistoryEntries(repositoryRoot, directoryEntries);
457456
if (isMergeCommitsEnabled) {
458-
assertFalse(historyEntryMap.isEmpty());
457+
assertFalse(fallback);
459458
} else {
460-
assertTrue(historyEntryMap.isEmpty());
459+
assertTrue(fallback);
461460
}
462461

463462
// Cleanup.

‎opengrok-web/src/main/java/org/opengrok/web/DirectoryListing.java‎

Lines changed: 19 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -30,10 +30,8 @@
3030
import java.text.Format;
3131
import java.text.SimpleDateFormat;
3232
import java.util.ArrayList;
33-
import java.util.Date;
3433
import java.util.List;
3534
import java.util.Locale;
36-
import java.util.Map;
3735
import java.util.logging.Level;
3836
import java.util.logging.Logger;
3937
import java.util.stream.Collectors;
@@ -44,7 +42,6 @@
4442
import org.opengrok.indexer.configuration.PathAccepter;
4543
import org.opengrok.indexer.configuration.RuntimeEnvironment;
4644
import org.opengrok.indexer.history.CacheException;
47-
import org.opengrok.indexer.history.HistoryEntry;
4845
import org.opengrok.indexer.history.HistoryGuru;
4946
import org.opengrok.indexer.logger.LoggerFactory;
5047
import org.opengrok.indexer.search.DirectoryEntry;
@@ -58,7 +55,7 @@ public class DirectoryListing {
5855

5956
private static final Logger LOGGER = LoggerFactory.getLogger(DirectoryListing.class);
6057

61-
protected static final String DIRECTORY_BLANK_PLACEHOLDER = "-";
58+
protected static final String BLANK_PLACEHOLDER = "-";
6259
private final EftarFileReader desc;
6360
private final long now;
6461

@@ -74,21 +71,19 @@ public DirectoryListing(EftarFileReader desc) {
7471

7572
/**
7673
* Write part of HTML code which contains file/directory last modification time and size.
77-
*
74+
* The size printed for directories will be always {@link #BLANK_PLACEHOLDER}.
75+
* The time printed will be string representation of the non {@code null} time or {@link #BLANK_PLACEHOLDER}.
7876
* @param out write destination
7977
* @param file the file or directory to use for writing the data
80-
* @param modTime the time of the last commit that touched {@code file} or {@code null} if unknown
78+
* @param lastModTime the time of the last commit that touched {@code file} or {@code null} if unknown
8179
* @param dateFormatter the formatter to use for pretty printing dates
8280
*
83-
* @throws IOException when cannot read last modified time from file system
81+
* @throws IOException when writing to the {@code out} parameter failed
8482
*/
85-
private void printDateSize(Writer out, File file, Date modTime, Format dateFormatter) throws IOException {
86-
long lastModTime = modTime == null ? file.lastModified() : modTime.getTime();
87-
83+
private void printDateSize(Writer out, File file, Long lastModTime, Format dateFormatter) throws IOException {
8884
out.write("<td>");
89-
if (modTime == null && RuntimeEnvironment.getInstance().isUseHistoryCacheForDirectoryListing() &&
90-
file.isDirectory()) {
91-
out.write(DIRECTORY_BLANK_PLACEHOLDER);
85+
if (lastModTime == null) {
86+
out.write(BLANK_PLACEHOLDER);
9287
} else {
9388
if (now - lastModTime < 86400000) {
9489
out.write("Today");
@@ -98,7 +93,7 @@ private void printDateSize(Writer out, File file, Date modTime, Format dateForma
9893
}
9994
out.write("</td><td>");
10095
if (file.isDirectory()) {
101-
out.write(DIRECTORY_BLANK_PLACEHOLDER);
96+
out.write(BLANK_PLACEHOLDER);
10297
} else {
10398
out.write(Util.readableSize(file.length()));
10499
}
@@ -217,8 +212,7 @@ public List<String> extraListTo(String contextPath, File dir, Writer out,
217212
out.write("</tr>\n");
218213
}
219214

220-
Map<String, HistoryEntry> lastHistoryEntriesMap
221-
= HistoryGuru.getInstance().getLastHistoryEntries(dir, entries);
215+
boolean fallback = HistoryGuru.getInstance().getLastHistoryEntries(dir, entries);
222216

223217
if (entries != null) {
224218
for (DirectoryEntry entry : entries) {
@@ -230,7 +224,6 @@ public List<String> extraListTo(String contextPath, File dir, Writer out,
230224
readMes.add(filename);
231225
}
232226
boolean isDir = child.isDirectory();
233-
HistoryEntry historyEntry = lastHistoryEntriesMap.get(filename);
234227

235228
out.write("<tr><td>");
236229
out.write("<p class=\"");
@@ -254,10 +247,10 @@ public List<String> extraListTo(String contextPath, File dir, Writer out,
254247
} else {
255248
out.write(Util.uriEncodePath(filename));
256249
out.write("\"");
257-
if (historyEntry != null) {
250+
if (entry.getDescription() != null) {
258251
out.write(" class=\"title-tooltip\"");
259252
out.write(" title=\"");
260-
out.write(Util.encode(historyEntry.getDescription()));
253+
out.write(Util.encode(entry.getDescription()));
261254
out.write("\"");
262255
}
263256
out.write(">");
@@ -266,7 +259,13 @@ public List<String> extraListTo(String contextPath, File dir, Writer out,
266259
}
267260
out.write("</td>");
268261
Util.writeHAD(out, contextPath, path + filename);
269-
printDateSize(out, child, historyEntry != null ? historyEntry.getDate() : null, dateFormatter);
262+
Long date = null;
263+
if (entry.getDate() != null) {
264+
date = entry.getDate().getTime();
265+
} else if (fallback) {
266+
date = child.lastModified();
267+
}
268+
printDateSize(out, child, date, dateFormatter);
270269
printNumlines(out, entry, isDir);
271270
printLoc(out, entry, isDir);
272271
if (offset > 0) {

‎opengrok-web/src/test/java/org/opengrok/web/DirectoryListingTest.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -300,7 +300,7 @@ private long getDateValue(Node item) throws Exception {
300300
String value = firstChild.getNodeValue();
301301
if (RuntimeEnvironment.getInstance().isUseHistoryCacheForDirectoryListing()) {
302302
// Assumes that the history cache was created.
303-
assertEquals(DirectoryListing.DIRECTORY_BLANK_PLACEHOLDER, value);
303+
assertEquals(DirectoryListing.BLANK_PLACEHOLDER, value);
304304
return DIRECTORY_INTERNAL_DATE;
305305
}
306306

@@ -321,7 +321,7 @@ private int getSize(Node item) throws NumberFormatException {
321321
Node val = item.getFirstChild();
322322
assertNotNull(val);
323323
assertEquals(Node.TEXT_NODE, val.getNodeType());
324-
if (DirectoryListing.DIRECTORY_BLANK_PLACEHOLDER.equals(val.getNodeValue().trim())) {
324+
if (DirectoryListing.BLANK_PLACEHOLDER.equals(val.getNodeValue().trim())) {
325325
// track that it had the DIRECTORY_SIZE_PLACEHOLDER character
326326
return DIRECTORY_INTERNAL_SIZE;
327327
}

0 commit comments

Comments
 (0)