tree 24f276502a0100f9a432fb11482ecf69634f3a08
parent 6f34d005235952195b3c311ac986bffb553b868d
author Alice Kober-Sotzek <aliceks@google.com> 1600097308 +0200
committer Alice Kober-Sotzek <aliceks@google.com> 1600423366 +0200

CommentsUtil: Don't use the diff cache to look up the parent commit

It's a bit strange that we used the diff caches to lookup a specific
parent commit (e.g. first parent, second parent, auto-merge) of a
patchset even though we weren't interested in the diff at all.

One reason might be that auto-merge commits are only reliably available
within the diff cache. Furthermore, the diff result is cached and hence
quickly available on subsequent calls.

Besides being confusing and possibly doing more than necessary (e.g.
computing actual diffs), this approach has another major disadvantage:
It returns the wrong commit SHA-1 if the specified 'side' doesn't exist
for the commit (e.g. if the second parent is specified but the patchset
isn't a merge commit). In such a case, the diff cache falls back to the
first parent commit without further notice. That's problematic if you're
relying on the returned commit SHA-1 to be correct as we do for ported
comments.

A better approach than using the diff cache is to directly lookup the
desired parent commit. One possible disadvantage of this approach is
that we always need to open the repository to do so.

The new approach also lets callers decide how to react to a non-existent
parent commit as it returns an Optional.empty() instead of directly
throwing an exception. Callers may choose to ignore such a parent or
throw an exception of a type and message of their choice.

For auto-merge commits, we still need to use the old approach. With the
redesigned diff caches, we should be able to switch to a different
approach, too.

Change-Id: Iad263b16c45b7c0d826b98aeb8f8865296326bf7