RFR: 1296: ArchiveWorkItem throws repeated exceptions after force push to PR

[Kevin Rushforth]

On Wed, 22 Dec 2021 19:21:31 GMT, Erik Joelsson <erikj at openjdk.org> wrote:

> The mlbridge bot sometimes fails handling of force-pushed changes to a PR. This patch tries to fix this.
> 
> When the previous head hash of a PR has been replaced with a force push, that hash will not automatically be present in the local repository that the bot uses to generate diffs. Sometimes it will be present if the bot happens to execute in the same scratch area as where the PR was processed before. Before this patch, the code assumes that the previous hash is present in the local repo.
> 
> I'm attacking this from two sides. I'm adding a method that tries to pull in the missing hash if needed. This may or may not succeed depending on if the server allows pulling of arbitrary hashes or not (my local git does not, but I believe github does). It can also fail if the hash has been GCd on the server. If the hash is, or has been made present in the local repo, we can proceed as before. If getting the hash was unsuccessful, I'm changing the email message to explain that incremental views of the changes are unavailable, and we only try to produce the full webrev.
> 
> To test this properly, I tried to adapt an existing test. However, while working with that test, I discovered that it wasn't really doing what (at least I) expected it to do. As I explained in a bug comment, TestCredentials::getHostedRepository, will return HostedRepository objects with the same backing Git repository in all of them. This was causing weird and unexpected behavior as changes for the main PR repo as well as the webrev and mail archive repos all ended up jumbled in the same repository, making it impossible to actually control the setup for the test. To handle this I added the ability to create multiple named repositories for a TestHost, and use it in the two tests affected by this change. I also found two other tests that relied on there only being one repo per TestHost (and fixed them). I believe a followup is necessary to try to fix the remaining tests for at least mlbridge as I suspect more of them are likely to have issues.
> 
> I added caching of the supplier results in ArchiveItem. The suppliers here are potentially calling out to remote URLs as well as running multiple git commands to generate the body and footer. I noticed that at least the body() method was called twice, so figured it would be good to only make these costly operations once.

Looks good.

-------------

Marked as reviewed by kcr (Reviewer).

PR: https://git.openjdk.java.net/skara/pull/1267