RFR: 1813: URLs printed by Skara should have ".git" at the end of the repo name [v7]

[Erik Joelsson]

On Fri, 10 Feb 2023 17:58:47 GMT, Zhao Song <zsong at openjdk.org> wrote:

>> In this patch, `.git` is added to the URLs printed by Skara.
>
> Zhao Song has updated the pull request incrementally with one additional commit since the last revision:
> 
>   rename remoteUrl and url

This turned into a big patch. I looked through all the `url()` -> `authenticatedUrl()` changes and found some that should be reverted or changed in some way.

bots/checkout/src/main/java/org/openjdk/skara/bots/checkout/CheckoutBot.java line 69:

> 67:         if (!uri.endsWith(".git")) {
> 68:             uri += ".git";
> 69:         }

This whole method is also redundant and should just be eliminated.

bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/CensusInstance.java line 85:

> 83: 
> 84:     static CensusInstance create(HostedRepository censusRepo, String censusRef, Path folder, PullRequest pr) {
> 85:         var repoName = censusRepo.authenticatedUrl().getHost() + "/" + censusRepo.name();

Here we should probably just call `.url()`.

bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/MailingListArchiveReaderBot.java line 117:

> 115:             resolvedPullRequests.put(first.id(), pr);
> 116:         }
> 117:         var bridgeIdPattern = Pattern.compile("^[^.]+\\.[^.]+@" + pr.repository().authenticatedUrl().getHost() + "$");

I think this should be `.url()`.

bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/ReviewArchive.java line 299:

> 297:             var encodedCommon = Base64.getUrlEncoder().encodeToString(digest.digest());
> 298: 
> 299:             return EmailAddress.from(encodedCommon + "." + UUID.randomUUID() + "@" + pr.repository().authenticatedUrl().getHost());

Another `.url()`.

bots/notify/src/test/java/org/openjdk/skara/bots/notify/issue/IssueNotifierTests.java line 1887:

> 1885:             // Verify that the 'original' repo URL is used in the comment and not the main one
> 1886:             assertTrue(comment.body().contains(originalRepo.authenticatedUrl().toString()));
> 1887:             assertFalse(comment.body().contains(repo.authenticatedUrl().toString()));

I think we should be checking for `.url()` here.

bots/notify/src/test/java/org/openjdk/skara/bots/notify/issue/IssueNotifierTests.java line 1989:

> 1987:             var comment = comments.get(0);
> 1988:             assertTrue(comment.body().contains(editHash.toString()));
> 1989:             assertTrue(comment.body().contains(repo.authenticatedUrl().toString()));

Same here.

bots/notify/src/test/java/org/openjdk/skara/bots/notify/issue/IssueNotifierTests.java line 2037:

> 2035:             var comment = comments.get(0);
> 2036:             assertTrue(comment.body().contains(editHash.toString()));
> 2037:             assertTrue(comment.body().contains(repo.authenticatedUrl().toString()));

And again.

bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java line 1216:

> 1214:             if (pr.sourceRepository().isPresent()) {
> 1215:                 var branchNames = pr.repository().branches().stream().map(HostedBranch::name).collect(Collectors.toSet());
> 1216:                 if (!pr.repository().authenticatedUrl().equals(pr.sourceRepository().get().authenticatedUrl()) && branchNames.contains(pr.sourceRef())) {

I think we should be comparing `.url()` here. If we compare `authenticatedUrl()` there is a risk that different user authentications would trigger a false negative.

bots/pr/src/main/java/org/openjdk/skara/bots/pr/LimitedCensusInstance.java line 117:

> 115: 
> 116:     private static Path getRepoFolder(HostedRepositoryPool hostedRepositoryPool, HostedRepository censusRepo, String censusRef, Path folder) {
> 117:         var repoName = censusRepo.authenticatedUrl().getHost() + "/" + censusRepo.name();

Only need `.url()` to extract host.

bots/tester/src/test/java/org/openjdk/skara/bots/tester/InMemoryHostedRepository.java line 239:

> 237:     @Override
> 238:     public URI url() {
> 239:         return null;

It probably doesn't matter much, but seems like this should return `url` just like `authenticatedUrl()` does.

cli/src/main/java/org/openjdk/skara/cli/pr/GitPrCreate.java line 308:

> 306: 
> 307:         var mailingLists = new ArrayList<String>();
> 308:         var parentProject = ForgeUtils.projectName(parentRepo.authenticatedUrl());

Should probably be `.url()`.

forge/src/main/java/org/openjdk/skara/forge/gitlab/GitLabMergeRequest.java line 533:

> 531: 
> 532:     private String linkToDiff(String path, Hash hash, int line) {
> 533:         return "[" + path + " line " + line + "](" + URIBuilder.base(repository.authenticatedUrl())

This looks like another case of `.url()`. The authentication is even removed below. This string is built to be presented as text somewhere.

test/src/main/java/org/openjdk/skara/test/TestHostedRepository.java line 59:

> 57:         this.projectName = projectName;
> 58:         this.localRepository = localRepository;
> 59:         pullRequestPattern = Pattern.compile(authenticatedUrl().toString() + "/pr/" + "(\\d+)");

Doesn't matter much as they resolve to the same value, but logically I think `.webUrl()` fits better here.

test/src/main/java/org/openjdk/skara/test/TestPullRequest.java line 217:

> 215:     public URI webUrl() {
> 216:         try {
> 217:             return new URI(targetRepository.authenticatedUrl().toString() + "/pr/" + id());

Logically this should be `.webUrl()`.

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

PR: https://git.openjdk.org/skara/pull/1467