13

I'm working in a codebase at work that requires that anyone who wants to merge a PR rebase against the last master prior to merging the pull request. To further complicate things, CI (which takes hours) has to pass before the PR can be merged, so if someone merges a commit to master while CI is running, you have to rebase (even if your PR has no conflicts with the rebase, which is the case most of the time!), rerun CI, and merge and hope no one else merges into master.

I'm not really familiar with how the merge process works with PRs, but this just seems like a ridiculously long iteration process and doesn't seem feasible if it's a codebase that receives a lot of PRs..

How typical is this?

At my previous employer, the rebase and merge is done automatically for you after CI passes. 95%+ of the time it merges cleanly. The other 5% of the time, if there's a merge conflict, it won't merge your PR, and will ask you to manually rebase against master so you can manually correct the conflicts, and then try again. I liked this process a lot better, and i have no idea why we don't currently use it. Is it substantially harder to set up on github?

How is the merge criteria set up on github?

13
  • 14
    CI (which takes hours). Just curious: why? Sep 26 at 12:36
  • 1
    @EricDuminil I can't answer for OP, but at my current gig we have some long-running CI, can take three or four hours. Mostly because the build server is a pentium that's old enough to vote. And we sometimes compile large projects like buildroot in CI. (and yes, I'm aware that this sucks) Sep 26 at 13:29
  • 5
    In case it's not obvious: the fact that you're told to rebase you branch onto main (rather than merging main into your branch) makes no difference. People can argue as much as they like about whether it's better to rebase or to merge, but if you had to merge main into your branch instead then it wouldn't help at all with your problem :-) Sep 26 at 14:29
  • 1
    @EricDuminil we also have this problem, although not to the degree of the OP (45min~1hr in our case) because QA and the suits insist that we run E2E on every single merge, and we haven't successfully persuaded them better yet. Sep 26 at 16:33
  • 2
    @JaredSmith: Thanks. There's a lot of value in E2E tests, so I guess a compromise needs to be found between testing times and % of unit tests/mocking/E2E tests. It obviously depends a lot on the scope of the software and the number of test cases, but I find anything longer than 5min too long already. Sep 26 at 17:09

2 Answers 2

24

The core issue here is that your CI process takes hours. Everything else is standard good practice as far as many companies/developers are concerned.

The reason to always expect a feature branch to be reasonably up to date with master (whether you do this via a rebase or a merge is irrelevant for this particular point) is to ensure that IF there are any conflicts, they occur in the feature branch where the other people who share the main branch with you are not affected by the time it will take you to resolve these conflicts.

You may claim that there's no reason to think that your branch would cause any conflicts, but that's a bold claim and blindly accepting it would set a dangerous precedent. Eventually, someone will claim no conflict when there's actually a conflict, and that's precisely what people want to avoid.
"I know my work isn't the source of the problem, so I should be able to circumvent this" is both the solution to an impractical working situation and the cause of it.

As to why it's a rebase; there's a tendency to prefer rebases over merges because it means your commit history isn't littered with merge messages. I agree with that point, but I also find that rebases can get ugly/complex easily and when that happens I tend to want to deviate to just doing a merge instead.
Whether you opt for a merge or a rebase is a subjective decision. Each has their benefits but they're equivalent with relation to having a PR up to date with its target branch.

As a side point, it's generally a good idea to frequently merge master into your feature branch anyway, and when you do it frequently, it shouldn't be an issue to do it one more time (just before the PR) since the amount of changes pulled in will be minimal.

At my previous employer, the rebase and merge is done automatically for you after CI passes. 95%+ of the time it merges cleanly. The other 5% of the time, if there's a merge conflict, it won't merge your PR, and will ask you to manually rebase against master so you can manually correct the conflicts, and then try again.

The gap that this leaves is that you don't test to confirm that your merge hasn't introduced any bugs. If you run the tests first, and then merge the code, which is still an alteration to the code, how can you be sure that this didn't introduce a bug?

Your current employer's approach is safer in this regard. However, it does come at the cost of only running your tests after you merge, therefore needing to rerun your tests if you need to merge again. That's a cost/benefit analysis that I can't make based on the question you posted alone.

13
  • 6
    In the Rust community, the idea of gating the main branch/repository by an automated test process on the merged code is known as the Not Rocket Science Rule of Software Engineering. In such a system OP's problem that some else might invalidate the main branch doesn't occur because you chuck merge-ready work into an automated queue that either successfully integrates (merges, tests) the work or reports an error. It may also ve possible to roll up multiple items and test them together, speeding up testing for high-confidence changes.
    – amon
    Sep 26 at 4:55
  • 3
    @amon isn't this prone to the same problems? If the "known-good revision" you started from is no longer the latest one by the time someone approved your PR and the temporary revision is tested? Sep 26 at 8:29
  • 6
    @guillaume31 The trick is that no one updates the main branch except through this automated process, for example GitHub's "merge queue" feature or the Bors bot. At most one integration job will run at any time, so the merge base cannot be invalidated. But I agree with Flater that the real problem in OP's scenario is latency caused by a slow QA pipeline.
    – amon
    Sep 26 at 10:42
  • 1
    @amon Got you, I was not sure what "automated" entailed. It seems to solve some of the OP's problems indeed, similarly to the system in place at their previous company. Even with a very slow CI, at least you get the peace of mind that 95% of the time you don't have to look at a concurrently invalidated PR and the rest you'll be notified. Sep 26 at 11:06
  • 2
    I was a pytorch developer previously. The process was great. IIRC and unless it's changed, CI is run before merging, and during the merge process, it does a rebase and if there's any conflicts, the author of the PR has to fix it manually. if there's no conflict, the rebase is successful, and then CI is rerun again (IIRC) and then merged after CI passes
    – roulette01
    Sep 26 at 13:48
3

The specific setting in github is whether or not branches are required to be up to date with main in order to merge to main. You can turn that off. Your current build process presumably requires at least two things before merge: the branch must be up to date and the tests must pass. I don't believe it's uncommon for a repo to apply both these requirements: I work with many repos that have both, but their tests don't take hours to run.

If you change this to, "the rebase and merge is done automatically for you after CI passes", then you'll no longer be requiring that the up-to-date version of the branch passes the tests. You'll only be requiring that some version of your changes that has no conflicts with the main branch, passes the tests. Some people will consider that sufficient, and if you want to persuade your current team to change their rules then you need to convince them that it is.

Clearly it's a least a bit riskier than the stronger requirement, though. It's easy enough to come up with two changes that do not conflict in the git sense, since they don't change the same lines, and neither of which causes the tests to fail on its own, but which do cause the tests to fail once both of them are applied. The current process would catch this without merging to main: your proposed change to the process would not. But if it's the only way to get any work done then the team might accept the risk, especially if CI runs again on main after the merge. Then the risk is of a main branch clearly marked broken, not the risk of a main branch that's unknown to be broken until someone else rebases off it and tries to run the tests on their branch.

More generally the problem you're facing is that optimistic locking can cause extremely long delays with high contention. For that matter, pessimistic locking can also cause extremely long delays with high contention, but at least with the pessimistic locking you don't think you're making progress when you aren't!

Your optimistic lock looks like this:

  • rebase onto main. At this point you acquire the read-only part of an imaginary reader-writer lock
  • run tests (many hours)
  • attempt to acquire writer lock in order to merge: fails if any write has occurred since you acquired the read-only lock. If it fails, restart the transaction from scratch.

A pessimistic lock for the same process would look like this:

  • stand up in the middle of the room and loudly say, "I have a PR ready to go and I'm rebasing onto main. Nobody else merge to main until I'm done, please". Or maybe you can find some implementation other than shouting: old-timey VCS used to let you "lock" files in the repo pretty much for this reason (also to prevent conflicts even happening). But distributed version control doesn't really work like that.
  • run tests (many hours)
  • merge to main
  • stand up un the middle of the room and loudly say, "OK, other people can merge now if they like" - they are now free to stand up, take the lock, and start their transaction.

A further consideration for your team is that regardless of the locking mechanism, there is a fundamental time constraint. Since each PR's "final" run of the tests cannot start until the previous PR's run has completed, necessarily you cannot merge PRs faster than the tests run. This is a real problem when the tests take many hours: how many PRs per day does your team want to be capable of merging? Put like that, probably more than two in most teams, and ideally without having to log back into work just before bed to check on your build. This argument might help persuade the team to relax the merge requirements, and/or to find ways to shorten the build process.

github does have an auto-merge feature, which you can use, to merge your PR as soon as all requirements are met. This needs to be enabled per-repo IIRC. Once enabled, on each PR that's still running the tests you get an option to "set auto-complete" in place of the usual option to merge. For that matter, you can set auto-complete while the PR is still waiting for a required review, and if that's the only requirement missing then the PR will merge as soon as it's approved. It would help you in the "happy" case where nobody else merges during your testing window. It won't help in the unhappy case where they do.

There may be another option, which is to use fast-forward only merges into the main branch. The reason this helps is that if someone is ahead of you in the queue, then instead of rebasing onto main, you can rebase your branch onto their branch. That way, once they merge to main, your branch is instantly up to date with main! And maybe you've already tested your branch by then, in which case you can instantly merge. Victory! However, this doesn't work if they do a regular merge into main, since your branch won't have their merge commit, so it won't be up to date even though it's base on an identical revision. I've never actually done this as a routine process, but I do occasionally base my feature branch off some other feature branch that I know will go in ahead of me.

I'm not aware of a github feature to do what you really want, which is "at the end of a successful test run, if the branch is not up to date with main but doesn't conflict, then rebase and rerun all the tests". Combined with auto-commit, this would solve your problem, although potentially it still would run the tests repeatedly for days before it can actually merge. The fact I haven't seen this feature, though, doesn't mean it doesn't exist and also doesn't mean you can't figure out a way to make your build script do that. I can't remember what the best way is to set up the permissions, because I create repos fairly rarely and they need this kind of shenanigans extremely rarely. But with the right token you basically could do "git pull; git rebase origin/main; git push" directly in your build script.

4
  • 3
    > "aware of a github feature to do what you really want" - I believe this feature is called a "merge queue". Github automatically creates a branch with a copy of the merged code for testing just before actually merging that code in. docs.github.com/en/repositories/…
    – bdsl
    Sep 26 at 16:14
  • 1
    @bdsl: "just before" - or in this case, that would need to be some hours before :-) Sep 26 at 16:16
  • true - but it is does seem to be doing what the OP wants, as long as github is prepared to wait the hours for the CI provider to return a result.
    – bdsl
    Sep 26 at 16:25
  • @bdsl also known as "merge train" (i.e. on GitLab or various automation bots like bors).
    – Dan M.
    Sep 27 at 14:04

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.