Skip to content

Commit

Permalink
Merge changes I7120b952,Ie82550fe,I83235c3e,I9319d95c,I4fdfcf19, ...
Browse files Browse the repository at this point in the history
* changes:
  AutoMerger: Fix log level
  DiffOperationsImpl#toMap: Return an immutable map
  ModifiedFilesCacheImpl#getIfPreset: Remove unneeded try-catch block
  ModifiedFilesLoader: Use immutable type instead of general interface type
  ModifiedFilesLoader: Migrate (via inlining) away from Streams.stream()
  ModifiedFilesLoader: Make gitModifiedFilesCache field final
  ModifiedFilesLoader: Improve sorting of modified files
  ModifiedFilesLoader/GitModifiedFilesLoader: Fix @nullable annotation
  CommitValidationListener: Add warning to not use DiffOperations
  ModifiedFilesLoader: Fix common typo
  • Loading branch information
EdwinKempin authored and Gerrit Code Review committed Jan 15, 2024
2 parents 6f3db39 + 4ad73c1 commit 762b64a
Show file tree
Hide file tree
Showing 6 changed files with 24 additions and 25 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,11 @@
*
* <p>Invoked by Gerrit when a new commit is received, has passed basic Gerrit validation and can be
* then subject to extra validation checks.
*
* <p>Do not use {@link com.google.gerrit.server.patch.DiffOperations} from {@code
* CommitValidationListener} implementations to get the modified files for the received commit,
* instead use {@link com.google.gerrit.server.patch.DiffOperationsForCommitValidation} that is
* provided in {@link CommitReceivedEvent#diffOperations}.
*/
@ExtensionPoint
public interface CommitValidationListener {
Expand Down
2 changes: 1 addition & 1 deletion java/com/google/gerrit/server/patch/AutoMerger.java
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ private ObjectId createAutoMergeCommit(
ObjectId treeId;
if (couldMerge) {
treeId = m.getResultTreeId();
logger.atSevere().log(
logger.atFine().log(
"AutoMerge treeId=%s (no conflicts, inserter: %s, caller: %s)",
treeId.name(), m.getObjectInserter(), callerFinder.findCallerLazy());
} else {
Expand Down
3 changes: 2 additions & 1 deletion java/com/google/gerrit/server/patch/DiffOperationsImpl.java
Original file line number Diff line number Diff line change
Expand Up @@ -453,7 +453,8 @@ private Map<String, ModifiedFile> loadModifiedFilesWithoutCacheIfNecessary(
return modifiedFiles;
}

private static Map<String, ModifiedFile> toMap(ImmutableList<ModifiedFile> modifiedFiles) {
private static ImmutableMap<String, ModifiedFile> toMap(
ImmutableList<ModifiedFile> modifiedFiles) {
return modifiedFiles.stream()
.collect(
toImmutableSortedMap(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,13 +93,8 @@ public ImmutableList<ModifiedFile> get(ModifiedFilesCacheKey key)
}
}

public Optional<ImmutableList<ModifiedFile>> getIfPresent(ModifiedFilesCacheKey key)
throws DiffNotAvailableException {
try {
return Optional.ofNullable(cache.getIfPresent(key));
} catch (Exception e) {
throw new DiffNotAvailableException(e);
}
public Optional<ImmutableList<ModifiedFile>> getIfPresent(ModifiedFilesCacheKey key) {
return Optional.ofNullable(cache.getIfPresent(key));
}

public void put(ModifiedFilesCacheKey key, ImmutableList<ModifiedFile> modifiedFiles) {
Expand Down
28 changes: 13 additions & 15 deletions java/com/google/gerrit/server/patch/diff/ModifiedFilesLoader.java
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Sets;
import com.google.common.collect.Streams;
import com.google.common.flogger.FluentLogger;
import com.google.errorprone.annotations.CanIgnoreReturnValue;
import com.google.gerrit.common.Nullable;
Expand Down Expand Up @@ -57,7 +56,7 @@
* Factory#createWithRetrievingModifiedFilesForTreesFromGitModifiedFilesCache()} in which case the
* trees are looked up via a new {@link RevWalk} instance that is created by {@code
* GitModifiedFilesCacheImpl.Loader}. Looking up the trees from a new {@link RevWalk} instance only
* succeeds if they were already fully persisted in the repository, i.e. if these are not newly
* succeeds if they were already fully persisted in the repository, i.e., if these are not newly
* created trees or tree which have been created in memory. This means using the {@link
* GitModifiedFilesCache} is expected to cause {@link MissingObjectException}s for the commit trees
* that are newly created or that were created in memory only.
Expand All @@ -67,7 +66,7 @@ public class ModifiedFilesLoader {

@Singleton
public static class Factory {
private GitModifiedFilesCache gitModifiedFilesCache;
private final GitModifiedFilesCache gitModifiedFilesCache;

@Inject
Factory(GitModifiedFilesCache gitModifiedFilesCache) {
Expand All @@ -93,7 +92,7 @@ public ModifiedFilesLoader create() {
* GitModifiedFilesCacheImpl.Loader}), and not by the {@link RevWalk} instance that is given to
* the {@link #load(com.google.gerrit.entities.Project.NameKey, Config, RevWalk, ObjectId,
* ObjectId)} method. Looking up the trees from a new {@link RevWalk} instance only succeeds if
* they were already fully persisted in the repository, i.e. if these are not newly created
* they were already fully persisted in the repository, i.e., if these are not newly created
* trees or tree which have been created in memory. This means using the {@link
* GitModifiedFilesCache} is expected to cause {@link MissingObjectException}s for the commit
* trees that are newly created or that were created in memory only. Also see the javadoc on
Expand All @@ -104,8 +103,9 @@ ModifiedFilesLoader createWithRetrievingModifiedFilesForTreesFromGitModifiedFile
}
}

private @Nullable Integer renameScore = null;
private @Nullable GitModifiedFilesCache gitModifiedFilesCache;
@Nullable private final GitModifiedFilesCache gitModifiedFilesCache;

@Nullable private Integer renameScore = null;

ModifiedFilesLoader(@Nullable GitModifiedFilesCache gitModifiedFilesCache) {
this.gitModifiedFilesCache = gitModifiedFilesCache;
Expand Down Expand Up @@ -147,12 +147,11 @@ public ImmutableList<ModifiedFile> load(
: DiffUtil.getTreeId(revWalk, baseCommit);
ObjectId newTree = DiffUtil.getTreeId(revWalk, newCommit);
ImmutableList<ModifiedFile> modifiedFiles =
DiffUtil.mergeRewrittenModifiedFiles(
ImmutableList.sortedCopyOf(
comparing(f -> f.getDefaultPath()),
DiffUtil.mergeRewrittenModifiedFiles(
getModifiedFiles(
project, repoConfig, revWalk.getObjectReader(), baseTree, newTree))
.stream()
.sorted(comparing(f -> f.getDefaultPath()))
.collect(toImmutableList());
project, repoConfig, revWalk.getObjectReader(), baseTree, newTree)));
if (baseCommit.equals(ObjectId.zeroId())) {
return modifiedFiles;
}
Expand Down Expand Up @@ -198,14 +197,14 @@ private Set<String> getTouchedFilesWithParents(
ObjectId parentOfNew)
throws IOException {
try {
List<ModifiedFile> oldVsBase =
ImmutableList<ModifiedFile> oldVsBase =
getModifiedFiles(
project,
repoConfig,
revWalk.getObjectReader(),
DiffUtil.getTreeId(revWalk, parentOfBase),
DiffUtil.getTreeId(revWalk, baseCommit));
List<ModifiedFile> newVsBase =
ImmutableList<ModifiedFile> newVsBase =
getModifiedFiles(
project,
repoConfig,
Expand Down Expand Up @@ -255,8 +254,7 @@ private ImmutableList<ModifiedFile> getModifiedFiles(

private ImmutableSet<String> getOldAndNewPaths(List<ModifiedFile> files) {
return files.stream()
.flatMap(
file -> Stream.concat(Streams.stream(file.oldPath()), Streams.stream(file.newPath())))
.flatMap(file -> Stream.concat(file.oldPath().stream(), file.newPath().stream()))
.collect(ImmutableSet.toImmutableSet());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ public class GitModifiedFilesLoader {
DiffEntry.ChangeType.COPY,
Patch.ChangeType.COPIED);

private @Nullable Integer renameScore = null;
@Nullable private Integer renameScore = null;

/**
* Enables rename detection
Expand Down

0 comments on commit 762b64a

Please sign in to comment.