RFR: 2084: Add a `/branch` command similar to `/tag` to create a new branch from a commit

[Erik Joelsson]

On Fri, 3 Nov 2023 18:14:07 GMT, Erik Duveblad <ehelin at openjdk.org> wrote:

> Hi all,
> 
> please review this patch that adds a `/branch` commit command. The `/branch` commit command allows an integrator to create a _new_ branch in a repository by adding the comment `/branch <name>` on a commit's web page. The command will check that no branch with the given name exists (i.e. it won't update a branch with that name to that commit).
> 
> The implementation is very similar to the `/tag` command, but I don't think it makes sense to try to share code between them. They are both only around 60 lines of code, so trying to share code won't give us much.
> 
> I also added a couple of new unit tests.
> 
> Thanks,
> Erik

bots/pr/src/main/java/org/openjdk/skara/bots/pr/BranchCommand.java line 102:

> 100:             localRepo.fetch(bot.repo().authenticatedUrl(), commit.hash().toString(), true);
> 101: 
> 102:             var remoteBranches = localRepo.remoteBranches(bot.repo().authenticatedUrl().toString());

I'm not sure this will contain all branches. We have had problems with repos materialized from HostedRepositoryPool, probably a bug in there (specifically in refreshSeed). Most uses fetch explicitly after to work around it, which you do here, but this usecase is different because we need every branch to be present, not just the main commit.

Instead of working around it here, it would be better to fix HostedRepositoryPool before integrating this.

If we could trust materialize, then the fetch after wouldn't be needed.

bots/pr/src/main/java/org/openjdk/skara/bots/pr/BranchCommand.java line 110:

> 108:                 if (ref.isEmpty()) {
> 109:                     throw new IllegalStateException("Cannot find hash for remote branch '" + branchName + "'");
> 110:                 }

I think this turned out a little awkward, especially the exception that should never actually happen.
Suggestion:

            var existingBranch = remoteBranches.stream()
                    .filter(b -> b.name().equals(branchName))
                    .findAny();
            if (existingBranch.isPresent()) {

bots/pr/src/main/java/org/openjdk/skara/bots/pr/BranchCommand.java line 127:

> 125:             log.info("Pushing branch '" + branch + "' to refer to commit: " + commit.hash().hex());
> 126:             localRepo.push(commit.hash(), bot.repo().authenticatedUrl(), branch.name(), false, false);
> 127:             log.info(String.join(", ", localRepo.branches().stream().map(Branch::name).collect(Collectors.toList())));

This will potentially be a rather long log message. Not sure we need to list all branches in the repo.

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

PR Review Comment: https://git.openjdk.org/skara/pull/1583#discussion_r1382168556
PR Review Comment: https://git.openjdk.org/skara/pull/1583#discussion_r1382176611
PR Review Comment: https://git.openjdk.org/skara/pull/1583#discussion_r1382173346