[Approved] RFR: git-sync should use APIs to find upstream

[Erik Helin]

On Thu, 21 Nov 2019 14:00:13 GMT, Robin Westberg <rwestberg at openjdk.org> wrote:

> On Thu, 21 Nov 2019 10:48:29 GMT, Erik Helin <ehelin at openjdk.org> wrote:
> 
>> Hi all,
>> 
>> this patch changes `git-sync` to to use the `Forge` APIs for finding the upstream repository for a personal fork (in case the `upstream` remote is missing). The big part of this change is making `Forge.repository` return `Optional<HostedRepository>` instead of `HostedRepository` (because a forge may not have the requested repository). Robin, what do you think about these changes?
>> 
>> Thanks,
>> Erik
>> 
>> ## Testing
>> - [x] Manual testing of `git sync`, both with and without an `upstream` remote
>> 
>> ----------------
>> 
>> Commits:
>>  - 20e4e200: git-sync should use APIs to find upstream
>> 
>> Changes: https://git.openjdk.java.net/skara/pull/262/files
>>  Webrev: https://webrevs.openjdk.java.net/skara/262/webrev.00
>>   Stats: 70 lines in 15 files changed: 44 ins; 0 del; 26 mod
>>   Patch: https://git.openjdk.java.net/skara/pull/262.diff
>>   Fetch: git fetch https://git.openjdk.java.net/skara pull/262/head:pull/262
> 
> Looks good, just have one comment!
> 
> bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java line 141:
> 
>> 140:                         var mergeSourceRepo = pr.repository().forge().repository(sourceRepo.get()).get();
>> 141:                         try {
>> 142:                             var sourceHash = prInstance.localRepo().fetch(mergeSourceRepo.url(), sourceBranch.get());
> 
> Perhaps this (and the other similar ones) could be replaced with `.orElseThrow()` instead? A few are written like this already.
> 
> ----------------
> 
> Approved by rwestberg (Reviewer).

Yeah, I agree. Although the effect of a failed `.get()` will be similar (a `NoSuchElementException` will be thrown), I agree that `.orElseThrow()` expresses the intent more clearly to the reader.

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