5 ms·

Merge queues/merge trains solve a problem that only exists if you require all PRs/MRs to be based on the tip of the target branch. And there's little reason to

Merge queues/merge trains solve a problem that only exists if you require all PRs/MRs to be based on the tip of the target branch. And there's little reason to do that unless your codebase is a total mess.

muspimerol 3y ago
I don't understand this, how could you be confident your changes won't break something if they are not based on the tip of a branch? What if other conflicting changes have been merged in the mean time?
- barneygale 3y ago
  If conflicting changes have been merged in, GitLab/Hub will tell you that your branch has conflicts and prompt you to update/rebase on the target branch. That doesn't happen simply because your branch is out-of-date with the target.
  - muspimerol 3y ago
    There are two types of conflicts: literally changing the same lines of code (which you're talking about) and conflicts of business logic between features/modules (which I'm talking about). What if your PR depends on a database table that was dropped in the most recent commit? If your branch is based on an old commit, your tests might pass, because that table has not been deleted yet from your POV. But there will be a failure when you merge.
aseipp 3y ago
No, merge queue logic is still useful even if you use literal merges, or rebases. We used GitLab at my last job and had a "merge train" bot that would do more-or-less the exact same thing as GitHub does here, it would just literally use 'git merge' instead of 'git rebase'. No big difference in that regard. In any case, many teams and groups prefer reducing the number of merges to zero, if at all possible, and rebasing all final commits on top of trunk, to make the history shorter and more manageable. It isn't really that unusual at all these days. I've been working this way for years. But merge queues/merge trains are more a matter of team size/velocity/commit rate than it is anything to do with the merge algorithm, ultimately.
- barneygale 3y ago
  You've misread my comment. Whether you rebase on the target, or merge in from the target, the effect is the same. My point is that you only need to do that if you encounter git conflicts that must be solved manually.
  - aseipp 3y ago
    It's not about git conflicts, it's about behavior of the resulting repository and merging changes safely. You can absolutely have changes two changes A and B that have no conflicts to resolve textually, but can result collectively in a bad repository state (i.e. build is broken, tests fail) when both A+B are applied. For example, say A is a patch that renames the function foo() to foobar(), and then B is a patch that calls the function foo() at a completely new callsite. The original repository is green, and both A and B are individually green too. You merge A into main, then merge B without rebasing or re-merging main, and the build is broken. Each of these changes passed the tests in isolation, but together they will result in a repository that is broken. This has nothing to do with whether the codebase is a mess or not; it's just a simple rename of a function. B and A are simply mutually exclusive, and most be ordered concretely between each other. To fix this case manually, you have to merge A to main, then rebase B onto main (or merge main into B), which would then result in "function foo() not found", or your tests failing or build exploding. Now that the CI has caught it, you can change B to use the new function foobar() and re-attempt a merge again. Except you also have patches C-through-Z written by 5 other developers that might also conflict. This kind of example is everywhere in a large codebase; imagine that you're renaming files, changing parameter types to a function, reworking test output from debug statements, etc. And now imagine every CI run is 15 minutes long. If anyone merges in that 15 minutes before you, you have to start all over again. This also applies to all 5 developers of all other 20+ patches, too. The long and short is that there are a lot of cases where, to be safe, you need to just rebase your change on top of the latest tip first, before you can be 100% sure the build passes. Codebase cleanliness has nothing to do with it; that's just too pessimistic. The Merge Queue solves it a different way. Just queue up A and queue up B to be merged in series. Actually, the order doesn't matter at all. Let's say B is up first, then A, then a new patch C that is totally unrelated. The build passes with B applied, because it worked originally so it gets merged. Next up is A. A now fails, because even though it applied the patch successfully, there's a new call to the old function named foo(). So it gets kicked out. Now C is up immediately after. It succeeds, so it gets merged. At this point, the author of A is now responsible for rebasing their change and fixing the build. At no point did the author of either B or C have to be responsible for re-merging or rebasing their changes on top of main, as they triggered the happy case. The best way I can describe merge queue versus manually rebasing is this: the merge queue is optimistic locking, while manual rebasing is pessimistic locking. The time-to-merge a change is the latency. In an optimistic lock strategy, you always try to do the thing, but just detect if it fails and safely abort. The pessimistic case requires strict serialization of the operations to ensure no conflicts, but it needlessly holds up many concurrent writers. It has nothing to do with "messiness" of the data structures, to use an optimistic lock; you might just have a really writer-heavy system on your hands! If we keep putting it in latency/locking terms, this results in a much better "p90 time-to-merge latency", in other words. In a large team of developers, working on a big codebase, the "optimistic locking" approach of the merge queue is very effective at getting PRs merged faster, and has very few downsides.

muspimerol 3y ago

I don't understand this, how could you be confident your changes won't break something if they are not based on the tip of a branch? What if other conflicting changes have been merged in the mean time?

barneygale 3y ago
If conflicting changes have been merged in, GitLab/Hub will tell you that your branch has conflicts and prompt you to update/rebase on the target branch. That doesn't happen simply because your branch is out-of-date with the target.
- muspimerol 3y ago
  There are two types of conflicts: literally changing the same lines of code (which you're talking about) and conflicts of business logic between features/modules (which I'm talking about). What if your PR depends on a database table that was dropped in the most recent commit? If your branch is based on an old commit, your tests might pass, because that table has not been deleted yet from your POV. But there will be a failure when you merge.

aseipp 3y ago

No, merge queue logic is still useful even if you use literal merges, or rebases. We used GitLab at my last job and had a "merge train" bot that would do more-or-less the exact same thing as GitHub does here, it would just literally use 'git merge' instead of 'git rebase'. No big difference in that regard. In any case, many teams and groups prefer reducing the number of merges to zero, if at all possible, and rebasing all final commits on top of trunk, to make the history shorter and more manageable. It isn't really that unusual at all these days. I've been working this way for years. But merge queues/merge trains are more a matter of team size/velocity/commit rate than it is anything to do with the merge algorithm, ultimately.

barneygale 3y ago
You've misread my comment. Whether you rebase on the target, or merge in from the target, the effect is the same. My point is that you only need to do that if you encounter git conflicts that must be solved manually.
- aseipp 3y ago
  It's not about git conflicts, it's about behavior of the resulting repository and merging changes safely. You can absolutely have changes two changes A and B that have no conflicts to resolve textually, but can result collectively in a bad repository state (i.e. build is broken, tests fail) when both A+B are applied. For example, say A is a patch that renames the function foo() to foobar(), and then B is a patch that calls the function foo() at a completely new callsite. The original repository is green, and both A and B are individually green too. You merge A into main, then merge B without rebasing or re-merging main, and the build is broken. Each of these changes passed the tests in isolation, but together they will result in a repository that is broken. This has nothing to do with whether the codebase is a mess or not; it's just a simple rename of a function. B and A are simply mutually exclusive, and most be ordered concretely between each other. To fix this case manually, you have to merge A to main, then rebase B onto main (or merge main into B), which would then result in "function foo() not found", or your tests failing or build exploding. Now that the CI has caught it, you can change B to use the new function foobar() and re-attempt a merge again. Except you also have patches C-through-Z written by 5 other developers that might also conflict. This kind of example is everywhere in a large codebase; imagine that you're renaming files, changing parameter types to a function, reworking test output from debug statements, etc. And now imagine every CI run is 15 minutes long. If anyone merges in that 15 minutes before you, you have to start all over again. This also applies to all 5 developers of all other 20+ patches, too. The long and short is that there are a lot of cases where, to be safe, you need to just rebase your change on top of the latest tip first, before you can be 100% sure the build passes. Codebase cleanliness has nothing to do with it; that's just too pessimistic. The Merge Queue solves it a different way. Just queue up A and queue up B to be merged in series. Actually, the order doesn't matter at all. Let's say B is up first, then A, then a new patch C that is totally unrelated. The build passes with B applied, because it worked originally so it gets merged. Next up is A. A now fails, because even though it applied the patch successfully, there's a new call to the old function named foo(). So it gets kicked out. Now C is up immediately after. It succeeds, so it gets merged. At this point, the author of A is now responsible for rebasing their change and fixing the build. At no point did the author of either B or C have to be responsible for re-merging or rebasing their changes on top of main, as they triggered the happy case. The best way I can describe merge queue versus manually rebasing is this: the merge queue is optimistic locking, while manual rebasing is pessimistic locking. The time-to-merge a change is the latency. In an optimistic lock strategy, you always try to do the thing, but just detect if it fails and safely abort. The pessimistic case requires strict serialization of the operations to ensure no conflicts, but it needlessly holds up many concurrent writers. It has nothing to do with "messiness" of the data structures, to use an optimistic lock; you might just have a really writer-heavy system on your hands! If we keep putting it in latency/locking terms, this results in a much better "p90 time-to-merge latency", in other words. In a large team of developers, working on a big codebase, the "optimistic locking" approach of the merge queue is very effective at getting PRs merged faster, and has very few downsides.