Qualify the test branch by PR id in order to prepare for a future change where we will have multiple PR ids

The PR id might not uniquely identify the thing you are building. Suppose we have PRs A and B, we build a train master <- A <- B, build A and B, but now A fails. Then we want to do master <- B, a different commit. I think if the strategy is to only build sequentially, and you don’t care about results of the pending builds on top of a failed build, then every pull request will occur at most once at the tip of a build, and it still works. But if you want to “hedge the bet” (e.g. build both master <- A <- B, and also master <- B, so that if A fails, then next best alternative is already in progress), a pull request can be at the tip of multiple builds.

One could think of mitigating this by having a small set of testing slots

Yeah, I think you will need this either way. The more parallel builds you speculatively start, the lower the return on investment, because the longer the train is, the more likely it is to fail. And if you don’t limit parallelism in Hoff but instead let the CI provider handle that, then priority is hard to control. I would go with the build slots, and make the amount configurable.

testing/1

Keep in mind that the branch testing and testing/1 cannot coexist, because Git tracks branches as files under .git/refs, and .git/refs/remotes/origin/testing cannot simultaneously be a file and a directory. When the naming scheme changes, it will be a one-time nuisance for all people who pull from the repository and who might have a testing (remote tracking) branch locally, and the error can be puzzling if you’re not aware of this implementation detail leak.

Qualify the test branch by PR id in order to prepare for a future change where we will have multiple PR ids

The PR id might not uniquely identify the thing you are building. Suppose we have PRs A and B, we build a train master <- A <- B, build A and B, but now A fails. Then we want to do master <- B, a different commit. I think if the strategy is to only build sequentially, and you don’t care about results of the pending builds on top of a failed build, then every pull request will occur at most once at the tip of a build, and it still works. But if you want to “hedge the bet” (e.g. build both master <- A <- B, and also master <- B, so that if A fails, then next best alternative is already in progress), a pull request can be at the tip of multiple builds.

The current idea is to build only:

• master <- A
• master <- A <- B
• master <- A <- B <- C

The following will only start building once A fails (and consequently B and C, since the current plan does not allow merging further PRs on top of a failing PR):

• master <- B
• master <- B <- C

In this way the PR id still uniquely identifies the build.

In the future, if we decide to move towards more speculative builds I would rather use testing/1-2-3 to signify three built on top of two built on top of one.

Yeah, I think you will need this either way. The more parallel builds you speculatively start, the lower the return on investment, because the longer the train is, the more likely it is to fail. And if you don’t limit parallelism in Hoff but instead let the CI provider handle that, then priority is hard to control. I would go with the build slots, and make the amount configurable.

See the idea above: testing/1-2-3.

Keep in mind that the branch testing and testing/1 cannot coexist, because Git tracks branches as files under .git/refs, and .git/refs/remotes/origin/testing cannot simultaneously be a file and a directory. When the naming scheme changes, it will be a one-time nuisance for all people who pull from the repository and who might have a testing (remote tracking) branch locally, and the error can be puzzling if you’re not aware of this implementation detail leak.

I am aware of that, the current implementation already takes this into account, see: 503461e. The testing branch is deleted before the creation of any testing/<id> branch. That's also listed in the TODO list of #77:

• fix issue of pushing testing/<id> in the presence of testing

The testing branch is deleted before the creation of any testing/ branch

Yes, but it will also be an issue for everybody who has cloned the repository and who has a remote-tracking branch (which is probably going to be everybody who uses that repository), it will make git fetch complain until the local ref is deleted:

$ git fetch
error: cannot lock ref 'refs/remotes/origin/testing/1': 'refs/remotes/origin/testing' exists; cannot create 'refs/remotes/origin/testing/1'
From /tmp/upstream
 ! [new branch]      testing/1  -> origin/testing/1  (unable to update local ref)

It’s nothing that an announcement can’t fix, I just wanted to point it out.

The testing branch is deleted before the creation of any testing/ branch

Yes, but it will also be an issue for everybody who has cloned the repository and who has a remote-tracking branch (which is probably going to be everybody who uses that repository), it will make git fetch complain until the local ref is deleted:

$ git fetch
error: cannot lock ref 'refs/remotes/origin/testing/1': 'refs/remotes/origin/testing' exists; cannot create 'refs/remotes/origin/testing/1'
From /tmp/upstream
 ! [new branch]      testing/1  -> origin/testing/1  (unable to update local ref)

It’s nothing that an announcement can’t fix, I just wanted to point it out.

@ruuda This makes sense, noted.

I haven't tested this, but I guess this can be (manually) fixed in two ways:

1. git fetch --prune
2. git branch -rd origin/testing && git fetch

I'll check whether either/both work and include these in an announcement accordingly.

LGTM for now - But I'll have a final look once it has been rebased after the other relevant PRs have been merged.

@fatho The other relevant PRs have been merged and I rebased on top of the new master. Can you please take a look again?

@fatho Thanks for the review.

I will wait until I deploy v0.25.9 before I merge this.

I performed a trivial rebase on top of master which was updated with a change in 3 lines of code. There were no conflicts:

rudy@dev-lt-55:~/channable/hoff$ git l ..master
2022-07-28 10:25:34 +0200  2cd75b0  Rudy Matela  Hoff v0.25.9   (tag: v0.25.9, origin/master, origin/HEAD, master)
2022-07-28 10:01:07 +0200  8e2ec1e  OpsBotPrime  Merge #145: Ignore duplicate build status webhooks   (origin/testing)
2022-07-28 09:45:02 +0200  6017ef6  Rudy Matela  Ignore duplicate build status hooks. (fix tests)  
2022-07-28 09:42:08 +0200  fac73f9  Rudy Matela  test/Spec: test duplicate GH hooks (break tests)  

rudy@dev-lt-55:~/channable/hoff$ git rebase master
First, rewinding head to replay your work on top of it...
Applying: Pass PullRequestId to tryIntegrate
Applying: Qualify testing branch name by PullRequestId
Applying: Add (unused) DeleteBranch operation
Applying: fixup! Add (unused) DeleteBranch operation
Applying: initial version of test branch cleaning up
Applying: src/Git: rename DeleteBranch to DeleteRemoteBranch
Applying: add and use the deleteBranch operation
Applying: refactor: add and use Logic.testBranch (DRY code)
Applying: fix bug in deleteRemoteBranch
Applying: src/Logic: delete unqualified testing branch
Applying: fix failing tests: cleanup of integration branches
Applying: EventLoopSpec: add test for a failing build
Applying: fix build of tests/Spec.hs

rudy@dev-lt-55:~/channable/hoff$ git push --force-with-lease
...
To github.com:channable/hoff
 + e7880bc...746472e merge-train/isolate-test-branches -> merge-train/isolate-test-branches (forced update)

... so I don't think there's need to review again. I would let @OpsBotPrime deal with this, but I need to rebase #137 on top of this very PR, so I might as well rebase on top of something that includes the latest master commit.

I tested this in conjunction with GitHub again just to be sure and all seems to be working. It's time to prepare the next deployment again. @OpsBotPrime merge on Friday.

Pull request approved for merge by @rudymatela, rebasing now.

Rebased as 9cbd57e, waiting for CI …

Waiting on CI job: https://channable.semaphoreci.com/workflows/5c426238-cd7d-478a-8bd9-9c8bad8e6bbe?pipeline_id=b7fdb860-0ddc-4c33-9843-9cab068ea3a3

Here is the message I will send to the users.

Manual intervention required for @OpsBotPrime (Hoff v0.26.0)

Hoff v0.26.0 builds PRs on separate testing branches qualified by PR# (testing/360, testing/1337, etc). After Hoff starts a testing build, you may get the following error when fetching or pulling from the remote repository:

$ git fetch
error: cannot lock ref 'refs/remotes/origin/testing/109': 'refs/remotes/origin/testing' exists; cannot create 'refs/remotes/origin/testing/109'
From github.com:organization/repository
 ! [new branch]      rudy-testing/109 -> origin/rudy-testing/109  (unable to update local ref)
error: some local refs could not be updated; try running
 'git remote prune origin' to remove any old, conflicting branches

If that happens, simply follow the instruction by git to prune the conflicting branch:

$ git remote prune origin
Pruning origin
URL: git@github.com:channable/git-sandbox
 * [pruned] origin/rudy-testing

Then pull or fetch again:

$ git fetch

Alternatively you can use git fetch --prune.

Unfortunately, this will happen once per repository. To fix the issue for all repositories that have been built by Hoff already, you can do:

$ cd <workspace>
$ for repo in *; do git -C $repo fetch --prune; done

Why is this change necessary?

A future version of Hoff will include parallel builds of a merge train (#77), this is necessary so we can support that.