RFR: 2084: Add a `/branch` command similar to `/tag` to create a new branch from a commit [v2]
Erik Duveblad
ehelin at openjdk.org
Mon Nov 6 14:28:01 UTC 2023
On Fri, 3 Nov 2023 20:27:03 GMT, Erik Joelsson <erikj at openjdk.org> wrote:
>> Erik Duveblad has updated the pull request incrementally with one additional commit since the last revision:
>>
>> Reviewer feedback
>
> 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.
We actually don't need the `fetch` (we are about to push a new ref), I just left it there to "follow the pattern". I agree that we should fix the problem `HostedRepositoryPool`, but I would prefer to integrate this first, just to get if off my plate (there is no reason to hold this PR hostage over that bug 😄).
I opted to use `HostedRepository::branches` in the latest version which hopefully will make the code a bit clearer.
-------------
PR Review Comment: https://git.openjdk.org/skara/pull/1583#discussion_r1383422892
More information about the skara-dev
mailing list